Replaced serde_cbor with ciborium - #56
Conversation
| members = [ ".", "nsm-lib", "nsm-test" ] | ||
| default-members = [ "." ] | ||
| members = [".", "nsm-lib", "nsm-test"] | ||
| default-members = ["."] |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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>), |
There was a problem hiding this comment.
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,
}
There was a problem hiding this comment.
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:
- Our
Error::Cborcan't exist anymore, because it would requiredTin situations where it doesn't make sense to provideT. This would haveAttestationDoc::from_binaryreturningciborium::de::Error<T>, which is really just another abstraction leak. - It's a breaking API change: we'd have to remove the error variant, update
from_binaryto 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() { |
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
An alternative to this expect could be to swap around how we enforce NSM_REQUEST_MAX_SIZE.
The current flow looks like this:
- Create a resizable buffer, initialized with
NSM_REQUEST_MAX_SIZEcapacity, and provide a request of unchecked size tonsm_encode_request_to_cbor - Successfully CBOR encode the request
- 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.
91426c9 to
eccddcf
Compare
|
advocating for this update given |
2 participants
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.