Repository navigation
feat(stack-encrypt): let a target take its context from the plaintext it encrypts - #1588
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 0 | 0 | 1 | 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.
Verdict: The combinator does what it says, and the EQL shim is a sound use of it. But as a public API it is wider than the shim needs. select sees the whole plaintext, including the part that is sealed, and nothing in the API or its docs stops a context being computed from secret data. Two documents also still say the opposite of what this PR adds. No crash, wipe or batching defects found.
What I ran on 0918e4219: tests/context_from.rs (4 passed), the context_from doctest (passed), migration_shim (4 passed), and cargo mutants --in-diff <this PR's diff> --list for stack-encrypt. That lists one mutant, operations.rs:429 replaced with Default::default(). It cannot compile, because Encryption has no Default, so the CI mutants gate has nothing that can survive. I also ran two throwaway probe tests, described below. They were not committed.
Findings
1. Medium: select can turn sealed plaintext into the context, which is sent to ZeroKMS and can be stored in the clear
packages/stack-encrypt/src/target/operations.rs:422-438
select: fn(&S) -> Result<Ctx::Value, Error> gets the whole borrowed source, including the field that project then seals. Whatever it returns becomes the context, and the context goes to three places:
- The ZeroKMS descriptor of every data-key request.
descriptor.rs:63says this string is "sent to ZeroKMS with every data-key request", andeql-bindingssays it appears in the ZeroKMS log. - The stored, unencrypted context field of a
context_fieldtarget, such asionTextEq. - The AAD. So the ciphertext cannot be opened without its own plaintext. The context-examples plan dropped two scenarios for exactly that reason.
Before this PR, no public combinator let plaintext reach a context. under and extend take values that exist when the description is built, inside a static encryption() with no source in scope. under_by, which reads the source, is pub(crate).
Failure case: someone writes .project(|s| &s.card).context_from(|s| NonEmpty::new(s.card.clone()).map(Into::into)...), a one-character slip from the intended field. It compiles and runs. CONFIRMED with a probe test against recording_cipher(): the generate request carried the descriptor b64:NDExMS0xMTExLTExMTEtMTExMQ==, which is base64 for the sealed value 4111-1111-1111-1111. Base64 is an encoding, not encryption.
The docs give "it captures nothing, so it cannot reach a cipher" as the safety argument. That covers a different risk (ADR-0004's no-source-and-cipher rule) and says nothing about this one.
Suggested fix: at minimum, a # Security paragraph saying:
selectmust read only data that is not secret;- the context is sent to ZeroKMS and may be stored in the clear;
- the output must store the context, or the caller must keep it.
Better, make the context source a separate, visible pick, as project is: for example context_from(pick: fn(&S) -> &C, into: fn(&C) -> Result<Ctx::Value, Error>). Then the conversion cannot see the sealed field, and the field choice is a one-line pick a reviewer can check. Or limit it to targets that store their context (see 2).
2. Medium/Low: the general combinator allows a pattern the API plan rules out, and the PR's own tests use it
packages/stack-encrypt/tests/context_from.rs:53-62,100 and the doc example at operations.rs:383-394. The rule is in docs/plans/2026-10-24-stack-encrypt-api-plan.md:1640-1648.
The plan says: "Nothing is read from the plaintext on one side and from the caller on the other … A program must never need to open a ciphertext to learn the context that opens it." The context examples allow a context read from the plaintext only when the record stores it in the row (2026-10-24-stack-encrypt-context-examples.md:49-51, scenario 3). The EQL shim fits that rule, because TextEq stores the column in i.
PlacedCiphertext does not. Encrypt reads the context from placed.context, the output is a bare StackCipherText that stores no context, and decrypt takes it from the caller (cipher.decrypt(sealed, nonempty!("users/email"))). If the caller has not kept the context somewhere else, the value cannot be opened. It also sits awkwardly with SDK principle 10 (docs/sdk-design-principles.md:186): the context is now chosen by row data, not fixed in source.
CONFIRMED by reading the code and the plan.
Suggested fix: do one of the following.
- Limit
context_fromto targets whose output records the context, for example acontext_fieldtarget whose input isRequiredContext<NonEmpty<C>>. - Or record this as a deliberate exception in the plan, or in an ADR amendment (see 3), with the rule that the output must store the context.
In either case, change the stack-encrypt test and the doc example to a target that stores the context, so they do not teach the pattern the plan rules out.
3. Low: ADR-0004 and two doc comments still say under and extend are the only ways to change a context
packages/stack-encrypt/docs/adr/0004-one-context-per-target-threaded-through-the-declaration-tree.md:89, heading: "underandextendare the only ways to change it".packages/stack-encrypt/src/target/operations.rs:10-11, module docs: "What a subtree may do is take a context of its own, by name, withunderorextend".operations.rs:111-118, the "One context per target" bullet. The PR updated the second bullet but not this one.
context_from is a third way. It is also different in kind: the context is chosen per value, and it replaces any surrounding context instead of extending it. ADR-0004 decision 1 accepts a target whose halves take different contexts, because each is "a literal in the declaration … visible at review". Under context_from that argument is weaker. CONFIRMED with a probe: a ciphertext half with .context_from(|s| s.a) zipped with an equality half with .context_from(|s| s.b) compiles, and runs under two different contexts per row.
Because ADR-0004 allows a declared context on each side, I do not count this as an engine defect. But an accepted ADR now says something that is no longer true.
Suggested fix: amend ADR-0004 decision 2 to list context_from and say how it differs. Then update the module doc and the first bullet.
4. Low: the rename test does not show that the ciphertext is sealed under the new column
packages/eql/tests/encryption/tests/migration_shim.rs:124-157
The negative check, decrypt_from_with_context(migrated, …, column("customers", "email")).is_err(), fails at ExpectedContext::validate (target/context.rs:262-268). That check compares the stored i with the expected column and returns ContextMismatch before any key is fetched. So it passes whatever the ciphertext is bound to. Nothing decrypts the migrated value under contact_email, and only that would show the AEAD binding moved with the rename. The test name claims it is "the column the value is stored under". CONFIRMED by reading.
Suggested fix: clone migrated and assert that it opens as "alan@example.com" under column("customers", "contact_email"). Also match Err(Error::ContextMismatch { .. }) for the old column, so the test says which check refused it.
Nits
migration_shim.rs:119:i32::decrypt_from(migrated, &keyset)passes no context, so it opens under the storediwithout checking it. Usedecrypt_from_with_context(…, column("customers", "age")), as theTextEqtest does. Also, ORE terms made under different keys compare as noise (sem/mod.rs,OreBlockTerm::compare), so the==at:115has a small chance of passing under a wrong column. Checking against 40 and 42 as well would make it a real order check.context_from.rs:126-146: the batch test shows one generate call, and that each row opens under its own context.recording_cipher()could also assert the two per-row descriptors that were sent.tests/common/mod.rsasks tests to assert descriptors rather than rely on the fake key source, which ignores them.- There is no test of a batch where one row's
selectfails.Pending::zip(pending.rs:330) fails the whole column with no key request, which is right, but it is not tested for this combinator. Nor is there a test that the plaintext is wiped whenselectfails. The defaultowned_encryption(conversion.rs:47-52) wipes throughProtectedon that path, so this is a missing test, not a bug. - Every caller writes
.map_err(|e| Error::Other(Box::new(e)))aroundNonEmpty::new. The crate already has a privatenonempty()helper for this (target/context.rs:16). Making it public, or aFrom<EmptyError> for Error, would shorten the example.
Checked and fine
- A failing
selectreturnsPending::failedbefore any key is requested. The unit test shows this throughencrypt_into, the owned path. - No closure is given the source and the cipher together.
selectis a capture-freefnpointer. - The shim's hand-written
Zeroizeskipsi. That is sound:iis the column name, which is sent to ZeroKMS and stored in the clear anyway, and the owned default wipespthroughProtectedon success, on failure and on unwind. - A
NoCallerContexttarget cannot be a plan field:FieldsBuilder::targetneedsT::ContextInput: From<CallerContext>, so it is a compile error, as documented.
…y value of the plaintext Review findings on #1588. `select` saw the whole plaintext and returned any context, so a context could be computed from the value being sealed and leave it as a ZeroKMS descriptor, a clear `i`, and an AAD that needed the plaintext to open. It also allowed a context read from the plaintext and never stored, which the API plan rules out: a program must never need the plaintext to learn the context that opens it. `context_from` now takes `pick: fn(&S) -> &C`, a reference to a field the plaintext holds, and applies only to a target whose context input is `NonEmpty<C>`: a record with a `context_field`, or an EQL type, which stores its column in `i`. It checks the context is non-empty itself, so callers no longer map the error. A `# Security` section says the field is sent to ZeroKMS and stored in the clear. ADR-0004 gains an amendment recording the third way to set a context and the limits that keep decision 2's reasons, and the module and type docs that listed only `under` and `extend` now name it. Tests move onto a target that stores its context: the value opens from the row alone; another column is a `ContextMismatch`; an empty column is refused with no key request and the plaintext wiped; each row of a column is sent under its own descriptor in one batch; and one bad row refuses the whole column. The migration shim's rename test now opens the value under the new column and shows a relabelled value fails authentication, and its ORE check orders the value between 40 and 42.
0918e42 to
7bb7234
Compare
|
Addressed in 7bb7234. The branch was also rebased onto #1587's fixes, which is why it was force-pushed.
Nits:
|
…y value of the plaintext Review findings on #1588. `select` saw the whole plaintext and returned any context, so a context could be computed from the value being sealed and leave it as a ZeroKMS descriptor, a clear `i`, and an AAD that needed the plaintext to open. It also allowed a context read from the plaintext and never stored, which the API plan rules out: a program must never need the plaintext to learn the context that opens it. `context_from` now takes `pick: fn(&S) -> &C`, a reference to a field the plaintext holds, and applies only to a target whose context input is `NonEmpty<C>`: a record with a `context_field`, or an EQL type, which stores its column in `i`. It checks the context is non-empty itself, so callers no longer map the error. A `# Security` section says the field is sent to ZeroKMS and stored in the clear. ADR-0004 gains an amendment recording the third way to set a context and the limits that keep decision 2's reasons, and the module and type docs that listed only `under` and `extend` now name it. Tests move onto a target that stores its context: the value opens from the row alone; another column is a `ContextMismatch`; an empty column is refused with no key request and the plaintext wiped; each row of a column is sent under its own descriptor in one batch; and one bad row refuses the whole column. The migration shim's rename test now opens the value under the new column and shows a relabelled value fails authentication, and its ORE check orders the value between 40 and 42.
7bb7234 to
3411933
Compare
…y value of the plaintext Review findings on #1588. `select` saw the whole plaintext and returned any context, so a context could be computed from the value being sealed and leave it as a ZeroKMS descriptor, a clear `i`, and an AAD that needed the plaintext to open. It also allowed a context read from the plaintext and never stored, which the API plan rules out: a program must never need the plaintext to learn the context that opens it. `context_from` now takes `pick: fn(&S) -> &C`, a reference to a field the plaintext holds, and applies only to a target whose context input is `NonEmpty<C>`: a record with a `context_field`, or an EQL type, which stores its column in `i`. It checks the context is non-empty itself, so callers no longer map the error. A `# Security` section says the field is sent to ZeroKMS and stored in the clear. ADR-0004 gains an amendment recording the third way to set a context and the limits that keep decision 2's reasons, and the module and type docs that listed only `under` and `extend` now name it. Tests move onto a target that stores its context: the value opens from the row alone; another column is a `ContextMismatch`; an empty column is refused with no key request and the plaintext wiped; each row of a column is sent under its own descriptor in one batch; and one bad row refuses the whole column. The migration shim's rename test now opens the value under the new column and shows a relabelled value fails authentication, and its ORE check orders the value between 40 and 42.
3411933 to
df8c975
Compare
Codex Review Summary: ✅ CompletedThis 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-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed df8c975 · posted from Claude Code · claude-sonnet-5-5 · effort unknown
Sources: code-review (medium) · claude-sonnet-5-5
Skills: code-review, review-cross-check, review-post
No findings.
Seven candidate findings were checked against df8c975 and none survived: two repeat points already raised and addressed in the existing review, and five were refuted at head.
Generated by Claude Code
… it encrypts Every context so far came from the caller. A value that arrives with the context it is stored under could not say so: a value decrypted from EQL v3 comes with its column, and re-encrypting it as v4 should read that column rather than ask for it again. The engine could already read a context from the source (`under_by`), but only for the plan builder, and only as a `Label` routed through `CallerContext`, which an EQL type cannot accept. `Encryption::context_from` reads the target's own context input out of the borrowed plaintext through a plain function pointer, so it cannot reach a cipher, and discharges the requirement into `NoCallerContext`. If the read fails, the description fails before any key is requested. It is borrowed-mode only; an owned target's default owned form wraps the plaintext and runs the borrowed one. `tests/context_from.rs` covers a term and a ciphertext under the carried context, the refusal with no key request, and a column whose rows carry different contexts in one batch. `migration_shim.rs` in the EQL encryption tests writes the v3-to-v4 shim on top of it: one generic `EncryptTarget` impl over every v4 type whose context is its column, checked against the usual query, the usual decryption, a renamed column and an empty one. Closes #1586.
…y value of the plaintext Review findings on #1588. `select` saw the whole plaintext and returned any context, so a context could be computed from the value being sealed and leave it as a ZeroKMS descriptor, a clear `i`, and an AAD that needed the plaintext to open. It also allowed a context read from the plaintext and never stored, which the API plan rules out: a program must never need the plaintext to learn the context that opens it. `context_from` now takes `pick: fn(&S) -> &C`, a reference to a field the plaintext holds, and applies only to a target whose context input is `NonEmpty<C>`: a record with a `context_field`, or an EQL type, which stores its column in `i`. It checks the context is non-empty itself, so callers no longer map the error. A `# Security` section says the field is sent to ZeroKMS and stored in the clear. ADR-0004 gains an amendment recording the third way to set a context and the limits that keep decision 2's reasons, and the module and type docs that listed only `under` and `extend` now name it. Tests move onto a target that stores its context: the value opens from the row alone; another column is a `ContextMismatch`; an empty column is refused with no key request and the plaintext wiped; each row of a column is sent under its own descriptor in one batch; and one bad row refuses the whole column. The migration shim's rename test now opens the value under the new column and shows a relabelled value fails authentication, and its ORE check orders the value between 40 and 42.
The rustdoc of Encryption::context_from, the ADR-0004 amendment, the CHANGELOG entry and the test docs now follow the plain-language skill (#1528): one idea per sentence, sentences of 25 words or fewer, literal verbs, and terms spelled out where they first appear. No facts were added or removed, and no code changed. Refs #1586
df8c975 to
a9239e0
Compare
…d API The codec went through `eql_bindings::encryption::targets`, the by-name dispatch for bindings that have no Rust type to name. That made it carry a plaintext-kind table, `ValueFrame` conversions, index-key parsing and `Pending::all` over raw bytes. A Rust program can name the types, and with #1587 (owned `EncryptTarget`/`DecryptSource` on every v4 type) and #1588 (`Encryption::context_from`) now on `next`, it uses the path a Rust application migrating its own data would. A legacy value decrypts with its column. One generic `Shim<T>` encrypts it as any v4 type, stored or query, under that column, through `context_from`. Each item's `type` is read into a serde enum, which also types the plaintext, so a plaintext of the wrong kind and an unknown type are refused while the request is read, before any key is requested. serde's own message for a wrong kind quotes the value, so only messages that name nothing but fields and types are passed on. Each list is still one ZeroKMS call, whatever mix of types it holds, because every item maps to a `Pending<Value>` and `Pending::all` merges them. The wire format is unchanged, so `tests/v4-codec.ts` and both migration suites need no change beyond comments. The tests now cover a mixed-type batch (one generate, one retrieve, order kept), a query operand matching its stored value, a payload read under another column, and a payload from another producer refused by name, as the suites' relabel tests expect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Q6HAQrFBLJvRQ1y1xmsxv
Summary
A Stack Encrypt target can now take its encryption context from a field of the plaintext. The caller then passes no context.
Every ciphertext is bound to a context, such as its table and column. Its search terms are bound to the same context.
A migration from EQL v3 to v4 needs this change. EQL (Encrypt Query Language) stores encrypted values in PostgreSQL. A value decrypted from a v3 column arrives with that column, as
{ i, p }. The value should be encrypted again as v4 under that column, without the caller passing the column again.To rename the column, the caller edits
ibefore the call. A compatibility layer for older formats works the same way.This PR is stacked on #1587. #1587 adds the owned traits to the EQL v4 types. The base branch is the branch of #1587, so the diff shows only this change.
Changes
Stack Encrypt
Encryption::context_from(pick)insrc/target/operations.rsreads the context of the target from a field of the borrowed plaintext. The description it returns needsNoCallerContext.NonEmpty<C>. A record with a#[stash(context_field)]qualifies. An EQL type qualifies too, because it stores its column ini. So opening a value never needs its plaintext.pickreturns a reference to a field of the plaintext, sopickcannot compute a context from the sealed value. When the context type differs from the type of the sealed value, the sealed field cannot be picked at all.IdentifierandStringare an example.context_fromchecks that the context is not empty. An empty context fails the encryption before any key is requested.# Securitysection. The picked field is sent to ZeroKMS and stored in the clear, so the field must not be secret.underandextendnow also namecontext_from.Tests
packages/stack-encrypt/tests/context_from.rsuses a record that stores its context. The tests check that:ContextMismatch;packages/eql/tests/encryption/tests/migration_shim.rscontains the v3-to-v4 shim from #1586. The shim is one genericEncryptTargetimpl over every v4 type whose context is its column. The tests check that:hmofTextEqequals the usual query, and the value opens under its column;IntegerOrdOreorders between the queries for 40 and 42;Verification
I ran these locally:
cargo nextest run -p stack-encrypt --all-features, on Rust 1.94.1cargo test -p stack-encrypt --all-features --doccargo test -p eql-encryption-tests --all-featurescargo mutants --listover the stack-encrypt diffDefault::default(). It does not compile.I did not run the Go suites or the Postgres suites locally. CI runs them.
Related
Closes #1586. Stacked on #1587. #1587 closes #1585. This PR is part of the work for the migration codec in #1496.
Review notes
context_fromand its docs first. Then readmigration_shim.rs. The migration shim is the use thatcontext_fromexists for.under_by.under_byispub(crate), and it takes aLabel. It also goes throughCallerContext. An EQL type cannot accept aCallerContext.