Repository navigation
feat(aead)!: seal a fixed-width None as a typed null as long as a Some - #497
Conversation
…e, so ciphertext length no longer shows which values are absent AES-GCM keeps the plaintext's length, and a sealed null was one byte where the value it stood in for was up to 17. Anyone who could read the ciphertexts could list the absent rows of a column without a key. A `None` of an int, a float, a date, a timestamp or a decimal now seals as a typed null: the new `TYPED_NULL` tag (0x15), the header a value of the type starts with, then zero padding to the value's leaf length. It opens only as `None` of an `Option` of the same type, with its header and padding checked strictly so every absence has one encoding; any other target refuses it as the wrong kind. The one-byte null leaf still opens as before, and bool, text and bytes still seal it. Breaking: `Sink` has a new required method, `typed_null`, which every sink must handle (a forwarding sink must forward it, or the padding is lost). `Encrypt` and `Decrypt` gain a `FIXED` constant, derived for fieldless enums and newtypes. A reader built before this release refuses a typed null. Refs #493
✅ No CRAP threshold violations1329 function(s) analyzed · threshold 30 |
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 68 | 0 | 35 | 0 |
✅ Every mutant in the changed lines was caught by a test.
…dening-tolerant reads, uuid - Write the plain null in every frame this crate writes (decrypt_to_frame, to_frame, passthrough subtrees). A frame is never sealed, so padding hides nothing there, and host decoders (Go's vcffi among them) already read the plain null. A frame a host sends may still carry a typed null. - Read a typed null as None for any Option<T> whose T reads a value of its type, so a widened field (u32 to u64, f32 to f64) reads its old Nones as it reads its old values. `Decrypt::reads_null` replaces the exact-match `Decrypt::FIXED`; `FromScalar::FIXED` stays only for a target that refuses zero, such as a fieldless enum. - Give a uuid a typed null: its type fixes it at sixteen bytes (`Width::Bytes16`). `FixedType::plain` now returns an Option, since plain bytes have no fixed width. - Write a typed null from a zero of its type through the codec every value goes through, and read one by comparing it with what would be written, so the two can never drift apart. - Fix docs that still said every Rust None is the plain null leaf. Refs #493
…ull refusal and the derived FIXED, so the mutants gate passes Option and Protected forward reads_null to their inner target, Clear refuses a typed null like any non-passthrough, and the derive declares FIXED by shape and leaves it out of a recursive newtype; tests now pin each. Nothing's typed_null is unreachable (Nothing is uninhabited), so it joins scalar and passthrough in the mutants exclusion, and the default reads_null drops its unused binding so its body is plainly false.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟢 merge as it is
No change is required before merging. The wire format and the reader were checked and are sound. Three small test gaps can wait for a follow-up: the index and N-API sinks each gained a typed_null method that no test calls.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-4-8 | rust | 0 found, 0 posted |
| claude | claude-opus-4-8 | test-gap | 2 found, 2 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
| codex | gpt-5.6-terra | test-gap | 2 found, 1 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 1 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. 1 comment(s) had a problem that stopped the reader acting; it rewrote 1.
Stack: position 1 of 2 (:asterisk: #497, #499). *️⃣ marks this pull request.
Context loaded: the description, 1 linked issue(s) and 21 discussion entries.
Findings not posted:
- Fix in a follow-up:
JsBuilder::typed_nullhas no live Node test. Fixed in #499 (refs/stack/499 adds typed-null vectors to testdata/value-conformance.json with kind "null" and value null. packages/aead-napi/tests/value-conformance.cjs decodes each vector with addon.decode and checks the JS value, so the Node addon's typed-null path is tested live.).
The ore scalar sink and the normalize Terms sink both forward a typed null to the plain-null path, which has no term. Nothing visited that forward: the existing tests only sent a plain null, so a change to the forward could give an absent field an order or equality term unnoticed. A test on each sink now walks a None of a fixed-width type and checks that only the present field has a term.
Closes items 1–4 of #493. Item 5 (index terms for a null) stays open as the follow-up the issue asks for.
Why
AES-GCM keeps the plaintext's length. A sealed null was 1 byte, and the value it stands in for is 2 to 17 bytes. So in a column of sealed dates, anyone who can read the table could list the absent rows without a key. The Go SDK's sealed-null mode for optional fixed-width fields (cipherstash/stack#1298) is blocked on this.
What changes for a caller
None::<T>for a fixed-widthTnow seals as a typed null of the same length asSome(v). Fixed-width means ints (all widths),f32,f64,NaiveDate,DateTime<Utc>,Timestamp,Decimal,Uuid, derived fieldless enums (au32leaf), and newtypes over any of these. Sealing aNone::<i64>goes from[0x00]to[0x15, 0x05, 0, 0, 0, 0, 0, 0, 0].bool, text, other bytes, containers and structs keep the one-byte null leaf. A bool's leaf is already 1 byte. Text and bytes are variable width, out of scope as the issue says.Noneof anOption<T>whoseTreads a value of its type, so a widened field (u32tou64,f32tof64) reads its oldNones as it reads its old values. A different kind (a float null into an int, a date into ani32, anything into text) is refused asDecodeError::Expected(..), and so is a non-optional target. It is never read as a zero.Nonefor anyOption, so data already sealed is unaffected.Wire format
TYPED_NULL = 0x15(new; tags are frozen, so it is never reused), then the header a value of the type starts with, then zeros for all but one byte of the payload:i815 0C0C xxu3215 06 00 00 0006 xx xx xx xx15 12 00 00 0012 d0 d1 d2 d315 13+ 11 zeros13+ 12 bytes15 14+ 15 zeros14+ 16 bytes15 F3 03 01 05+ 7 zerosF3 03 01 05+ 8 bytes15 F3 05 00 0B+ 15 zerosF3 05 00 0B+ 16 bytesA typed null is written from a zero of its type through the same codec as every value. The reader parses the header and then compares the input byte for byte with what would be written, so writer and reader cannot drift. The reader is strict, so every absence has one encoding and a key-holding writer cannot pass another plaintext off as an absence. It refuses nonzero padding, the wrong length, a tag with no typed null (null, bool, text, bytes, another typed null), an envelope where a shorthand tag exists, and a logical type over a physical type it can't ride on. Known-answer tests pin every kind. A quickcheck test checks that whatever is accepted writes back byte for byte.
API
plaintext::Width(ints, floats, andBytes16for a uuid) andplaintext::FixedType(re-exported at the root).FixedType::plainreturns anOption, since plain bytes have no fixed width. AFixedTypeis normalised to how its leaf is written, so two are equal exactly when their typed nulls are the same bytes. For example, every decimal scale shares one typed null, because the scale travels in the payload.Sink::typed_null(self, FixedType)is a new required method. I made it required, not defaulted toscalar(None, Null). With a default, a forwarding sink (Tee,Either,Forward, …) that forgets to override it would drop the padding without any error, and absent values would show again. A required method makes every sink choose. Index sinks (vitaminc-normalizeterms,vitaminc-ore's scalar sink) treat it as a plain null for now (item 5). The napi builder makes it JSnull.Encrypt::FIXED(defaultNone), forwarded through&T,&&T,Option<T>,Protected<T>and derived newtypes, and set tou32for derived fieldless enums.Decrypt::reads_null(FixedType) -> bool(defaultfalse). For aFromScalartarget it means "accepts a zero of that type", or the type is itsFromScalar::FIXED, an escape hatch for a target that refuses zero, such as a derived enum. A test pins that every built-in target reads its own source'sNone.leaf::write_typed_null,leaf::read_item,leaf::read_leaf_item,leaf::Item,leaf::typed_null_len.leaf::read/read_leafkeep their signatures and refuse a typed null, so an existing caller fails closed.Framereads a typed null, so a host that encrypts from a frame can ask for padding.FrameWriterwrites the plain null:decrypt_to_frame,to_frameand passthrough subtrees are never sealed, so padding hides nothing there, and every host decoder already reads0x00.vitaminc-aead-frame's serde format refuses a typed null. That format is for data that is not sealed, and it never writes one.Trade-offs to review
Malformed). New writers produce data old readers can't open, but no data becomes unreadable to new readers.[u8; N]left out. A plain bytes typed null would need a length in its header to be readable in a frame, which is a wire-format decision of its own. Follow-up.hlist::StaticCipher::encrypt_noneunchanged. ItsAbsentpairs withEncrypted, a bytes leaf (variable width), so a typed null doesn't apply there.Not in this PR
0x15into any frame, so Go keeps reading what it reads today. Sending typed nulls from Go means teachingvcffito encode them (for example nil*int32→ typed null) and to accept them on decode, plus rebuilding the committed guest.wasm(the differential fuzz tests compare the two). That belongs with Go SDK: stashgen refuses a pointer field such as*encrypt.Date, so an optional value cannot be encrypted stack#1298.testdata/value-conformance.json: they come with the Go and Node decoders.Review
A high-effort review posted 9 inline findings. All are addressed in e838f1e, with a reply on each thread.
A second, automated review recommended merging as is and left two test gaps for a follow-up: neither index sink's
typed_nullwas visited by a test. Both are closed in 9be42c8 instead, with a reply on each thread.Testing
cargo test --workspace --all-features: all pass exceptvitaminc-kms::test_finalize, which needs localstack on :4566 and fails the same way onmain.packages/aead/tests/typed_null.rs: for every fixed-width kind (primitives, chrono, decimal,Timestamp, uuid, derived enum and newtype), the sealedNoneandSomeare the same length and theNonereads back. Also covers a struct with optional fields (same leaf lengths either way), frames reading typed nulls and writing the plain null, a passthroughNonestaying the plain null, a widened field reading its oldNones, and a forged nonzero padding refused.vitaminc-encrypt'ssink_cipher.rs: the same length check through real AES-256-GCM.vitaminc-ore's scalar sink andvitaminc-normalize'sTermssink: aNone::<u32>/None::<i32>reaches each as a typed null and gets no term, like a plain null, and does not stop the walk.chrono,rust_decimal,uuid,hlist, aead-frameserde), clippy-D warnings, rustdoc-D warnings, wasm32 clippy forvitaminc-encrypt, andcargo +1.88.0 check(MSRV) are clean. The Go guest's tests and the napi addon build pass.