feat: Add AEAD encryption function with explicit sequence number#40
Conversation
There was a problem hiding this comment.
Adds encrypt_with_seq for SFrame (RFC 9605) — the approach is correct and the nonce construction matches the spec's big-endian XOR. Two items to address before merging:
- FFI hygiene: a
&mutreference is used where the rest of the file uses&raw mutfor pointers crossing the FFI boundary, and two.unwrap()calls diverge from the existing error-propagation pattern inencrypt/decrypt. - DRY opportunity: the nonce XOR loop is now duplicated between
encrypt_with_seqanddecryptwith subtly different error handling — a small shared helper (usingto_be_bytes()) would eliminate the duplication and the inconsistency in one step.
Minor: docstring references a non-existent open method, and a ChaCha20Poly1305 roundtrip test is missing.
There was a problem hiding this comment.
All five items from the prior review are addressed: make_nonce is extracted and shared between encrypt_with_seq and decrypt using to_be_bytes(), FFI pointer style (&raw mut) is consistent, error handling uses map_err instead of unwrap(), docstring references decrypt, and a ChaCha20Poly1305 roundtrip test is added.
The nonce construction correctly implements big-endian XOR per RFC 9605 §4.4.1. One minor inconsistency remains: encrypt_with_seq passes 0 for ulFixedBits while decrypt passes NONCE_LEN - COUNTER_LEN — functionally harmless with CKG_NO_GENERATE, but worth aligning (see inline comment).
There was a problem hiding this comment.
Clean addition. The make_nonce extraction eliminates the duplication between encrypt/decrypt paths and the to_be_bytes() formulation is clearer than the prior bit-shifting loop — nonce construction correctly implements RFC 9605 §4.4.1 big-endian XOR.
All feedback from previous reviews has been addressed: docstring fix, &raw mut consistency, map_err over unwrap, ulFixedBits alignment, naming consistency, and ChaCha20Poly1305 test coverage.
One architectural note: callers can now hold a single Aead in Mode::Encrypt and call both encrypt (auto-counter via CKG_GENERATE_COUNTER_XOR) and encrypt_with_seq (explicit nonce via CKG_NO_GENERATE). Mixing the two on the same context risks nonce reuse if the explicit seq overlaps with the internal counter. The docstring's "never reuse (nonce_base, seq)" caveat covers this, but in practice callers should use one or the other — worth a sentence in module-level docs if this API is meant to be stable.
Fixes #39