feat(native-rust): message-header inline structures (EDKs, encryption context)#901
Open
lucasmcdonald3 wants to merge 19 commits into
Open
feat(native-rust): message-header inline structures (EDKs, encryption context)#901lucasmcdonald3 wants to merge 19 commits into
lucasmcdonald3 wants to merge 19 commits into
Conversation
added 11 commits
April 30, 2026 14:25
…context) serialization and tests
…, implications on source
…ts from unreviewed
There was a problem hiding this comment.
Pull request overview
Implements Rust serialization/deserialization for the message header’s inline sub-structures (Encrypted Data Keys and Encryption Context AAD encoding) per the message-header spec, plus accompanying spec-driven tests.
Changes:
- Add low-level big-endian read/write primitives used by message serialization.
- Add EDK and Encryption Context (canonical form + AAD subsection) serializers/deserializers and shape/length predicates.
- Add spec-referenced tests for EDK and EC-AAD encoding behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
esdk/src/message/serialize_functions.rs |
Adds shared byte-level read/write helpers with “raw” mirroring support. |
esdk/src/message/serializable_types.rs |
Adds EC/EDK type aliases plus length/shape predicates and helpers. |
esdk/src/message/encryption_context.rs |
Implements canonical EC read + AAD section write logic. |
esdk/src/message/encrypted_data_keys.rs |
Implements EDK section write/read for header serialization. |
esdk/tests/test_encryption_context_aad.rs |
Adds spec-driven tests around EC AAD length/order/serialization. |
esdk/tests/test_encrypted_data_keys.rs |
Adds spec-driven tests around EDK count/entry ordering and length fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lucasmcdonald3
pushed a commit
that referenced
this pull request
May 4, 2026
…efensive EDK count checks Addresses Copilot review on PR #901. - write_aad_section now emits key-value-pairs length = 2 (count) + pair bytes, matching Python/Java/C/JS. Previous output excluded the count field and diverged from every other ESDK on the wire for any non-empty encryption context. - read_canonical_ec now enforces that the declared length equals the bytes consumed by deserialization; previously ignored the length entirely. - write_edks rejects empty EDK lists (spec: count MUST be greater than 0). - read_edks rejects count == 0 (defense in depth; previously surfaced as a downstream SerializationError from misaligned parsing). - is_esdk_encryption_context / is_esdk_encrypted_data_keys: change >= boundary to > so u16::MAX values are valid (matches u16::try_from checks elsewhere). - Tests updated for the new convention (kvp_len 12 -> 14 for single pair). - Shared test_helpers (and one cross-scope test_decrypt_parse_header assertion) updated to match the on-wire convention. Verified: test_encrypted_data_keys (9), test_encryption_context_aad (6), test_decrypt_parse_header (11), and test_vectors_integration (test_python_decrypt, test_java_decrypt, test_rust_encrypt_decrypt) all pass.
added 2 commits
May 4, 2026 11:27
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
added 5 commits
May 5, 2026 16:25
…nnotation from unreviewed
josecorella
reviewed
May 18, 2026
|
|
||
| // Encrypted Data Key Entries | ||
|
|
||
| let mut edks = Vec::with_capacity(usize::from(count)); |
Contributor
There was a problem hiding this comment.
should we have a check in the spec that verifies that the count found on the edks structure matches the actual number of edks found?
| //= spec/data-format/message-header.md#encrypted-data-key | ||
| //= type=implication | ||
| //# The encrypted data key MUST be interpreted as bytes. | ||
| let ciphertext = read_seq_u16(r, raw)?; |
Contributor
There was a problem hiding this comment.
nit: s/ciphertext/edk
but i will say it "technically" is ciphertext
| let mut length = 0; | ||
| for pair in data { | ||
| // key_len(2) + key bytes + val_len(2) + val bytes. | ||
| length += 2 + pair.0.len() + 2 + pair.1.len(); |
Contributor
There was a problem hiding this comment.
perhaps the .len() not intuitive for someone jsut starting to dive into rust but the comment says + key bytes ... + val bytes but the code uses .len() could we update the comment to say that .len extracts the bytes and not jsut a length value?
| //= spec/data-format/message-header.md#key-value-pairs-length | ||
| //# The key value pairs length MUST be interpreted as a UInt16. | ||
| let Ok(bytes_u16) = u16::try_from(bytes) else { | ||
| return ser_err("value too large for u16"); |
Contributor
There was a problem hiding this comment.
Suggested change
| return ser_err("value too large for u16"); | |
| return ser_err("Encryption context key value pair length value is too large for u16"); |
| } | ||
|
|
||
| /// Write the key-value-pairs body: count, then (key, value) pairs. | ||
| pub(crate) fn write_aad( |
Contributor
There was a problem hiding this comment.
nit: same comment as above with the error messages
|
|
||
| // Big-endian fixed-width writers. | ||
| pub(crate) fn write_u8(w: &mut dyn SafeWrite, data: u8) -> Result<(), Error> { | ||
| write_bytes(w, &[data]) |
Contributor
There was a problem hiding this comment.
?
Suggested change
| write_bytes(w, &[data]) | |
| write_bytes(w, &data.to_be_bytes()) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the reusable sub-structures inside the message header — encrypted data keys and encryption context — per message-header.md.
Scope:
esdk/src/message/encrypted_data_keys.rs— EDK serialization/deserialization.esdk/src/message/encryption_context.rs— canonical EC + header AAD sub-section.esdk/src/message/serializable_types.rs— EC/EDK shape predicates and length helpers.esdk/src/message/serialize_functions.rs— low-level big-endian read/write primitives.esdk/tests/test_encrypted_data_keys.rs,esdk/tests/test_encryption_context_aad.rs.The header body itself (V1/V2 header, header auth, message-ID, etc.) is reviewed separately; this PR covers only the inline structures the header embeds.
Note: CI does not run on this PR. This branch is scoped for focused review and intentionally omits test helpers, scaffolding, and other supporting code. Full code — including helpers, test infrastructure, and working tests — lives on the
aws-crypto-rust/unreviewedbranch.If you want more context or to actually run the tests, see
aws-crypto-rust/unreviewed.Related spec: data-format/message-header.md