Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI checks for build, test, lint, and dep vulnerabilities #23

Merged
merged 28 commits into from
Jun 15, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/audit.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# Perform a audit any time a change is dependencies is detected in Cargo.toml or Cargo.lock
# See: https://github.com/actions-rs/audit-check

name: Security audit
on:
push:
paths:
- '**/Cargo.toml'
- '**/Cargo.lock'
pull_request:
emostov marked this conversation as resolved.
Show resolved Hide resolved
paths:
- '**/Cargo.toml'
- '**/Cargo.lock'

jobs:
security_audit:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
- uses: actions-rs/audit-check@v1
with:
token: ${{ secrets.GITHUB_TOKEN }}
74 changes: 74 additions & 0 deletions .github/workflows/pr.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# Checks for PRs
# See: https://github.com/actions-rs/example/blob/master/.github/workflows/quickstart.yml

on: [pull_request]

name: PR

jobs:
Copy link

@keyz keyz Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense for "Checkout + fetch dependencies" to be a separate job, and have other jobs depend on that? I guess this way we can use Cargo.lock as the key for caching the toolchain and dependencies across CI runs

Copy link
Contributor Author

@emostov emostov Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could "Checkout + fetch dependencies" and then have subsequent jobs run in the same environment it makes sense because it saves us for having to do it for each. But AFAICT every job runs in its own environment so it won't save us any time but instead just make sure subsequent jobs don't run if it fails?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup every job runs in its own environment, however there's a ~global cache that every job can read/write. I think we can have the "checkout + fetch" job write to the cache, then let other jobs restore the cache: maybe something like this

# build:
# name: Build
# runs-on: ubuntu-latest
# strategy:
# matrix:
# target:
# - ubuntu-latest
# - macOS-latest
# steps:
# - uses: actions/checkout@v2
# - uses: actions-rs/toolchain@v1
# with:
# toolchain: stable
# target: ${{ matrix.target }}
# override: true
# - uses: actions-rs/cargo@v1
# with:
# use-cross: true
# command: build
# args: --release --target=${{ matrix.target }}

test:
name: Test Suite
runs-on: ubuntu-latest
steps:
- name: Checkout sources
uses: actions/checkout@v2

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true

- name: Run cargo test
uses: actions-rs/cargo@v1
with:
command: test

lints:
name: Lints
runs-on: ubuntu-latest
steps:
- name: Checkout sources
uses: actions/checkout@v2

- name: Install stable toolchain
uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true
components: rustfmt, clippy

- name: Run cargo fmt
uses: actions-rs/cargo@v1
with:
command: fmt
args: --all -- --check

- name: Run cargo clippy
uses: actions-rs/cargo@v1
with:
command: clippy
args: -- -D warnings
18 changes: 9 additions & 9 deletions qos-client/src/attest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub mod nitro {
/// AWS Nitro root CA certificate.
/// This should be validated against the below checksum:
/// `8cf60e2b2efca96c6a9e71e851d00c1b6991cc09eadbe64a6a1d1b1eb9faff7c`
pub(crate) const AWS_ROOT_CERT: &'static [u8] =
pub(crate) const AWS_ROOT_CERT: &[u8] =
std::include_bytes!("./aws_root_cert.pem");

/// Extract a DER encoded certificate from bytes representing a PEM encoded
Expand Down Expand Up @@ -126,7 +126,7 @@ pub mod nitro {

/// Verify the certificate chain against the root & end entity certificates.
fn verify_certificate_chain(
cabundle: &Vec<ByteBuf>,
cabundle: &[ByteBuf],
root_cert: &[u8],
end_entity_certificate: &[u8],
validation_time: u64,
Expand All @@ -136,7 +136,7 @@ pub mod nitro {
// (first element). Ordering is: root cert .. intermediate certs ..
// end entity cert.
let intermediate_certs: Vec<_> =
cabundle[1..].into_iter().map(|x| x.as_slice()).collect();
cabundle[1..].iter().map(|x| x.as_slice()).collect();

let anchor = vec![webpki::TrustAnchor::try_from_cert_der(root_cert)?];
let anchors = webpki::TlsServerTrustAnchors(&anchor);
Expand All @@ -148,7 +148,7 @@ pub mod nitro {
&intermediate_certs,
webpki::Time::from_seconds_since_unix_epoch(validation_time),
)
.map_err(|e| AttestError::InvalidCertChain(e))?;
.map_err(AttestError::InvalidCertChain)?;

Ok(())
}
Expand Down Expand Up @@ -187,8 +187,8 @@ pub mod nitro {
use super::*;

/// Mandatory field
pub(super) fn module_id(id: &String) -> Result<(), AttestError> {
if id.len() < 1 {
pub(super) fn module_id(id: &str) -> Result<(), AttestError> {
if id.is_empty() {
Err(AttestError::InvalidModuleId)
} else {
Ok(())
Expand All @@ -198,7 +198,7 @@ pub mod nitro {
pub(super) fn pcrs(
pcrs: &BTreeMap<usize, ByteBuf>,
) -> Result<(), AttestError> {
let is_valid_pcr_count = pcrs.len() > 0 && pcrs.len() <= 32;
let is_valid_pcr_count = !pcrs.is_empty() && pcrs.len() <= 32;

let is_valid_index_and_len = pcrs.iter().all(|(idx, pcr)| {
let is_valid_idx = *idx <= 32;
Expand All @@ -214,9 +214,9 @@ pub mod nitro {
}
/// Mandatory field
pub(super) fn cabundle(
cabundle: &Vec<ByteBuf>,
cabundle: &[ByteBuf],
) -> Result<(), AttestError> {
let is_valid_len = cabundle.len() > 0;
let is_valid_len = !cabundle.is_empty();
let is_valid_entries = cabundle
.iter()
.all(|cert| cert.len() >= 1 || cert.len() <= 1024);
Expand Down
19 changes: 9 additions & 10 deletions qos-client/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@ impl Command {
}
}
}
impl Into<Command> for &str {
fn into(self) -> Command {
match self {

impl From<&str> for Command {
fn from(s: &str) -> Command {
match s {
"health" => Command::Health,
"echo" => Command::Echo,
"describe-nsm" => Command::DescribeNsm,
Expand All @@ -46,7 +47,6 @@ struct ClientOptions {
cmd: Command,
host: HostOptions,
echo: EchoOptions,
// ... other options
}
impl ClientOptions {
/// Create `ClientOptions` from the command line arguments.
Expand All @@ -59,14 +59,14 @@ impl ClientOptions {
};

let mut chunks = args.chunks_exact(2);
if chunks.remainder().len() > 0 {
if !chunks.remainder().is_empty() {
panic!("Unexpected number of arguments");
}

while let Some([cmd, arg]) = chunks.next() {
options.host.parse(&cmd, &arg);
options.host.parse(cmd, arg);
match options.cmd {
Command::Echo => options.echo.parse(&cmd, arg),
Command::Echo => options.echo.parse(cmd, arg),
Command::Health => {}
Command::DescribeNsm => {}
Command::MockAttestation => {}
Expand Down Expand Up @@ -104,9 +104,8 @@ impl EchoOptions {
Self { data: None }
}
fn parse(&mut self, cmd: &str, arg: &str) {
match cmd {
"--data" => self.data = Some(arg.to_string()),
_ => {}
if cmd == "--data" {
self.data = Some(arg.to_string())
};
}
fn data(&self) -> String {
Expand Down
2 changes: 1 addition & 1 deletion qos-client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub mod request {
.read_to_end(&mut buf)
.map_err(|_| "send response error".to_string())?;

let pr = serde_cbor::from_slice(&mut buf)
let pr = serde_cbor::from_slice(&buf)
.map_err(|_| "send response error".to_string())?;

Ok(pr)
Expand Down
52 changes: 22 additions & 30 deletions qos-core/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::{
server::SocketServer,
};

#[derive(Clone, Debug, PartialEq)]
#[derive(Default, Clone, Debug, PartialEq)]
pub struct EnclaveOptions {
cid: Option<u32>,
port: Option<u32>,
Expand All @@ -16,14 +16,14 @@ pub struct EnclaveOptions {

impl EnclaveOptions {
pub fn new() -> Self {
Self { cid: None, port: None, usock: None, mock: false }
Default::default()
}

fn from(args: Vec<String>) -> EnclaveOptions {
let mut options = EnclaveOptions::new();

let mut chunks = args.chunks_exact(2);
if chunks.remainder().len() > 0 {
if !chunks.remainder().is_empty() {
panic!("Unexepected number of arguments")
}

Expand All @@ -42,45 +42,37 @@ impl EnclaveOptions {
}

pub fn parse_cid(&mut self, cmd: &str, arg: &str) {
match cmd {
"--cid" => {
self.cid = arg
.parse::<u32>()
.map_err(|_| {
panic!("Could not parse provided value for `--cid`")
})
.ok();
}
_ => {}
if cmd == "--cid" {
self.cid = arg
.parse::<u32>()
.map_err(|_| {
panic!("Could not parse provided value for `--cid`")
})
.ok();
}
}

pub fn parse_port(&mut self, cmd: &str, arg: &str) {
match cmd {
"--port" => {
self.port = arg
.parse::<u32>()
.map_err(|_| {
panic!("Could not parse provided value for `--port`")
})
.ok();
}
_ => {}
if cmd == "--port" {
self.port = arg
.parse::<u32>()
.map_err(|_| {
panic!("Could not parse provided value for `--port`")
})
.ok();
}
}

pub fn parse_usock(&mut self, cmd: &str, arg: &str) {
match cmd {
"--usock" => self.usock = Some(arg.to_string()),
_ => {}
if cmd == "--usock" {
self.usock = Some(arg.to_string())
}
}

pub fn parse_mock(&mut self, cmd: &str, arg: &str) {
match cmd {
"--mock" => self.mock = arg == "true",
_ => {}
}
if cmd == "--mock" {
self.mock = arg == "true"
};
}

pub fn addr(&self) -> SocketAddress {
Expand Down
4 changes: 2 additions & 2 deletions qos-core/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl Client {
&serde_cbor::to_vec(&request)
.expect("ProtocolMsg can be serialized. qed."),
)?;
let mut response = stream.recv()?;
serde_cbor::from_slice(&mut response).map_err(Into::into)
let response = stream.recv()?;
serde_cbor::from_slice(&response).map_err(Into::into)
}
}
9 changes: 3 additions & 6 deletions qos-core/src/io/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ pub struct Stream {
}

impl Stream {
#[must_use]
pub(crate) fn connect(addr: &SocketAddress) -> Result<Self, IOError> {
let mut err = IOError::UnknownError;

Expand All @@ -88,8 +87,7 @@ impl Stream {
Err(err)
}

#[must_use]
pub(crate) fn send(&self, buf: &Vec<u8>) -> Result<(), IOError> {
pub(crate) fn send(&self, buf: &[u8]) -> Result<(), IOError> {
let len = buf.len();

// First, send the length of the buffer
Expand Down Expand Up @@ -130,7 +128,6 @@ impl Stream {
Ok(())
}

#[must_use]
pub(crate) fn recv(&self) -> Result<Vec<u8>, IOError> {
// First, read the length
let length: usize = {
Expand Down Expand Up @@ -264,7 +261,7 @@ fn socket_fd(addr: &SocketAddress) -> Result<RawFd, IOError> {
// is both a type and protocol.
None,
)
.map_err(|e| IOError::NixError(e))
.map_err(IOError::NixError)
}

#[cfg(test)]
Expand Down Expand Up @@ -306,7 +303,7 @@ mod test {
while let Some(stream) = listener.next() {
let req = stream.recv().unwrap();
stream.send(&req).unwrap();
break
break;
}
});

Expand Down
2 changes: 1 addition & 1 deletion qos-core/src/protocol/attestor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub trait NsmProvider {
pub struct Nsm;
impl NsmProvider for Nsm {
fn nsm_process_request(&self, fd: i32, request: NsmRequest) -> NsmResponse {
nsm::driver::nsm_process_request(fd, request.into()).into()
nsm::driver::nsm_process_request(fd, request)
}

fn nsm_init(&self) -> i32 {
Expand Down
4 changes: 2 additions & 2 deletions qos-core/src/protocol/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,13 @@ impl Executor {
}

impl server::Routable for Executor {
fn process(&mut self, mut req_bytes: Vec<u8>) -> Vec<u8> {
fn process(&mut self, req_bytes: Vec<u8>) -> Vec<u8> {
if req_bytes.len() > MAX_ENCODED_MSG_LEN {
return serde_cbor::to_vec(&ProtocolMsg::ErrorResponse)
.expect("ProtocolMsg can always be serialized. qed.");
}

let msg_req = match serde_cbor::from_slice(&mut req_bytes) {
let msg_req = match serde_cbor::from_slice(&req_bytes) {
Ok(req) => req,
Err(_) => {
return serde_cbor::to_vec(&ProtocolMsg::ErrorResponse)
Expand Down
Loading