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

Conversation

tannaurus
Copy link

Issue #, if available:

#19

Description of changes:

Replaces serde_cbor, which is unmaintained, with ciborium. I selected ciborium due to it appearing to meet the requirements, mixed with my existing familiar with its API + its usage in Google's coset, hopefully resolving any concerns with its reliability.

Testing
I was unable to follow the testing instructions that are scattered throughout this repo. I feel as though I may be missing something, but the tests are consistency not run. I am on MacOS, which may (?) be behind my issue; I'm not sure. This is my first direct contribution to this repo. If anyone has advice on how to successfully run the test suite on MacOS, I'd appreciate a point in the right direction.

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.

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)

/// 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.

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

/// *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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant