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

Replaced serde_cbor with ciborium #56

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ categories = ["hardware-support"]
repository = "https://github.com/aws/aws-nitro-enclaves-nsm-api"

[workspace]
members = [ ".", "nsm-lib", "nsm-test" ]
default-members = [ "." ]
members = [".", "nsm-lib", "nsm-test"]
default-members = ["."]
Copy link
Author

Choose a reason for hiding this comment

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

My formatter noticed that the cargo.toml was inconsistent here. Happy to revert these changes if they're view as a problem.


[profile.release]
lto = true
Expand All @@ -23,7 +23,7 @@ libc = "0.2"
log = "0.4"
serde = { version = "1.0", features = ["derive"] }
serde_bytes = "0.11"
serde_cbor = "0.11"
ciborium = "0.2"

[features]
default = ["nix"]
1 change: 0 additions & 1 deletion nsm-test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ config_emulated = []

[dependencies]
serde_bytes = "0.11"
serde_cbor = "0.11"
aws-nitro-enclaves-nsm-api = { path = "../" }
nsm-lib = { path = "../nsm-lib" }
nix = "0.26"
Expand Down
18 changes: 6 additions & 12 deletions src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,14 @@ use std::result;

use serde::{Deserialize, Serialize};
use serde_bytes::ByteBuf;
use serde_cbor::error::Error as CborError;
use serde_cbor::{from_slice, to_vec};

#[derive(Debug)]
/// Possible error types return from this library.
pub enum Error {
/// An IO error of type `std::io::Error`
Io(IoError),
/// A CBOR ser/de error of type `serde_cbor::error::Error`.
Cbor(CborError),
/// A CBOR de error of type `ciborium::de::Error<std::io::Error>`.
Cbor(ciborium::de::Error<std::io::Error>),
Copy link
Author

@tannaurus tannaurus Oct 14, 2024

Choose a reason for hiding this comment

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

This (<std::io::Error>) feels like an abstraction leak. Happy to rethink this if the maintainers want to avoid that. One could also make a very strong argument that because this isn't pub, this doesn't matter.

ciborium::de::Error contains the following variants:

/// An error occurred during deserialization
#[derive(Debug)]
pub enum Error<T> {
    /// An error occurred while reading bytes
    ///
    /// Contains the underlying error returned while reading.
    Io(T),

    /// An error occurred while parsing bytes
    ///
    /// Contains the offset into the stream where the syntax error occurred.
    Syntax(usize),

    /// An error occurred while processing a parsed value
    ///
    /// Contains a description of the error that occurred and (optionally)
    /// the offset into the stream indicating the start of the item being
    /// processed when the error occurred.
    Semantic(Option<usize>, String),

    /// The input caused serde to recurse too much
    ///
    /// This error prevents a stack overflow.
    RecursionLimitExceeded,
}

Error::Io should be infallible due to this using &[u8]'s Read implementation, so one could argue that ciborium::de::Error using a generic, in this use case, is unnecessary. If we all agree there, I'm thinking the alternative would be to re-create the error variants we care about, and map ciborium::de::Error to them in a From impl. Something like this:

pub enum CborError {
    /// An error occurred while parsing bytes
    ///
    /// Contains the offset into the stream where the syntax error occurred.
    Syntax(usize),

    /// An error occurred while processing a parsed value
    ///
    /// Contains a description of the error that occurred and (optionally)
    /// the offset into the stream indicating the start of the item being
    /// processed when the error occurred.
    Semantic(Option<usize>, String),

    /// The input caused serde to recurse too much
    ///
    /// This error prevents a stack overflow.
    RecursionLimitExceeded,
}

Copy link
Author

Choose a reason for hiding this comment

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

The second alternative would be to allow downstream consumers to provide their own Reader. This would only be used inside AttestationDoc::from_binary, the other usage of ciborium::from_reader assumes an internal error.

A couple problems with that:

  1. Our Error::Cbor can't exist anymore, because it would required T in situations where it doesn't make sense to provide T. This would have AttestationDoc::from_binary returning ciborium::de::Error<T>, which is really just another abstraction leak.
  2. It's a breaking API change: we'd have to remove the error variant, update from_binary to expect a buffer, etc.

I threw that idea out pretty early but felt like I'd include it here for group consensus.

}

/// Result type return nsm-io::Error on failure.
Expand All @@ -37,12 +35,6 @@ impl From<IoError> for Error {
}
}

impl From<CborError> for Error {
fn from(error: CborError) -> Self {
Error::Cbor(error)
}
}

/// List of error codes that the NSM module can return as part of a Response
#[repr(C)]
#[derive(Debug, Serialize, Deserialize)]
Expand Down Expand Up @@ -290,13 +282,15 @@ impl AttestationDoc {
/// Helper function that converts an AttestationDoc structure to its CBOR representation
pub fn to_binary(&self) -> Vec<u8> {
// This should not fail
to_vec(self).unwrap()
let mut buf = Vec::with_capacity(1024);
ciborium::into_writer(self, &mut buf).unwrap();
buf
}

/// Helper function that parses a CBOR representation of an AttestationDoc and creates the
/// structure from it, if possible.
pub fn from_binary(bin: &[u8]) -> Result<Self> {
from_slice(bin).map_err(Error::Cbor)
ciborium::from_reader(bin).map_err(Error::Cbor)
}
}

Expand Down
63 changes: 41 additions & 22 deletions src/driver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use nix::request_code_readwrite;
use nix::unistd::close;

use std::fs::OpenOptions;
use std::io::{IoSlice, IoSliceMut};
use std::io::IoSlice;
use std::mem;
use std::os::unix::io::{IntoRawFd, RawFd};

Expand All @@ -35,29 +35,29 @@ struct NsmMessage<'a> {
/// User-provided data for the request
pub request: IoSlice<'a>,
/// Response data provided by the NSM pipeline
pub response: IoSliceMut<'a>,
pub response: IoSlice<'a>,
}

/// Encode an NSM `Request` value into a vector.
/// *Argument 1 (input)*: The NSM request.
/// *Returns*: The vector containing the CBOR encoding.
fn nsm_encode_request_to_cbor(request: Request) -> Vec<u8> {
serde_cbor::to_vec(&request).unwrap()
/// Encode an NSM `Request` value into a vector.
/// *Argument 1 (input)*: The NSM request.
/// *Argument 2 (input)*: The buffer that will have the CBOR written into it.
fn nsm_encode_request_to_cbor(request: Request, buf: &mut Vec<u8>) {
ciborium::into_writer(&request, buf).expect("buf's Writer returned an unexpected error");
Copy link
Author

@tannaurus tannaurus Oct 14, 2024

Choose a reason for hiding this comment

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

An alternative to this expect could be to swap around how we enforce NSM_REQUEST_MAX_SIZE.

The current flow looks like this:

  1. Create a resizable buffer, initialized with NSM_REQUEST_MAX_SIZE capacity, and provide a request of unchecked size to nsm_encode_request_to_cbor
  2. Successfully CBOR encode the request
  3. Check the length of CBOR, returning an error if it exceeds NSM_REQUEST_MAX_SIZE

This could be turned on its head so that into_writer is given a non-resizable buf. If the request exceeded NSM_REQUEST_MAX_SIZE, we'd encounter a WriteZero. This would remove the need for the additional length check as that would be enforced when writing: a write attempt would fail when it runs out of space to write.

}

/// Decode an NSM `Response` value from a raw memory buffer.
/// *Argument 1 (input)*: The `iovec` holding the memory buffer.
/// Decode an NSM `Response` value from a raw memory buffer.
/// *Argument 1 (input)*: The `iovec` holding the memory buffer.
/// *Returns*: The decoded NSM response.
fn nsm_decode_response_from_cbor(response_data: &IoSliceMut<'_>) -> Response {
match serde_cbor::from_slice(response_data) {
fn nsm_decode_response_from_cbor(response_data: &IoSlice<'_>) -> Response {
match ciborium::from_reader(response_data.as_ref()) {
Ok(response) => response,
Err(_) => Response::Error(ErrorCode::InternalError),
}
}

/// Do an `ioctl()` of a given type for a given message.
/// *Argument 1 (input)*: The descriptor to the device file.
/// *Argument 2 (input/output)*: The message to be sent and updated via `ioctl()`.
/// Do an `ioctl()` of a given type for a given message.
/// *Argument 1 (input)*: The descriptor to the device file.
/// *Argument 2 (input/output)*: The message to be sent and updated via `ioctl()`.
/// *Returns*: The status of the operation.
fn nsm_ioctl(fd: i32, message: &mut NsmMessage) -> Option<Errno> {
let status = unsafe {
Expand All @@ -80,22 +80,23 @@ fn nsm_ioctl(fd: i32, message: &mut NsmMessage) -> Option<Errno> {

/// Create a message with input data and output capacity from a given
/// request, then send it to the NSM driver via `ioctl()` and wait
/// for the driver's response.
/// *Argument 1 (input)*: The descriptor to the NSM device file.
/// *Argument 2 (input)*: The NSM request.
/// for the driver's response.
/// *Argument 1 (input)*: The descriptor to the NSM device file.
/// *Argument 2 (input)*: The NSM request.
/// *Returns*: The corresponding NSM response from the driver.
pub fn nsm_process_request(fd: i32, request: Request) -> Response {
let cbor_request = nsm_encode_request_to_cbor(request);
let mut cbor_request = Vec::with_capacity(NSM_REQUEST_MAX_SIZE);
nsm_encode_request_to_cbor(request, &mut cbor_request);

// Check if the request is too large
if cbor_request.len() > NSM_REQUEST_MAX_SIZE {
return Response::Error(ErrorCode::InputTooLarge);
}

let mut cbor_response: [u8; NSM_RESPONSE_MAX_SIZE] = [0; NSM_RESPONSE_MAX_SIZE];
let cbor_response: [u8; NSM_RESPONSE_MAX_SIZE] = [0; NSM_RESPONSE_MAX_SIZE];
let mut message = NsmMessage {
request: IoSlice::new(&cbor_request),
response: IoSliceMut::new(&mut cbor_response),
response: IoSlice::new(&cbor_response),
Copy link
Author

Choose a reason for hiding this comment

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

This didn't appear to need to be mut, even in the previous implementation. I planned on relying on the test suite to prove me wrong but was unable to successfully follow the testing instructions (see PR description)

};
let status = nsm_ioctl(fd, &mut message);

Expand All @@ -108,7 +109,7 @@ pub fn nsm_process_request(fd: i32, request: Request) -> Response {
}
}

/// NSM library initialization function.
/// NSM library initialization function.
/// *Returns*: A descriptor for the opened device file.
pub fn nsm_init() -> i32 {
let mut open_options = OpenOptions::new();
Expand All @@ -126,7 +127,7 @@ pub fn nsm_init() -> i32 {
}
}

/// NSM library exit function.
/// NSM library exit function.
/// *Argument 1 (input)*: The descriptor for the opened device file, as
/// obtained from `nsm_init()`.
pub fn nsm_exit(fd: i32) {
Expand All @@ -136,3 +137,21 @@ pub fn nsm_exit(fd: i32) {
Err(e) => error!("File of descriptor {} failed to close: {}", fd, e),
}
}

#[cfg(test)]
mod test {
use super::{Request, NSM_REQUEST_MAX_SIZE};
#[test]
fn test_nsm_encode_request_to_cbor_exceed_max_size_without_panic() {
Copy link
Author

Choose a reason for hiding this comment

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

This is just here to ensure if anyone in the future decides to try to get clever with the buf type, we'll see this panic. For instance, you can't provide &mut [u8] here, because it won't resize itself: it will hit nsm_encode_request_to_cbor's expect

let request = Request::ExtendPCR {
index: 0,
data: vec![0u8; NSM_REQUEST_MAX_SIZE],
};

let mut buf = Vec::with_capacity(NSM_REQUEST_MAX_SIZE);
super::nsm_encode_request_to_cbor(request, &mut buf);

// Ensure the buffer increased its capacity without panicking
assert!(buf.len() > NSM_REQUEST_MAX_SIZE);
}
}