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

aes-kw: no_std support + other cleanups #7

Merged
merged 3 commits into from Jan 6, 2022

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Jan 6, 2022

  • Makes the Kek::{wrap, unwrap} functions take an output buffer rather than allocating a Vec<u8>, allowing them to be used in heapless no_std contexts.
  • Adds an alloc feature along with Kek::{wrap_vec, unwrap_vec} functions which provided the previous API.
  • Adds convenience KekAes128/KekAes192/KekAes256 type aliases.
  • Makes the IV and IV_LEN constants public.
  • Removes the use of hex for test vectors replacing it with hex-literal instead.
  • Improves README.md and rustdoc documentation.

- Makes the `Kek::{wrap, unwrap}` functions take an output buffer rather
  than allocating a `Vec<u8>`, allowing them to be used in heapless
  `no_std` contexts.
- Adds an `alloc` feature along with `Kek::{wrap_vec, unwrap_vec}`
  functions which provided the previous API.
- Adds convenience `KekAes128`/`KekAes192`/`KekAes256` type aliases.
- Makes the `IV` and `IV_LEN` constants public.
- Removes the use of `hex` for test vectors replacing it with
  `hex-literal` instead.
- Improves README.md and rustdoc documentation.
Comment on lines 9 to 19
/// Input data length invalid.
InvalidDataLength,
/// Invalid kek size.
InvalidKekSize(usize),
/// Output buffer size invalid.
InvalidOutputSize {
/// Expected size in bytes.
expected: usize,
},
/// Integrity check did not pass.
IntegrityCheckFailed,
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like these could be a little more consistent:

  • *Length vs *Size
  • Tuple variant vs one with named fields

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd probably suggest settling on *Size (it's what we tend to use in other crates, and also shorter).

I'd also suggest changing InvalidKekSize { size: usize } for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, makes sense to standardize on size

@@ -8,8 +9,41 @@
#![forbid(unsafe_code)]
#![warn(missing_docs, rust_2018_idioms)]

//! # Usage
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to move the usage out of README.md in order to be able to feature-gate the doctests, unfortunately.

Otherwise they fail unless the correct features are available.

aes-kw/src/kek.rs Outdated Show resolved Hide resolved
aes-kw/src/kek.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri force-pushed the aes-kw/no_std+other-fixups branch 2 times, most recently from ed4ba84 to 4415799 Compare January 6, 2022 19:36
Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

nice!

@tarcieri tarcieri merged commit ded9053 into master Jan 6, 2022
@tarcieri tarcieri deleted the aes-kw/no_std+other-fixups branch January 6, 2022 20:15
@tarcieri
Copy link
Member Author

tarcieri commented Jan 6, 2022

I think this is ready to release then?

@dignifiedquire anything else? Should I cut a release or do you want to?

@cryptographix
Copy link
Contributor

Functionally, looks good. Just upd Deno PR to use latest commit, and is now passing 100% of WPT (WebCrypto/wrapKey_unwrapKey) and JOSE (AxxxKW) tests!

Great work, guys. Thanks a lot for your effort and patience ;)

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.

None yet

3 participants