Feat: Implement ASCON Lightweight Cryptographic Libraries - #21
Conversation
|
@officialfrancismendoza I see this. On my todo list! |
29dadd5 to
7120e4a
Compare
… much easier review
…Updated summary.md as well
…ulting from changes in core that I have to build around
937be59 to
0f15f1f
Compare
…llible (guards absorb after squeeze), AEAD conformed to main SymmetricCipher and AEADCipher in release/0.1.2.alpha, use Suspendable for hash/XOF/CXOF and SuspendableKeyed for AEAD (bcgit#21)
…llible (guards absorb after squeeze), AEAD conformed to main SymmetricCipher and AEADCipher in release/0.1.2.alpha, use Suspendable for hash/XOF/CXOF and SuspendableKeyed for AEAD (bcgit#21)
|
@officialfrancismendoza, a couple things on this one so far:
|
There was a problem hiding this comment.
First review:
This PR is currently failing the rustfmt check. Just run $ cargo fmt in the root and commit again.
The Crucible project only has tests for ML-KEM and ML-DSA. So does not apply here.
There are ascon vectors in both Wycheproof and bc-test-data, so you should be able to take the test harnesses that we have in, for example, ml-dsa and port that over for ascon.
(I will continue with a code review, though I admit that +3,900 is a lot to review, so I will have to do this in chunks.)
| @@ -0,0 +1,262 @@ | |||
| # ASCON Implementation & Testing — Work Summary | |||
There was a problem hiding this comment.
This file seems maybe like it is your personal notes that were checked in by accident? I don't see any long-term value in including this file in the source code.
| //! [`bouncycastle_utils::secret::Secret`] wrappers, so they are scrubbed with volatile writes | ||
| //! when the value is dropped. The hash/XOF states are likewise `Secret`-wrapped. | ||
| //! - **Decryption release:** never release decrypted plaintext until finalization returns `Ok`; an | ||
| //! `Err(AuthenticationFailed)` means the ciphertext or tag was tampered with. |
There was a problem hiding this comment.
I like this point and I think it should be expanded. I'm thinking something like:
Decryption tag check failure: a ciphertext decryption whose finalization returns an
Err(AuthenticationFailed)should be considered to be tampered with and the entire plaintext should be rejected. For example, if the plaintext being decrypted is large enough that it must be processed by the application in a streaming fashion, the application should have a way to cancel the operation or transaction with an error if ASCON finalization returns an AuthenticationFailed error.
| //! h.update_bytes(b"hello "); | ||
| //! h.update_bytes(b"world"); | ||
| //! let mut out = [0u8; 32]; | ||
| //! h.do_final_into(&mut out); |
There was a problem hiding this comment.
The rest of the library uses do_final_out() for this pattern. I actually kinda like _into instead of _out, but one way or another this should be made consistent with the rest of the library.
If you think that _into is the better name, than why don't you open a github issue to propose that change, and then create a PR against 0.1.3alpha.
| //! ``` | ||
| //! use bouncycastle_ascon::ascon_aead128::AsconAead128; | ||
| //! | ||
| //! let key = [0u8; 16]; |
There was a problem hiding this comment.
Why is this not a KeyMaterial?
| //! | ||
| //! let mut ct = vec![0u8; plaintext.len() + 16]; // ciphertext || 16-byte tag | ||
| //! let n = AsconAead128::encrypt(&key, &nonce, Some(ad), plaintext, &mut ct); | ||
| //! ct.truncate(n); |
There was a problem hiding this comment.
This API pattern seems extremely weird to me.
First the user has to create a mut vec of the correct size plaintext.len() + 16.
Then the user has to hand that in to the encrypt function.
Then the user has to truncate it because it might actually be shorter than plaintext.len() + 16 ?
I might expect some acrobatics like that in no_std where the user has to hand in a [u8; N] of the correct size, but the whole point of Vec's is to avoid making the user deal with array lengths.
If we're using vec here, then can't we hide this inside the function call and -> Vec<u8> that we size correctly?
| /// Algorithm name for Ascon-XOF128. | ||
| pub const ASCON_XOF128_NAME: &str = "Ascon-XOF128"; | ||
| /// Algorithm name for Ascon-CXOF128. | ||
| pub const ASCON_CXOF128_NAME: &str = "Ascon-CXOF128"; |
There was a problem hiding this comment.
All the ASCON variants should impl traits::Algorithm and maybe traits::HashAlgParams?
| //! These replace the external `arrayref` crate so that this crate carries no third-party runtime | ||
| //! dependencies (per the project's QUALITY_AND_STYLE rules). All callers pass slices that are at | ||
| //! least 8 bytes long at the given offset, so `copy_from_slice` is infallible by construction and | ||
| //! no fallible conversion is involved. |
There was a problem hiding this comment.
Claude likes to leave comments like these explaining its reasoning. That's fine for you to understand what it's done, but should be deleted before committing.
3 participants
Description
Implementation of NIST SP 800-232 ASCON lightweight cryptography standards for constrained devices (#22), including:
Note: due to the important considerations of embedded systems, this PR must follow, where applicable, utilization of non-heap memory structures (for example: [u8, CONST_SIZE] instead of vec) to respect their compute and RAM limitations.
Type of Change
A full list of detailed changes are outlined in the crate's summary.md file.
Testing
cargo test -p bouncycastle-asconto test ASCON crate and associated KAT test casescargo test --workspacecargo mutants --package bouncycastle-asconfor mutant testsImportant TBDs for testing: