You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(stack-encrypt)!: derive owned record declarations with EncryptTarget and DecryptSource - #1504
Stack Encrypt (the Rust crate stack-encrypt) encrypts a value under a per-value ZeroKMS data key. Until now a user could encrypt one scalar the owned way (keyset.encrypt(value) consumes the value and wipes it afterwards) but a record (a struct whose fields are encrypted individually, some kept in the clear) could only be declared the older structural way, over a borrowed value, with no way to say "carry this field through in the clear".
With this PR a user writes:
#[derive(Zeroize,OpaqueDebug,TimingSafeEq)]// no Debug, Clone or PartialEq on a plaintextstructUser{id:u64,name:String,email:String,created_at:i64}#[derive(EncryptTarget,DecryptSource,Serialize,Deserialize)]#[stash(struct = User, scope = "users")]// the record's name: its first scopestructEncryptedUser{#[stash(passthrough)]id:u64,// kept in the clear, moved into the recordname:Ciphertext,// sealed under ("users", "name")email:Ciphertext,// sealed under ("users", "email")#[stash(passthrough)]created_at:i64,}let encrypted:EncryptedUser = user.encrypt_into(&keyset).await?;// one batched key requestlet restored = Protected::<User>::decrypt_from(encrypted,&cipher).await?;// wiped on droplet rows:Vec<SearchableUser> = users.encrypt_into(&keyset).await?;// a whole column, one batch
The conversion wipes the plaintext fields it seals and moves (never copies) the fields the caller keeps in the clear. A struct record must name every field of its plaintext (an unaccounted field is a compile error, not a field dropped unwiped), and a field opened before a later field is refused is wiped. A struct record takes no context from the caller: encrypt_into_with_context does not compile for it. Under the merged API plan (#1365) a caller's scope or row binding is declared on the record and passed as Scope::new(..) / Bind::new(..), with a binding left out of the search terms; that is #1487, not this PR. Records nest, a #[stash(plaintext = String)] record is a searchable scalar (ciphertext plus equality term) usable as a field, a record can store its own context, and a field whose target cannot take the record's context is a compile error at that field.
The structural derives (EncryptFrom / DecryptInto) stay; the owned derives emit them as a bridge so an owned record is also a field of a structural record or a typed plan field. Their removal is a separate issue.
Changes
Core (packages/stack-encrypt)
EncryptTarget gains owned_encryption() (the description over the plaintext by value, defaulting to guard-with-Protected, run the borrowed encryption(), wipe) and indexes(). EncryptInto and the owned Vec<T> column now run owned_encryption; the column runs every element even after an earlier element fails, so none is dropped unwiped.
New Decryption::zip_wiping: zip that wipes the left side's plaintext when the right side fails or panics; the owned record derive opens its fields through it. The owned Vec column keeps the elements it has not yet handed over in a wiping guard, so a panicking element declaration does not drop the rest unwiped.
New Encryption::split (two descriptions over two different parts of one owned value, one context, one batch) and Encryption::project_owned: how a record divides its struct between its fields without cloning. split also takes a wipe: fn(&mut U) for the part that waits while the first description runs. If that description panics, the waiting part is wiped during the unwind. The derive's wipe zeroizes only the sealed fields, so passthrough field types get no Zeroize bound.
EncryptInto::encrypt_into (context omitted) now guards the plaintext with Protected until the declaration and its context are built, as encrypt_into_with_context already did. A declaration that panics while it is being built wipes the plaintext.
New caller inputs: DeclaredScopes, the input of a struct record (the caller passes nothing; a containing record or a plan passes the field's context, which extends the inner record's), and ExpectedContext<T> as a ContextInput (omit = open under the stored context; supply = also check it). Conversions so a record field is handed its pair: RequiredContext<C>: From<CallerContext>, DeclaredScopes: From<CallerContext>, RequiredContext<NonEmpty<T>> into the core contexts, and the reverse for the bridge.
Owned declarations on the core leaves: StackCipherText (both ways, RequiredContext<AeadContext>), Encrypted<Terms> (seal under RequiredContext<CallerContext>, open under RequiredContext<AeadContext>), EqualityTerm, MatchTerms, OreTerm, OpeTerm, OreBlockTerm as EncryptTarget alone. Ciphertext is Decryptable and a DecryptField, so it is a record field.
A refused context no longer drops a plaintext copy unwiped (found in review). target::ciphertext() checks the context, descriptor length included, before it takes the plaintext, so borrowed mode makes no copy on refusal. StackCipherText and Encrypted<Terms> override owned_encryption() with an owned-mode sealing that makes no copy and wipes the value if the context is refused. json_document() had the same ordering fault in released code and is fixed too.
FieldsBuilder::target::<T, _>(): a typed plan field laid out by an owned-only target (e.g. Ciphertext).
Breaking: the structural Vec<T>: DecryptInto<Vec<P>> / DecryptField<Vec<P>, _> impls open through Decryption::all_wiping and so require P: Zeroize; previously the elements recovered before a late element failure were dropped unwiped. A new drop-witness test fails without the wipe.
Derive (packages/stack-encrypt-derive)
The owned derive writes a generic plaintext's destructure pattern with a turbofish (decrypt::struct_literal_path), so #[stash(struct = Pt<u64>)] compiles.
#[derive(DecryptSource)] also implements DecryptSource<Protected<P>> for each plaintext type a record names, so Protected::<User>::decrypt_from(..) returns the plaintext already wrapped, and a column decrypts into Vec<Protected<User>>. A blanket impl over every source would conflict with Ciphertext's impl, which is generic over any OpaqueDecode; a single value into Protected needs Vitamin C to implement FromScalar for Protected<T>.
Breaking: the container attribute context = ".." is now scope = ".." on all four derives, per the naming review in the API plan. The stored data does not change; the old spelling is a compile error naming the new one. Plan::context(..) and the Go context= tag are renamed with the rest of the naming review (stack-encrypt refactor: finalize crate ownership, naming and coordinated migration #1489).
#[derive(EncryptTarget)] and #[derive(DecryptSource)], taking the existing attributes plus #[stash(passthrough)] (with struct = .. only; refused on the structural derives, on plaintext records and beside identity/default/decrypt, each with its own message). The owned form destructures the plaintext exhaustively, so the plaintext struct cannot implement Drop. struct records get both a borrowed and an owned form; plaintext records, context_type = AeadContext records and context_field records are supported. Both derives also emit the structural EncryptFrom / DecryptInto / DecryptField bridge and Decryptable.
Tests
tests/owned_records.rs: the public mixed-record and searchable-record programs as executable tests (serde round trip, descriptors per field, passthrough moved not cloned — a Clone that panics — sealed fields wiped before any await and when a nested record's oversized context is refused, Vec<User> into Vec<SearchableUser> as one generate_keys call and the matched rows opened in one retrieve_keys call, nested/renamed/pinned fields and their composed descriptors, stored context with expectation, plaintext records over structural and opaque leaves, the bridge into a structural record and a plan).
tests/owned_equivalence.rs: one record sealed by a hand-written EncryptTarget, the derive and the dynamic data plan against one recording provider: same descriptors, same equality-term bytes, every author opens every author's record.
tests/ui: owned_struct_record_takes_no_context.rs pins that a struct record refuses a caller context, to encrypt and to decrypt. Field-specific compile-fail cases (NoCallerContext / RequiredContext<u64> field in a struct record, a term in an AeadContext-only record, omitted required context, non-Zeroize plaintext, every passthrough misuse) and a must-compile file of the owned forms; eight existing snapshots re-recorded for rustc's longer implementor lists.
tests/collections.rs: the structural Vec path wipes its recovered prefix after a late element failure, as a column and as a column field of a structural record; a panicking element declaration wipes the elements after it.
tests/owned_records.rs also pins that a record field opened before a later refusal is wiped; tests/ui/owned_struct_unmapped_field.rs pins the unaccounted-field refusal.
tests/owned_records.rs panic witnesses: a_panicking_field_declaration_wipes_the_sealed_fields_after_it (a panic in the outer split and in a nested split) and a_declaration_that_panics_while_constructed_wipes_the_plaintext. Both are enrolled in the owned-records lifecycle suite.
Mutant coverage: each_owned_term_target_names_its_index. Two equivalent mutants are excluded in .cargo/mutants.toml, each with its reason: ExpectedContext::omitted -> Default::default() and DeclaredScopes::omitted -> Default::default() (both bodies are Self::default()).
tests/owned_records.rs: eight drop witnesses for a refused context, built on the reviewer's probe, one per leaking path (long scope literal, nested record under a long scope, plaintext record under a long caller context and a long stored context, a Vec<StackCipherText> column, a scalar StackCipherText, a Vec<StackCipherText>, and Encrypted<EqualityTerm>). All eight failed before the fix. Enrolled in lifecycle-buffer-cleanup-v1 under owned-records.
tests/ui/pass/owned_generic_plaintext.rs: a generic plaintext through project_owned and through split.
lifecycle-profiles.json: a new owned-records suite enrolls the record drop witnesses; the new collection witnesses join collections.
Docs
Crate docs "Records" section (doctest), target module docs with the trait/ownership table, derive docs "Owned records" section and the passthrough attribute row, ADR-0012 (signature/import/ownership map and the bridge decision; numbered 0012 because feat(stack-encrypt)!: add OpaqueBlob whole-value encryption #1503 takes 0011), conformance/stack-encrypt/coverage.md rows, CHANGELOG [Unreleased] (Breaking / Added / Deprecated).
Verification
All run from the worktree on the pushed head 327b69398, no credentials:
cargo fmt --all --check — clean.
mise x -- cargo nextest run -p stack-encrypt -p stack-encrypt-derive --all-features — 1192 passed, 1 skipped, the ui trybuild suite included.
RUSTDOCFLAGS="-D warnings" cargo doc for both crates — clean.
cargo mutants --list on target/input.rs — the DeclaredScopes::omitted mutant is no longer listed. A full mutants sweep was not run; the CI gate runs --in-diff.
Every test name enrolled in the owned-records and collections suites (22) appears in cargo nextest list.
Earlier on this branch, before the review fixes: cargo check -p eql-bindings --features stack-encrypt and the Go guest compiled unchanged; node scripts/stack-lifecycle-conformance.mjs rust passed. Neither was re-run after the review fixes, since this worktree has no node_modules; CI runs the lifecycle script.
Not run: a full cargo-mutants sweep, live ZeroKMS examples.
Start with packages/stack-encrypt/docs/adr/0012-owned-record-declarations.md, then src/target/conversion.rs (owned_encryption), src/target/input.rs (the inputs and conversions) and packages/stack-encrypt-derive/src/owned_encrypt.rs (split chain). tests/owned_records.rs is the executable version of the public examples.
Deliberately deferred, with reasons:
Configured nested targets (a JSON index with options as a record field, the plan's target(options = factory) attribute): the only configurable target's plaintext is serde_json::Value, which is not Zeroize, so it cannot be an owned source yet. Nested targets with and without caller context are supported; a typed-options attribute waits on a zeroizing document plaintext.
Option<T> and tuple targets stay structural-only in this slice (nullable scalars are deferred by the scalar slice too).
EQL adapter owned impls: eql-bindings compiles unchanged and keeps its structural declarations. Adding EncryptTarget impls there would build against the crates.io stack-encrypt, which lacks this surface until the next release (see AGENTS.md on the stack-encrypt feature's registry gate).
A #[stash(passthrough)] field type must be Clone: the record's borrowed form (what a plan or a structural record runs) copies it; the owned form never does, and a test with a panicking Clone holds it to that.
The dynamic lowering and the Go guest are untouched by design: the data grammar already expresses passthrough and sealed fields, and the equivalence test is what ties the three authors to one engine.
Summary by CodeRabbit
New Features
Added owned record encryption and decryption derives, supporting nested and renamed fields, stored or caller-provided contexts, and searchable indexes.
Added passthrough fields that move into records without encryption, plus owned targets for plan fields and batched collections.
Bug Fixes
Recovered plaintext is wiped when later decryption fails; owned plaintext is also wiped in covered failure and panic cases.
Structural vector decryption now requires plaintext elements to support zeroization.
Documentation
Added guides and examples for owned records, context handling, passthrough fields, and migration between owned and structural APIs.
Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.
This PR includes no changesets
When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types
Auto reviews are disabled on base/target branches other than the default branch.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
Changes
Owned record declarations
Layer / File(s)
Summary
Owned target and context runtime packages/stack-encrypt/src/target/*, packages/stack-encrypt/src/plan/build.rs, packages/stack-encrypt/src/scalar.rs
Adds owned encryption and decryption operations, context conversions, collection handling, index support, and typed plan fields.
Owned record derive generation packages/stack-encrypt-derive/src/*
Adds EncryptTarget and DecryptSource derives with passthrough fields, field accounting, context checks, structural bridges, and wiping-aware reconstruction.
Wiping and failure cleanup packages/stack-encrypt/tests/collections.rs, packages/stack-encrypt/tests/owned_records.rs, conformance/stack-encrypt/*
Adds cleanup behavior and evidence for late failures, refused contexts, declaration panics, collection conversion, and passthrough moves.
Record integration and conformance evidence packages/stack-encrypt/tests/owned_equivalence.rs, packages/stack-encrypt/tests/ui/*, packages/stack-encrypt/docs/*, packages/stack-encrypt/CHANGELOG.md, conformance/stack-encrypt/*
Adds integration, UI, documentation, lifecycle enrollment, and equivalence coverage for owned records and their structural bridges.
sequenceDiagram
participant Caller
participant EncryptTarget
participant FieldTarget
participant Pending
participant DecryptSource
Caller->>EncryptTarget: encrypt owned record with context
EncryptTarget->>FieldTarget: split or project fields
FieldTarget->>Pending: create field operations
Pending-->>Caller: return encrypted record
Caller->>DecryptSource: decrypt stored record
DecryptSource->>FieldTarget: open field groups
FieldTarget->>Pending: combine results with wiping
Pending-->>Caller: return plaintext record
Loading
Possibly related issues
cipherstash/stack#1475 — Adds caller and stored context handling for owned record fields.
cipherstash/stack#1501 — Provides owned collection conversion and element cleanup used by record columns.
cipherstash/stack#1493 — Provides lifecycle enrollment extended by owned-record evidence.
cipherstash/stack#1404 — Provides the owned target and source contracts extended by this change.
Suggested reviewers:freshtonic, calvinbrewer
Merge Risk: Minimal · previous review b3f71
The conformance enrollment is supported by the referenced tests and presents no actionable merge-blocking risk.
Pre-merge checks: 3 · 2
Failed checks: 2 warnings
Check name
Status
Explanation
Resolution
Linked Issues check
Issue #1483 is open and directly linked. The PR implements the owned derives, passthrough mapping, context diagnostics, cleanup, batching, executable record tests, and typed/dynamic/generated equivale…
Implement configured nested targets and migrate the remaining plan, dynamic/guest, and EQL consumers against the owned declaration surface, or split those requirements into follow-up issues and revise #1483 and the PR description so that th…
Docstring Coverage
Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 3 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the primary change: owned record declarations for stack-encrypt, built on the EncryptTarget and DecryptSource derives. The raw summary confirms this is the main change. It adds owned d…
Out of Scope Changes check
The changed core APIs, derives, context inputs, cleanup behavior, collection bounds, tests, lifecycle evidence, documentation, ADR, changelog, and mutation configuration support the owned-record objec…
Full details: Title check
Explanation
The title names the primary change: owned record declarations for stack-encrypt, built on the EncryptTarget and DecryptSource derives. The raw summary confirms this is the main change. It adds owned derive macros, owned encryption and decryption, passthrough fields, and context inputs, and the title covers that focus. The title is 91 characters, which is a little long but still within a readable single sentence. It uses the conventional commit prefix with the breaking-change marker, which matches the summary's breaking note on structural vector decryption requiring Zeroize.
Full details: Out of Scope Changes check
Explanation
The changed core APIs, derives, context inputs, cleanup behavior, collection bounds, tests, lifecycle evidence, documentation, ADR, changelog, and mutation configuration support the owned-record objectives in #1483 or the stated migration bridge. The structural Vec cleanup change supports the collection cleanup requirement. The PR does not show an unrelated change. Cargo.lock was intentionally excluded and was not inspected.
Full details: Linked Issues check
Explanation
Issue #1483 is open and directly linked. The PR implements the owned derives, passthrough mapping, context diagnostics, cleanup, batching, executable record tests, and typed/dynamic/generated equivalence. The PR does not meet all coding requirements in #1483. Configured nested targets remain deferred. Plan lowering gains FieldsBuilder::target, but dynamic/guest consumers and EQL adapters remain on structural declarations. These gaps match the PR's stated deferrals.
Resolution
Implement configured nested targets and migrate the remaining plan, dynamic/guest, and EQL consumers against the owned declaration surface, or split those requirements into follow-up issues and revise #1483 and the PR description so that this PR does not claim to close #1483. Reassess after the issue scope and implementation agree.
Full details: Docstring Coverage
Explanation
Docstring coverage is 54.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 33 functions across 29 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Rebase first. The branch is rebased onto feat/stack-encrypt-owned-collections at 4b42186 (now 55ff30f). This is a rebase, not a content change. The one conflict was tests/ui/owned_collection_elements.stderr. I kept the base's UFCS form (E0277, no $RUST/ snippet) and re-blessed it. The only difference is that Zeroize now prints unqualified. grep -l 'RUST/' tests/ui/*.stderr prints nothing, and none of this PR's .stderr files depend on E0599.
Checked by running code: cargo check -p eql-bindings --features stack-encrypt and the Go guest (default and --features eql) both compile.
Standards
packages/stack-encrypt/docs/adr/0011-owned-record-declarations.md: number collision.feat(stack-encrypt)!: add OpaqueBlob whole-value encryption #1503 (feat/stack-encrypt-opaque-blob) also adds 0011-a-whole-value-is-one-storage-blob-sealed-as-one-leaf.md. This one moves to 0012. Nothing in the tree links to it, but the PR body does. CONFIRMED.
src/target/operations.rs (DecryptField<Vec<P>, Ctx> for Vec<T>): if this impl's all_wiping goes back to all, no test fails. I ran the full stack-encrypt nextest suite. The new collections test reaches only the DecryptInto impl. That is a surviving mutant on a Breaking-labelled line, and mutants.yml --in-diff would flag it. CONFIRMED.
src/plan/build.rsFieldsBuilder::target repeats the FieldOpener construction from encrypt_into almost line for line (duplicated code). Judgement call; left.
ADR §2, the CHANGELOG Added bullet and the split docs all say the conversion "wipes what it seals and transfers what the caller chose to keep". As written that is not true: see the first Spec bullet. CONFIRMED.
Spec
packages/stack-encrypt-derive/src/owned_encrypt.rs (divide, let #source { .., .. } = __source): plaintext fields the record does not name are dropped unwiped. The owned override replaces the default guard over the whole struct. Any field of User that is neither sealed nor passthrough falls through .. and is freed as it is. I reproduced it with a Wide { sealed, unmapped } drop witness: unmapped is never zeroized. The default path and the borrowed path both wipe it. DecryptSource already has to name every field to rebuild the struct, so an exhaustive destructure costs a both-derive record nothing. CONFIRMED.
src/target/collections.rs:63-70 (owned Vec column): a panicking element drops the rest of the column unwiped.source.into_iter().map(..).collect() unwinds through vec::IntoIter's drop. Before this PR the whole vector sat in one Protected guard, so this is a regression on the panic path that the base branch otherwise hardens. I reproduced it with a target that panics on element 0: elements 1 and 2 are never wiped. CONFIRMED.
owned_decrypt.rsstruct_body → Decryption::zip → Pending::zip (pending.rs:300): a field opened before a later field fails is dropped unwiped.Ok((first(responses)?, second(responses)?)) discards first. The column has collect_wiping, but the record has nothing equivalent, so the acceptance point "preserve owned plaintext cleanup" holds for columns and not for records. The structural derive has the same gap. CONFIRMED (by reading; a fix needs a test).
Encryption::split (operations.rs): while the first field's description builds, theirs is held unguarded, so a panic there drops the later fields unwiped. Guarding it would put U: Zeroize on passthrough field types. PLAUSIBLE.
A source struct that implements Drop (ZeroizeOnDrop) cannot be destructured (E0509), so it cannot be an owned struct record source. The derive docs don't say so. CONFIRMED (language rule).
Acceptance bullets:
Met: the signature/ownership map (ADR table); explicit passthrough, source-field mapping and decrypt reconstruction; context capabilities with field-spanned diagnostics (owned_record_field_context.stderr points at fixed: Fixed and bound: Bound); executable examples and three-author equivalence (owned_equivalence.rs, which also opens across authors).
Partial: cleanup (the three CONFIRMED items above); nested targets (configured targets are deferred); EQL adapters (deferred); decrypt-side field diagnostics (only the encrypt side is pinned in tests/ui).
Deferrals:
Acceptable: configured nested targets (serde_json::Value is not Zeroize); EQL owned impls (the registry gate in AGENTS.md applies); Option/tuple staying structural-only; no #[deprecated]; passthrough needing Clone (pinned by the panicking Clone).
Not acceptable: skipping lifecycle-profile enrollment. The two parent slices enrolled their drop witnesses in lifecycle-profiles.json. This slice's record witnesses (wiped before await, wiped on a refused context) are the same evidence for records and should be enrolled the same way.
Standards: 4 confirmed, 1 judgement call. Worst: the CHANGELOG Deprecated heading reclassifies the #1482 Added bullets.
Spec: 4 confirmed, 1 plausible. Worst: an owned struct record drops the plaintext fields it does not name, unwiped.
15ba61d: renamed to 0012-owned-record-declarations.md. Nothing in the tree linked to it by number. The PR body now links the new name.
CHANGELOG Deprecated heading in the middle of Added
15ba61d: moved after the full Added list. The owned-collections bullet no longer reads as a deprecation.
DecryptField<Vec<P>, _>'s all_wiping untested
f56c542: added a_late_element_failure_in_a_column_field_wipes_the_recovered_prefix, a structural record with a column field. It fails when all_wiping goes back to all.
Owned struct record drops plaintext fields it does not name
a80c98b: the destructure is now exhaustive. An unaccounted field is E0027 at the struct = .. path, pinned in tests/ui/owned_struct_unmapped_field.rs.
Owned column: a panicking declaration drops the rest unwiped
f56c542: the elements not yet handed over sit in a guard that wipes them. An element is taken out only once its description and context exist. a_panicking_element_declaration_wipes_the_elements_after_it fails without the guard.
Owned record decrypt drops already-opened fields on a later refusal
1830fbd: added Decryption::zip_wiping / Pending::zip_wiping (a guard holds the left side). The owned derive opens its fields through it. a_field_opened_before_a_later_field_is_refused_is_wiped fails with plain zip. The structural DecryptInto derive is left as it is: its field plaintexts carry no Zeroize bound, and adding one would be a separate breaking change.
Docs claimed "wipes what it seals and transfers what it keeps"
15ba61d: the ADR, the derive's "Owned records" docs and the CHANGELOG now say what is true after the fixes above.
A source with Drop / ZeroizeOnDrop cannot be an owned struct source
15ba61d: documented in the derive docs, the ADR and the CHANGELOG. Not changed: lifting it needs a Default bound on every moved field (mem::take) or unsafe.
Lifecycle-profile enrollment skipped
3c23ed0: added a new owned-records suite with three witnesses, plus two collection witnesses. node scripts/stack-lifecycle-conformance.mjs rust passes.
Encryption::split holds later fields unguarded during an earlier field's build (PLAUSIBLE)
Declined. It is reachable only through a panicking field declaration. Guarding the later fields needs U: Zeroize on split, which would extend to passthrough field types. That is a design decision for you, not a mechanical fix.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Review attestation
Reviewed c4784c9 · posted from Claude Code · claude-sonnet-5-5
Sources: code-review (medium) · claude-sonnet-5-5
Skills: code-review, review-cross-check
No findings.
Six candidate findings were verified against c4784c9 and cross-checked against existing feedback: two are already raised in open threads (unguarded plaintext in encrypt_into at conversion.rs:80; unguarded remainder in Encryption::split), and the others were dismissed as documented design or left undecided, so none is posted.
Codex P2: Encryption::split drops the waiting fields unwiped if an earlier field's declaration panics
Fixed in 9d3729a. I had declined this earlier. split now takes a wipe: fn(&mut U) and keeps the waiting part in a guard until the first description returns. The derive writes each wipe: it zeroizes the sealed fields still waiting and leaves the passthrough ones alone. Sealed sources are already Zeroize, as every EncryptTarget source is, so passthrough field types get no new bound. Witness: a_panicking_field_declaration_wipes_the_sealed_fields_after_it, which covers both the outer split and a nested split. The ADR, CHANGELOG and derive docs are updated.
Codex P2: encrypt_into leaves self unguarded while the declaration is built
Fixed in b40cf93. The plaintext stays in Protected until the declaration and the omitted context are built. Witness: a_declaration_that_panics_while_constructed_wipes_the_plaintext, which checks both entry points.
Mutant: MatchTermsEncryptTarget::indexes → vec![]
bfa3d58: each_owned_term_target_names_its_index. It covers every owned term target.
bfa3d58: equivalent, because the body is Self::default(). It is excluded in .cargo/mutants.toml next to the matching From<()> entry. The omitted behaviour is pinned by a_stored_context_opens_as_stored_and_checks_an_expectation.
I confirmed the two killed mutants by applying each one by hand: the new test failed under each. Both panic witnesses failed before their fixes and are enrolled in the owned-records lifecycle suite.
CodeRabbit finished its review with no actionable comments.
Rebased onto the rebased #1501 (57a9b210c). This was a force push. One conflict came up: the first commit extends two coverage.md rows in which #1491 had renamed v1/declarations.json to v1/record-declarations-v1.cases.json. I kept this PR's text with the new path. Compared line by line, the PR diff otherwise matches the one before the rebase: 52 files, +4097/−84. Review feedback is not addressed in this push.
… the changelog sections
ADR-0011 is taken by the opaque-blob ADR on a sibling branch, so the
owned-record ADR becomes 0012. Nothing in the tree linked to it by number.
The `[Unreleased]` `Deprecated` heading sat in the middle of `Added`,
which filed the owned-collections bullet under deprecations. It now
follows the full `Added` list. The owned-record bullet, the derive's
"Owned records" docs and the ADR now cover what the fixes on this
branch made true: a `struct` record names every plaintext field, its
plaintext cannot implement `Drop`, and a field opened before a later
refusal is wiped. A new bullet adds `Decryption::zip_wiping`. The
owned-collections bullet now describes the per-element owned form.
…fecycle profile
The owned-column slice enrolled its drop witnesses. The owned-record
witnesses are the same evidence for records and now go in the same place.
A new `owned-records` suite (`tests/owned_records.rs`) adds three witnesses:
- under buffer cleanup, the record wipes its sealed fields before any
await and on a refused context, and moves its passthrough fields;
- under partial output, a field opened before a later field is refused
is wiped.
Two new collection witnesses join the existing `collections` suite. One
checks the elements after a panicking declaration. The other checks a
structural record's column field.
… its declaration
The context-omitting EncryptInto::encrypt_into handed self to the declaration only after T::owned_encryption() had been constructed, with nothing guarding it meanwhile, so a declaration that panicked while constructed (a record building its fields' declarations) dropped the plaintext unwiped. It now holds the plaintext in Protected until the declaration and the omitted context exist, as encrypt_into_with_context already did for its context conversion; that variant now also constructs the declaration under the guard rather than as an argument.
… over
Encryption::split ran the first part's description while the other part sat in a local, so a panic in that description (a field declaration's selector, say) dropped the later sealed fields of an owned struct record unwiped. split now takes a wipe for the waiting part and holds it in a guard that calls it during the unwind; the part leaves the guard only once the first description has returned.
The derive writes each wipe to zeroize the sealed fields still waiting and leave the passthrough ones. A sealed field's plaintext is already Zeroize, as every EncryptTarget source is, so no bound is added, and passthrough field types take none.
BREAKING CHANGE: Encryption::split takes a fourth argument, wipe: fn(&mut U).
…ion inspection
Covers two mutants that survived the in-diff gate: MatchTerms' EncryptTarget::indexes replaced with vec![] (each_owned_term_target_names_its_index, which also pins the other owned terms) and ContextExtension::into_inner replaced with None (a_context_extension_is_inspected_as_supplied_or_omitted). The third survivor, ExpectedContext's omitted replaced with Default::default(), is equivalent: the body is Self::default(). It is excluded in .cargo/mutants.toml beside the matching From<()> entry, with the test that pins the omitted behaviour named.
…se-resistance rules
The example's plaintext type derived Debug, Clone and PartialEq, so it
printed decrypted rows, cloned a plaintext to encrypt it twice, and
compared plaintexts with a non-constant-time ==. It also used the
structural derives, which this branch replaces for new records.
The example now uses the owned derives. User derives Zeroize and nothing
else. Each decrypted value goes straight into a Protected, which wipes it
on drop, and the example prints counts and match results rather than
plaintext. The row-binding section builds each plaintext from a function
instead of cloning one.
Applies the rules in cipherstash/vitaminc#494 (misuse-resistance rules
4 and 9).
…and state what the caller must wipe
Rewrites the prose this branch adds: the crate's "Records" section, the
target module docs, the derive crate's "Owned records" section, ADR-0012,
the CHANGELOG entries, the conformance rows, and the doc and test
comments. The text follows ISO 24495-1: one idea per sentence, terms
explained where they first appear, and lists in place of long chains.
It also applies the misuse-resistance rules in cipherstash/vitaminc#494:
- The doc examples no longer show FakeDataKeySource in visible code, and
their plaintext types derive only Zeroize.
- Decrypted values go into a Protected in the examples.
- DecryptFrom, the derive docs and ADR-0012 say that a decrypted
plaintext is not wiped on drop. The plaintext of a struct record
cannot derive ZeroizeOnDrop, so the caller must wrap or wipe it.
- The passthrough docs say the field can be read, changed or removed in
storage, and must not decide authorisation or tenancy.
- ExpectedContext and the derive docs say that a context_field record
opened with no expected context is not checked.
- Encryption::project_owned says it drops the other fields unwiped.
- ADR-0012 gains "Options considered" and "What is left to the caller".
The trybuild snapshots change only in line numbers.
…ller context
Apply the naming review recorded in the merged API plan to the record
derives, so this change lands no name the review replaces.
- The container attribute `context = ".."` is now `scope = "..."`, on the
structural and owned derives. The value is the record's name, the first
scope of each field's context. The stored data does not change. The old
spelling is a compile error that names the new one.
- `ContextExtension` is removed. A `struct = ..` record takes
`DeclaredScopes`, which accepts no context from the caller, so
`encrypt_into_with_context` does not compile for it. A containing record
or a plan still passes a field's context through it. The plan replaces
the caller extension with declared `Scope` and `Bind` parts, with a
binding left out of the search terms. That work is tracked separately.
- The encrypted_record example drops its row-binding section, and the
docs, ADR-0012 and the CHANGELOG say what a record accepts now.
…rewrite the record docs in plain language
Address the review of the owned-record docs.
- The decrypt derive also implements DecryptSource<Protected<P>> for each
plaintext type a record names. Protected::<User>::decrypt_from(..)
wraps the plaintext as soon as it is recovered, and a Vec of records
decrypts into Vec<Protected<User>>. A blanket impl over every source is
not possible: Ciphertext's impl is generic over any OpaqueDecode, which
Vitamin C could extend to Protected. A drop witness, enrolled in the
lifecycle buffer-cleanup profile, checks the wipe on drop.
- The plaintext type may derive OpaqueDebug and TimingSafeEq from
vitaminc::protected in place of Debug and PartialEq. The
encrypted_record example derives both and shows what they print.
- The derive guide explains what "owned" means, why only one pair of
derives is allowed, how to check where a context_field record came
from, and shows passthrough fields as the normal case for id and
created_at columns. The remaining sections are rewritten in plain
language, and the AeadContext example uses the owned derive.
- The derive crate docs no longer show encrypt_as or decrypt_as, which
the API plan removes. Both examples use the owned derives.
- ADR-0012 is rewritten in plain language and checked against the API
plan: the structural traits are removed by the plan, decryption into
Protected is a decision, and ExpectedContext goes when Planned lands.
OpaqueDecode now covers every Decrypt + Zeroize type, as the bound on
StackCipherText and the indexed types already does, so
keyset.decrypt::<Protected<String>>(ct) works. A target that is not a
scalar now compiles and is refused when it opens; a compile error for it
needs a marker trait in Vitamin C.
…t is refused
The borrowed form of `target::ciphertext()` cloned the plaintext before
the context's descriptor was checked. When the context was too long for
a ZeroKMS descriptor, `encrypt_opaque` refused it and dropped the clone
without wiping it. The owned `StackCipherText` and `Encrypted<Terms>`
targets used the default `owned_encryption`, which runs that borrowed
form, so all seven paths in the review leaked a copy: a struct record
with a long scope, a nested record under one, a plaintext record under a
long caller or stored context, a column, a scalar `StackCipherText`, and
a `Vec<StackCipherText>`.
- `ciphertext()` now checks the context (`AeadContext::sealable`) before
it takes the plaintext, so a refused context makes no copy.
- `StackCipherText` and `Encrypted<Terms>` override `owned_encryption`.
They run an Owned-mode ciphertext that seals the value with no copy,
and wipes it through `Protected` when the context is refused. The
public `ciphertext()` keeps no `Zeroize` bound, so its Owned mode
still drops a refused value as it is; that is documented.
- The term operations derive no descriptor and cannot refuse a context.
Each term scheme consumes its copy, so they are unchanged.
- `json_document()` guarded its copy only after the descriptor check;
it now guards it first.
Eight drop-witness tests in tests/owned_records.rs cover the seven
paths and an `Encrypted<EqualityTerm>`. All eight failed before this
change. They are enrolled in lifecycle-buffer-cleanup-v1.
…utant
DeclaredScopes derives Default, and omitted() returns Self::default(), so
replacing the body with Default::default() changes nothing. The mutant
survived and turned the Mutants gate red. The exclusion matches only that
replacement, as the ExpectedContext one beside it does.
`cargo mutants --list` on input.rs no longer lists it.
…ofish in the owned pattern
The owned form of a `struct = ..` record destructures the plaintext with
the type path as written. For a generic plaintext, `struct = Pt<u64>`,
that emitted `let Pt<u64> { .. } = ..`, which does not parse: the user
saw "generic args in patterns require the turbofish syntax" and
"proc-macro derive produced unparsable tokens". The decrypt side already
rewrites the path with `decrypt::struct_literal_path`; the encrypt side
now does the same.
tests/ui/pass/owned_generic_plaintext.rs covers a generic plaintext with
one field (the `project_owned` path) and with two (the `split` path). It
failed to compile before this change.
…entence, and enrol the structural-path witness
- `Encryption::project_owned` drops the other fields of a struct without
wiping them. The derive needs it public, so it is now `#[doc(hidden)]`,
and its doc comment says it is for generated code and when the derive
calls it.
- attributes.md said an owned record takes no caller context. Only a
`struct = ..` record does not; a `plaintext = ..` record and a
`context_field` record take one.
- `a_late_element_failure_wipes_the_recovered_prefix_on_the_structural_path_too`
is the witness for the structural `Vec<T>: DecryptInto` change. It is
now enrolled in lifecycle-partial-output-v1 beside its column-field
sibling, and the contract names the path it covers.
@freshtonic the three "must fix" items and the three nits from your re-review are addressed, and the branch is pushed at 327b693 (four commits on top of 31162e8, no rebase).
The refused-context leak: fixed in 3e90e2f. ciphertext() checks the context before it takes the plaintext, so borrowed mode makes no copy on refusal, and StackCipherText and Encrypted<Terms> override owned_encryption() with an owned-mode sealing that wipes a refused value. Eight drop witnesses built on your probe cover every path in your table; all eight failed before the fix. Details in the inline reply.
Mutants gate: 0829076 adds the exclusion with the reason. The PR body claim about DeclaredScopes coverage is corrected.
Nits: the generic plaintext pattern (1aa4d6c, with a compile-pass case), project_owned hidden (327b693), and the structural-path witness enrolled (327b693).
Could you take another look when you have a moment?
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
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.
Summary
Stack Encrypt (the Rust crate
stack-encrypt) encrypts a value under a per-value ZeroKMS data key. Until now a user could encrypt one scalar the owned way (keyset.encrypt(value)consumes the value and wipes it afterwards) but a record (a struct whose fields are encrypted individually, some kept in the clear) could only be declared the older structural way, over a borrowed value, with no way to say "carry this field through in the clear".With this PR a user writes:
The conversion wipes the plaintext fields it seals and moves (never copies) the fields the caller keeps in the clear. A
structrecord must name every field of its plaintext (an unaccounted field is a compile error, not a field dropped unwiped), and a field opened before a later field is refused is wiped. Astructrecord takes no context from the caller:encrypt_into_with_contextdoes not compile for it. Under the merged API plan (#1365) a caller's scope or row binding is declared on the record and passed asScope::new(..)/Bind::new(..), with a binding left out of the search terms; that is #1487, not this PR. Records nest, a#[stash(plaintext = String)]record is a searchable scalar (ciphertext plus equality term) usable as a field, a record can store its own context, and a field whose target cannot take the record's context is a compile error at that field.The structural derives (
EncryptFrom/DecryptInto) stay; the owned derives emit them as a bridge so an owned record is also a field of a structural record or a typed plan field. Their removal is a separate issue.Changes
Core (
packages/stack-encrypt)EncryptTargetgainsowned_encryption()(the description over the plaintext by value, defaulting to guard-with-Protected, run the borrowedencryption(), wipe) andindexes().EncryptIntoand the ownedVec<T>column now runowned_encryption; the column runs every element even after an earlier element fails, so none is dropped unwiped.Decryption::zip_wiping:zipthat wipes the left side's plaintext when the right side fails or panics; the owned record derive opens its fields through it. The ownedVeccolumn keeps the elements it has not yet handed over in a wiping guard, so a panicking element declaration does not drop the rest unwiped.Encryption::split(two descriptions over two different parts of one owned value, one context, one batch) andEncryption::project_owned: how a record divides its struct between its fields without cloning.splitalso takes awipe: fn(&mut U)for the part that waits while the first description runs. If that description panics, the waiting part is wiped during the unwind. The derive'swipezeroizes only the sealed fields, so passthrough field types get noZeroizebound.EncryptInto::encrypt_into(context omitted) now guards the plaintext withProtecteduntil the declaration and its context are built, asencrypt_into_with_contextalready did. A declaration that panics while it is being built wipes the plaintext.DeclaredScopes, the input of astructrecord (the caller passes nothing; a containing record or a plan passes the field's context, which extends the inner record's), andExpectedContext<T>as aContextInput(omit = open under the stored context; supply = also check it). Conversions so a record field is handed its pair:RequiredContext<C>: From<CallerContext>,DeclaredScopes: From<CallerContext>,RequiredContext<NonEmpty<T>>into the core contexts, and the reverse for the bridge.StackCipherText(both ways,RequiredContext<AeadContext>),Encrypted<Terms>(seal underRequiredContext<CallerContext>, open underRequiredContext<AeadContext>),EqualityTerm,MatchTerms,OreTerm,OpeTerm,OreBlockTermasEncryptTargetalone.CiphertextisDecryptableand aDecryptField, so it is a record field.target::ciphertext()checks the context, descriptor length included, before it takes the plaintext, so borrowed mode makes no copy on refusal.StackCipherTextandEncrypted<Terms>overrideowned_encryption()with an owned-mode sealing that makes no copy and wipes the value if the context is refused.json_document()had the same ordering fault in released code and is fixed too.FieldsBuilder::target::<T, _>(): a typed plan field laid out by an owned-only target (e.g.Ciphertext).Vec<T>: DecryptInto<Vec<P>>/DecryptField<Vec<P>, _>impls open throughDecryption::all_wipingand so requireP: Zeroize; previously the elements recovered before a late element failure were dropped unwiped. A new drop-witness test fails without the wipe.Derive (
packages/stack-encrypt-derive)decrypt::struct_literal_path), so#[stash(struct = Pt<u64>)]compiles.#[derive(DecryptSource)]also implementsDecryptSource<Protected<P>>for each plaintext type a record names, soProtected::<User>::decrypt_from(..)returns the plaintext already wrapped, and a column decrypts intoVec<Protected<User>>. A blanket impl over every source would conflict withCiphertext's impl, which is generic over anyOpaqueDecode; a single value intoProtectedneeds Vitamin C to implementFromScalarforProtected<T>.context = ".."is nowscope = ".."on all four derives, per the naming review in the API plan. The stored data does not change; the old spelling is a compile error naming the new one.Plan::context(..)and the Gocontext=tag are renamed with the rest of the naming review (stack-encrypt refactor: finalize crate ownership, naming and coordinated migration #1489).#[derive(EncryptTarget)]and#[derive(DecryptSource)], taking the existing attributes plus#[stash(passthrough)](withstruct = ..only; refused on the structural derives, onplaintextrecords and besideidentity/default/decrypt, each with its own message). The owned form destructures the plaintext exhaustively, so the plaintext struct cannot implementDrop.structrecords get both a borrowed and an owned form;plaintextrecords,context_type = AeadContextrecords andcontext_fieldrecords are supported. Both derives also emit the structuralEncryptFrom/DecryptInto/DecryptFieldbridge andDecryptable.Tests
tests/owned_records.rs: the public mixed-record and searchable-record programs as executable tests (serde round trip, descriptors per field, passthrough moved not cloned — aClonethat panics — sealed fields wiped before any await and when a nested record's oversized context is refused,Vec<User>intoVec<SearchableUser>as onegenerate_keyscall and the matched rows opened in oneretrieve_keyscall, nested/renamed/pinned fields and their composed descriptors, stored context with expectation, plaintext records over structural and opaque leaves, the bridge into a structural record and a plan).tests/owned_equivalence.rs: one record sealed by a hand-writtenEncryptTarget, the derive and the dynamic data plan against one recording provider: same descriptors, same equality-term bytes, every author opens every author's record.tests/ui:owned_struct_record_takes_no_context.rspins that astructrecord refuses a caller context, to encrypt and to decrypt. Field-specific compile-fail cases (NoCallerContext/RequiredContext<u64>field in astructrecord, a term in anAeadContext-only record, omitted required context, non-Zeroizeplaintext, everypassthroughmisuse) and a must-compile file of the owned forms; eight existing snapshots re-recorded for rustc's longer implementor lists.tests/collections.rs: the structuralVecpath wipes its recovered prefix after a late element failure, as a column and as a column field of a structural record; a panicking element declaration wipes the elements after it.tests/owned_records.rsalso pins that a record field opened before a later refusal is wiped;tests/ui/owned_struct_unmapped_field.rspins the unaccounted-field refusal.tests/owned_records.rspanic witnesses:a_panicking_field_declaration_wipes_the_sealed_fields_after_it(a panic in the outer split and in a nested split) anda_declaration_that_panics_while_constructed_wipes_the_plaintext. Both are enrolled in theowned-recordslifecycle suite.each_owned_term_target_names_its_index. Two equivalent mutants are excluded in.cargo/mutants.toml, each with its reason:ExpectedContext::omitted -> Default::default()andDeclaredScopes::omitted -> Default::default()(both bodies areSelf::default()).tests/owned_records.rs: eight drop witnesses for a refused context, built on the reviewer's probe, one per leaking path (long scope literal, nested record under a long scope,plaintextrecord under a long caller context and a long stored context, aVec<StackCipherText>column, a scalarStackCipherText, aVec<StackCipherText>, andEncrypted<EqualityTerm>). All eight failed before the fix. Enrolled inlifecycle-buffer-cleanup-v1underowned-records.tests/ui/pass/owned_generic_plaintext.rs: a generic plaintext throughproject_ownedand throughsplit.lifecycle-profiles.json: a newowned-recordssuite enrolls the record drop witnesses; the new collection witnesses joincollections.Docs
targetmodule docs with the trait/ownership table, derive docs "Owned records" section and thepassthroughattribute row, ADR-0012 (signature/import/ownership map and the bridge decision; numbered 0012 because feat(stack-encrypt)!: add OpaqueBlob whole-value encryption #1503 takes 0011),conformance/stack-encrypt/coverage.mdrows, CHANGELOG[Unreleased](Breaking / Added / Deprecated).Verification
All run from the worktree on the pushed head
327b69398, no credentials:cargo fmt --all --check— clean.mise x -- cargo nextest run -p stack-encrypt -p stack-encrypt-derive --all-features— 1192 passed, 1 skipped, theuitrybuild suite included.cargo clippy -p stack-encrypt -p stack-encrypt-derive --all-features --all-targets -- -D warnings— clean.stack-encrypt61 passed;stack-encrypt-derive12 passed, 15 ignored.RUSTDOCFLAGS="-D warnings" cargo docfor both crates — clean.cargo mutants --listontarget/input.rs— theDeclaredScopes::omittedmutant is no longer listed. A full mutants sweep was not run; the CI gate runs--in-diff.owned-recordsandcollectionssuites (22) appears incargo nextest list.cargo check -p eql-bindings --features stack-encryptand the Go guest compiled unchanged;node scripts/stack-lifecycle-conformance.mjs rustpassed. Neither was re-run after the review fixes, since this worktree has nonode_modules; CI runs the lifecycle script.Related
EncryptFrom/DecryptIntosurface follows once every consumer has an owned equivalent; nothing is removed here)Vec<S>collections as one batch #1501 (legacyVecdecrypt path left recovered elements unwiped)Review notes
Start with
packages/stack-encrypt/docs/adr/0012-owned-record-declarations.md, thensrc/target/conversion.rs(owned_encryption),src/target/input.rs(the inputs and conversions) andpackages/stack-encrypt-derive/src/owned_encrypt.rs(splitchain).tests/owned_records.rsis the executable version of the public examples.Deliberately deferred, with reasons:
target(options = factory)attribute): the only configurable target's plaintext isserde_json::Value, which is notZeroize, so it cannot be an owned source yet. Nested targets with and without caller context are supported; a typed-options attribute waits on a zeroizing document plaintext.Option<T>and tuple targets stay structural-only in this slice (nullable scalars are deferred by the scalar slice too).eql-bindingscompiles unchanged and keeps its structural declarations. AddingEncryptTargetimpls there would build against the crates.iostack-encrypt, which lacks this surface until the next release (see AGENTS.md on thestack-encryptfeature's registry gate).#[deprecated]attribute onEncryptFrom/DecryptInto: the crate's own plan and dynamic modules implement them, so the attribute would only be noise until stack-encrypt refactor: remove transitional traits and redundant execution APIs #1488; the CHANGELOGDeprecatedsection carries the notice.#[stash(passthrough)]field type must beClone: the record's borrowed form (what a plan or a structural record runs) copies it; the owned form never does, and a test with a panickingCloneholds it to that.Summary by CodeRabbit