Repository navigation
fix(stack-encrypt): let a non-Clone plaintext run a target operation - #1068
Conversation
|
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 0 | 0 | 13 | 0 |
Every mutant in the changed lines was caught by a test.
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Reviewed e8fe8cb (base main). Comment only.
Should fix
packages/stack-encrypt/src/target/source.rs:71—Borrowed::Source = &'s Sties the source borrow to the description's lifetime, where the olddyn FnOnce(&S, ..)was higher-ranked. Nothing in the tree breaks (encrypt_as, the derive and EQL'sTextEq/TextEqQueryimpls all infer'sper call, and a description built before its value compiles), butKeysetCipher::runis new and public and #1057'sbuild()row in the plan doc wants a boxed, reusable recipe. A boxed recipe must pin one's(dyn for<'s> Fn() -> Encryption<'s, ..>is E0582), and thenkeyset.run(recipe(), &local, ..)inside a loop fails with E0597 (reproduced in the worktree). Decide the shape now whileSourceModeis sealed: a GATtype Source<'r>keeps the borrow higher-ranked; otherwise document the constraint onrun(its rustdoc at operations.rs:552 says&S). Inline comment has the repro.
Nit
- Spec gap, already disclosed in the PR body:
projectstaysBorrowed-only, so the plan doc'sfields()row ("for the FFIselectpicks by name from anFfiValueobject") still needs each field to beClone. The scalar case that #1056 names as the hard prerequisite is unblocked; the record case is not, and #1057 should carry that as an explicit dependency.
What I checked and found fine: SourceMode/ConsumeSource/ShareSource are genuinely sealed (sealed::Sealed lives in a private mod of a private module and is not re-exported); zip in Owned mode shares as (source.clone(), source) before either side runs, so self gets the copy, other takes ownership, n operations make n-1 copies (test pins 0/1/2), and the self-then-other build order, hence key-request order, is unchanged; encrypt_native, the Term impls and the four constructors only moved the single clone from encrypt_native into Borrowed::take, no second copy anywhere; matching reads through &S in both modes with no clone; on the error paths (cx.validated() failing, descriptor failure in encrypt_native) the owned value is dropped in the closure so a zeroizing type still wipes; KeysetCipher::run is exactly encrypt_as minus the EncryptFrom lookup — one Ctx through validated(), zip still requires Ctx: Clone and feeds both halves the same value, so ADR-0004 holds and a write and a query cannot spell the context differently; run over FfiValue is a bare ciphertext() operation, not an EncryptFrom<FfiValue> target, so ADR-0004 §6 is not contradicted; EncryptFrom leaf impls keep S: Clone as documented; the two changed trybuild snapshots differ only in the added M / trailing , _ and rustc's ... truncation; the new pass/owned_without_clone.rs is picked up by ui.rs's t.pass("tests/ui/pass/*.rs"); cargo test -p stack-encrypt --all-features for source_mode, the run doctest and the ui suite all pass here; no Linear ids in the diff, commit or PR body; the commit is signed (G); the crate's CHANGELOG is release-plz/git-cliff generated, so no manual entry is needed; --in-diff mutants are all unviable (generic returns), which matches the PR's claim.
Decrypt through a plan is open, because StackCipher::decrypt already exists with another signature. A plan has two starts and takes its context from exactly one of three sources. The typed verb encrypt_into and the picker are Rust-only. The derive emits the plan only after its grammar is narrowed and the plan's widened, so the two stay one grammar. EQL types are assembled per language from standard outputs, with no registry and no target name in the data grammar. The lowering table now names what #1068, #1069 and #1071 shipped.
e8fe8cb to
849c031
Compare
|
Rebased onto main (014a04a) with no conflicts and no content change; commit hashes changed, nothing else. |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 849c031516
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…intext Review of #1068 asked whether Borrowed and Owned should derive OpaqueDebug because they stand for a plaintext. They are uninhabited marker enums: no value of either type exists, so their Debug impl can print nothing. The doc now says so, and names where the plaintext actually lives (the description's closure, which has no Debug, and the Pending output).
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes (1 of 4 review job(s) failed)
One change is needed before merge. stack-encrypt is published on crates.io, and this commit changes the arity of five public constructors. Mark the commit as breaking (! and a BREAKING CHANGE: footer), so release-plz does not publish it as a patch release with no upgrade note. The other four findings can wait: one API limit in Owned mode (a copy for zip with a match term), one usability point on KeysetCipher::run, and two test gaps.
The rest of the change is correct. The source-mode traits are sealed. The one clone moved out of encrypt_native into Borrowed::take, with no second copy. matching reads through &S in both modes. On the error paths, an owned value is dropped inside the closure, so a zeroizing type is still wiped.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 2 found, 2 posted |
| claude | claude-opus-5-5 | rust | 3 found, 3 posted |
| codex | gpt-5.6-sol | test-gap | failed |
| codex | gpt-5.6-sol | rust | 0 found, 0 posted |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 3 comment(s) had a problem that stopped the reader acting; it rewrote 3.
Stack: position 1 of 5 (:asterisk: #1068, #1069, #1071, #1073, #1074). *️⃣ marks this pull request.
Context loaded: the description, 3 linked issue(s) and 12 discussion entries.
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
Thanks for this @coderdan.
Approved, in anticipation of making this either a minor or major release, per @cipherstash-bot's feedback.
…intext Review of #1068 asked whether Borrowed and Owned should derive OpaqueDebug because they stand for a plaintext. They are uninhabited marker enums: no value of either type exists, so their Debug impl can print nothing. The doc now says so, and names where the plaintext actually lives (the description's closure, which has no Debug, and the Pending output).
849c031 to
639858b
Compare
|
Review feedback addressed. New head: 639858b (was 849c031).
Checks at the new head: nextest for stack-encrypt and stack-encrypt-derive, 338 passed (includes trybuild). |
…intext Review of #1068 asked whether Borrowed and Owned should derive OpaqueDebug because they stand for a plaintext. They are uninhabited marker enums: no value of either type exists, so their Debug impl can print nothing. The doc now says so, and names where the plaintext actually lives (the description's closure, which has no Debug, and the Pending output).
Vitamin C's Encrypt, PrfValue and CllwOreEncrypt consume the value they are given, and every target description was handed its plaintext by reference. So each operation cloned the plaintext, and ciphertext(), equality(), ore(), ope() and their EncryptFrom impls all required S: Clone. A plaintext that is deliberately not Clone, so its one copy is moved and wiped (the zeroizing FfiValue the WASI guest decodes), could not enter the target layer at all, which blocks the plan builder (#1056). An Encryption now carries a mode, M, saying how it is handed the plaintext. Borrowed (the default) hands it &S, as before: an operation that consumes clones once, so EncryptFrom declarations, records and the derive are unchanged. Owned hands it S, through the new KeysetCipher::run: a single operation consumes it with no copy and needs no Clone. zip is the one place an owned plaintext needs Clone; the first side gets a clone and the last takes the value, so n operations make n - 1 copies instead of n. A match term only reads its text and never copies in either mode. encrypt_native and the Term impls now take the plaintext by value, so the clone happens once, where a borrow becomes a value. Refs #1056 BREAKING CHANGE: Encryption gains a last type parameter, M: SourceMode (default Borrowed), and its struct declares S: 's. The five constructors ciphertext, equality, matching, ore and ope gain a source-mode type parameter M, so a turbofish that named their generics must name it too (in matching it comes before O), and a call whose result type does not fix the mode must name one. zip now asks M: ShareSource, which Borrowed always meets. Code that names Encryption<'s, S, T, K, Ctx> without a mode keeps the borrowed behaviour.
Owned mode exists so a zeroizing plaintext is moved once and wiped. A new test counts drops and asserts that ciphertext, equality and matching let the value go while the description runs, before run returns its Pending, so a later change that kept it alive across the ZeroKMS round trip fails. The compile-pass file now also runs ore and ope over an S bounded only by CllwOreEncrypt / CllwOpeEncrypt, which do not imply Clone, so Clone returning to either constructor fails the ui suite. Checked by adding it back to ore: trybuild reports `S: Clone` is not satisfied. zip's rustdoc now says that in Owned mode a match term next to a consuming operation still needs S: Clone; a combinator that avoids the copy is left until a non-Clone source with a match index arrives.
639858b to
889bef9
Compare
…intext Review of #1068 asked whether Borrowed and Owned should derive OpaqueDebug because they stand for a plaintext. They are uninhabited marker enums: no value of either type exists, so their Debug impl can print nothing. The doc now says so, and names where the plaintext actually lives (the description's closure, which has no Debug, and the Pending output).
Decrypt through a plan is open, because StackCipher::decrypt already exists with another signature. A plan has two starts and takes its context from exactly one of three sources. The typed verb encrypt_into and the picker are Rust-only. The derive emits the plan only after its grammar is narrowed and the plan's widened, so the two stay one grammar. EQL types are assembled per language from standard outputs, with no registry and no target name in the data grammar. The lowering table now names what #1068, #1069 and #1071 shipped.
3 participants
Summary
stack-encrypt is our Rust library that seals each value under its own data key from ZeroKMS (our key service) and can derive search terms (equality, match, ORE, OPE) beside the ciphertext. Its target layer, the
Encryptiondescriptions insrc/target/, required every plaintext to beClone, because each operation copied the value before encrypting it. A value that is deliberately notClone, so that its one copy is moved and then wiped from memory, could not be encrypted through that layer at all. The WebAssembly guest'sFfiValueis such a value, and the plan builder (#1056) needs it to go through this layer.This PR lets a non-
Cloneplaintext run a single operation (a ciphertext alone, or one term alone) with no copy, and moves theClonerequirement to the one place it is unavoidable: running several operations over the same owned value (zip). Existing code,EncryptFromdeclarations, records and the derive keep working unchanged.Breaking change
stack-encrypt 0.2.0 is published on crates.io, and this changes public signatures, so the next release that carries it must be 0.3.0, not 0.2.1. The commit is marked breaking (
fix(stack-encrypt)!:with aBREAKING CHANGE:footer), andpackages/stack-encrypt/CHANGELOG.mdhas a new## [Unreleased]section with these entries:Encryptiongains a last type parameter,M: SourceMode = Borrowed. Code that namesEncryption<'s, S, T, K, Ctx>still compiles and keeps the borrowed behaviour.ciphertext,equality,matching,oreandopegain a type parameterM. A turbofish must name it (inmatchingit comes beforeO), and a call whose result type does not fix the mode must name one.KeysetCipher::runandtarget::{SourceMode, ConsumeSource, ShareSource, Borrowed, Owned}.Changes
src/target/source.rs, new): anEncryptionnow has a sixth type parameterMsaying how it is handed its plaintext.Borrowed(the default) hands it&S, exactly as before.Ownedhands itSby value. Both are sealed marker types, with three small traits:SourceMode(what is handed over),ConsumeSource(get the value: free when owned, a clone when borrowed, so onlyBorrowedneedsS: Clone),ShareSource(hand one value to two operations: free when borrowed, one clone when owned, so onlyOwnedneedsS: Clone).src/target/operations.rs):ciphertext,equality,oreandopedropCloneand askM: ConsumeSource<'s, S>instead.matchingonly reads its text, so it works in either mode and never copies.zipasksM: ShareSource<'s, S>. In owned mode the last operand takes ownership and the others get a copy, sonoperations maken - 1copies instead ofn.projectstays borrowed-only: it picks a field out of a borrowed struct.KeysetCipher::run(encryption, source, context)runs a description held in a variable.sourceis&Sfor a borrowed description andSfor an owned one. Before this, the only way to run a description wasencrypt_asthrough a type'sEncryptFromimpl.src/target/core.rs,src/sem/mod.rs):encrypt_nativeand the internalTermimpls take the plaintext by value, so the singlesource.clone()moved out ofencrypt_nativeto the point where a borrow becomes a value (Borrowed: ConsumeSource).EncryptFromimpls forStackCipherText,EqualityTerm,OreTermandOpeTermkeepS: Clone. AnEncryptFromdeclaration always runs borrowed (encrypt_as(&value, ..)), and consuming a borrowed value means cloning it. A comment above the impls says so. The non-Clonepath is the constructors, run withKeysetCipher::run.Verification
Run at the current head, 639858b, with the pinned Rust 1.94.1:
mise x --env test -- cargo nextest run -p stack-encrypt -p stack-encrypt-derive --all-features: 338 passed, 0 failed (includes the trybuilduisuite).mise run test:doc: all pass, including theKeysetCipher::rundoctest, which seals aProtected<String>(notClone).mise run doc(rustdoc, warnings are errors): passes.cargo fmt --all --check: clean.cargo clippy --locked --no-deps --workspace --all-targets --all-features -- -D warnings: clean.cargo mutants --no-shuffle -p stack-auth -p stack-encrypt --in-diff <this PR's diff against main>: 13 mutants, all unviable (they replace generic returns withDefault::default()), so none survive.Cloneback onore's bounds makes trybuild fail withthe trait bound S: Clone is not satisfied.Run at the previous head (849c031), and not re-run, because the follow-up commit adds only tests and one rustdoc sentence:
mise x --env test -- cargo nextest run --locked --workspace --all-features: 778 passed, 0 failed.languages/golang/stackencrypt/guest, a separate Cargo workspace that depends on stack-encrypt):cargo clippy --locked --target wasm32-wasip1 -- -D warnings,cargo clippy --locked --all-targets -- -D warningsandcargo test --lockedall pass, with no guest changes.packages/eql, also depends on stack-encrypt by path):cargo test --locked -p eql-bindings --features stack-encryptandcargo test --locked -p eql-encryption-testspass, with no EQL changes.New tests:
tests/source_mode.rs: aSecretnewtype overProtected<String>that is notClonegoes throughciphertext(),equality()andmatching()alone in owned mode. The ciphertext decrypts to the text, and each term is byte-identical to the one the existingequality_term/match_termscalls give. With thedynamicfeature, a realFfiValueis sealed by value and opens to the same text. A clone-counting plaintext pins the copy counts: 0 for one owned operation, 1 for an ownedzipof two, 2 for three, 1 per consuming operation when borrowed, and 0 for a match term in either mode.tests/source_mode.rs,an_owned_plaintext_is_dropped_before_its_key_request_is_sent: a drop-counting plaintext shows that ciphertext, equality and matching each drop the owned value while the description runs, beforerunreturns itsPending, and exactly once. A later change that kept the value alive across the ZeroKMS round trip fails it.tests/ui/pass/owned_without_clone.rs: compile-pass check that generic functions bounded only by the scheme's own trait (Encrypt,PrfValue,AsRef<str>,CllwOreEncrypt,CllwOpeEncrypt; noClone) can run each of the five single operations.tests/ui/owned_fan_out_needs_clone.rs: compile-fail check that an ownedzipover a non-Clonetype is refused, and the error nameszipandS: Clone, not the operations.Related
Closes #1077
Refs #1056. This unblocks it: the issue lists the
S: Encrypt + Clonebound as a hard prerequisite, because the guest'sFfiValueis notClone. It is also a prerequisite for #1057 and #1059.Review notes
Encryptionnow has a sixth type parameter and rustc prints it:nested_leaf_without_context.stderr: the impl path and the quotedacceptingsignature now showM.divergent_context_in_target.stderr: the full types in thenote:lines are unchanged apart from a trailing, _. But the one-line label is now truncated by rustc toEncryption<'_, _, _, _, ..., _>and no longer showsDeclaredContext/EqualityTerm. The error and its cause are the same.M, defaulting toBorrowed. It is a sound version of "the last operand of aziptakes ownership". Each mode decides at compile time how an operation gets the value, so there is no runtime branch that could fail. Because the parameter has a default, every existingEncryption<'s, S, T, K, Ctx>spelling, including the trait signature and the derive's output, still means what it meant.build: FnOnce(S, ..)with the entry cloning):encrypt_as(&value)would then need the whole source to beClone, including a derived record's struct. Every projected field would also need a copy of the struct.Cow<'_, S>throughbuild:CowrequiresS: ToOwned, which meansClone, so a non-Clonetype cannot be in it at all. A hand-rolled owned-or-borrowed enum compiles, but its borrowed arm has to clone, so the leaves still needClone, or the arm fails at runtime.KeysetCipher::runis the combinator-level half of that issue's "execute-by-value" item, which this PR needed so the owned path could be called at all. The plan-levelrun(&plan, ..)/usingremains stack-encrypt: add the engine pieces the plan builder needs (passthrough, run-by-value, Index trait, per-field types) — 0.3.0 #1056's. It may want a different name, or may build on this one.project. A fields plan over an ownedFfiValueobject will need to move each field out instead of borrowing it, for example with a combinator that splits an owned value into its fields.projectis borrowed-only today.zip_matchingcombinator that would let a non-Clonevalue carry a ciphertext and a match term without a copy.zip's rustdoc now says that this case still needsS: Clone. The plan builder is the consumer, and its index sets go throughindexed()in feat(stack-encrypt): passthrough, Index/Indexes, Encrypted<Terms> and field types for the plan builder #1069, so the shape is decided there.run_owned/run_borrowed.runis called from generic code (feat(stack-encrypt): chained plan builder as the single front door for writes, queries and reads #1071), so the turbofish only appears in tests and doctests.packages/stack-encrypt/CONTEXT.md(landing with docs(plans): the plan builder, one front door for stack-encrypt, the derive, the FFI and Go #1052) may want an entry for the source mode (Borrowed / Owned). It is not edited here.