Repository navigation
Conversation
|
| Name | Type |
|---|---|
| @cipherstash/eql | Minor |
| stash | Patch |
| @cipherstash/stack-prisma | Patch |
| @cipherstash/basic-example | Patch |
| @cipherstash/e2e | Patch |
| @cipherstash/prisma-example | Patch |
| @cipherstash/stack-drizzle | Patch |
| @cipherstash/stack-supabase | Patch |
| @cipherstash/stack | Patch |
| @cipherstash/wizard | Patch |
| @cipherstash/bench | Patch |
| @cipherstash/test-kit | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
|
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 ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 6 | 0 | 8 | 0 |
Every mutant in the changed lines was caught by a test.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
cipherstash-agent
left a comment
There was a problem hiding this comment.
Automated review attestation — Claude Code /code-review medium, with the top finding cross-checked against head bcce9ff. Source: scheduled routine on pull_request.ready_for_review.
No critical blockers. One should-fix before merge:
packages/stack-encrypt/src/op/mod.rs:891:impl<T> LabelledTarget for Encrypted<T>lacks aT: LabelledTargetbound, so it bypasses the label guard and can cause silent no-match queries (verified at head; inline).packages/stack-encrypt/CHANGELOG.md: source-breaking API changes are recorded underAddedonly (inline).
Non-blocking, line not in diff so no inline anchor:
eql-bindings/src/encryption/query.rs~490:column()falls back to an empty column onIdentifier::from_labelfailure instead of erroring. Labels are also parsed twice.eql-bindings/src/encryption/query.rs~554:.expect(..)onbloom_filter(..)can panic in the query path. The data path returnsError::Otherfor the same condition.eql-codegen/src/targets.rs~918:answers_for_indexespanics for multi-index or ordered producible types, and the panic runs insiderows().eql-bindings/src/v4/targets.rs~733: test-onlypub const ANSWERSwidens the public API.
Nit: per-query Label clone and a duplicated Chooses::on body (inline, chain.rs).
Generated by Claude Code
| impl<S: CllwOreEncrypt> LabelledTarget for OreTerm<S> {} | ||
| impl<S: CllwOpeEncrypt> LabelledTarget for OpeTerm<S> {} | ||
| impl<T: LabelledTarget> LabelledTarget for (crate::StackCipherText, T) {} | ||
| impl<T> LabelledTarget for crate::target::Encrypted<T> {} |
There was a problem hiding this comment.
Should fix (verified at head): impl<T> LabelledTarget for Encrypted<T> has no T: LabelledTarget bound, unlike the tuple impl on line 890. An Encrypted<ByTenant> whose context is sealed under a tenant id then counts as labelled. Its queries compile and derive terms under the plan's label, those terms never match the stored ones, and nothing errors. That is the silent-no-match case the guard (op_target_not_labelled.rs) is meant to prevent. Require T: LabelledTarget.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in b9f84a5. It now reads impl<T: LabelledTarget> LabelledTarget for Encrypted<T>, the same as the tuple impl. So an Encrypted counts as labelled only when its term does. The stack-encrypt tests and the trybuild cases still pass.
|
|
||
| ### Added | ||
|
|
||
| - **A target sealed under a context of its own can opt in to queries on |
There was a problem hiding this comment.
Should fix: this entry lists source-breaking changes to a crates.io crate under ### Added only. QueryToken::new changes signature, Chooses loses its Resolves supertrait and gains choose, and external Answers impls now need LabelledTarget to stay queryable. Add a Changed/Breaking entry so release-plz and semver consumers get the signal.
Generated by Claude Code
There was a problem hiding this comment.
I left the CHANGELOG as it is. Nothing here breaks a released version: op and plan didn't exist in stack-encrypt 0.2.0, the only release with an op that could change. You can check the stack-encrypt-v0.2.0 tag, whose lib.rs has no op module. QueryToken::new, Chooses, Supports and Answers all first ship in 0.3.0. The Unreleased ### Added entry for op already describes them as they'll ship, so a Breaking entry would describe a change no published version saw.
| Ok(verifier) => verifier, | ||
| Err(error) => return Pending::failed(keyset, error), | ||
| }; | ||
| let token = QueryToken::new(label.clone(), extended); |
There was a problem hiding this comment.
Nit: label.clone() allocates on every query, though only EQL twin indexes read the label. Chooses::on also duplicates the body of QueryBuilder::via(..).on(..), so the two can drift. Consider delegating to a shared helper.
Generated by Claude Code
There was a problem hiding this comment.
I've left both as they are. The clone copies one boxed slice of segments per awaited query, which is small next to the key derivation. Avoiding it would mean derive handing the label back. The two on bodies only share the destructure-and-build of QueryOp. They resolve differently: one takes the plan's choose with its own index, the other Resolves::resolve with the caller's. A shared helper would take a closure and save about six lines.
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-crate encryption-context routing changes warrant human review, with regression execution not independently verified here.
No open issues 🎉
What changed in this PR
Adds typed Rust queries for supported EQL v4 columns, bringing Stack Encrypt’s typed query chain into parity with the data path used by Go.
Changes:
- Adds typed equality and text-match queries returning EQL query twins.
- Validates query labels and rejects unsupported context extensions.
- Adds parity tests, compile-fail coverage, examples, and release notes.
| File | Description |
|---|---|
packages/stack-encrypt/tests/ui/op_target_not_labelled.stderr |
Records missing-marker diagnostics. |
packages/stack-encrypt/tests/ui/op_target_not_labelled.rs |
Rejects targets without label compatibility. |
packages/stack-encrypt/tests/ui/op_matches_on_integer.stderr |
Updates trait-bound diagnostics. |
packages/stack-encrypt/tests/op.rs |
Tests labelled queries and early refusal. |
packages/stack-encrypt/src/target/context.rs |
Detects context extensions. |
packages/stack-encrypt/src/plan/value.rs |
Separates query layout from encryption requirements. |
packages/stack-encrypt/src/plan/chain.rs |
Adds query selection and label validation. |
packages/stack-encrypt/src/op/mod.rs |
Adds marker, validation hook, and token context. |
packages/stack-encrypt/docs/adr/0009-operators-select-the-index-and-a-query-term-is-not-the-stored-term.md |
Documents query-context validation. |
packages/stack-encrypt/CHANGELOG.md |
Records typed-query engine changes. |
packages/eql/tests/encryption/tests/typed_queries.rs |
Tests query parity and invalid contexts. |
packages/eql/tests/encryption/tests/common/mod.rs |
Tracks key loads for refusal tests. |
packages/eql/tests/encryption/Cargo.toml |
Registers the executable query example. |
packages/eql/crates/eql-codegen/src/targets.rs |
Generates operator implementations and tests. |
packages/eql/crates/eql-bindings/src/v4/targets.rs |
Adds generated query-twin implementations. |
packages/eql/crates/eql-bindings/src/v4/mod.rs |
Documents generated query support. |
packages/eql/crates/eql-bindings/src/encryption/query.rs |
Implements query twins and match verification. |
packages/eql/crates/eql-bindings/src/encryption/query_example.rs |
Demonstrates typed equality and match queries. |
packages/eql/crates/eql-bindings/src/encryption.rs |
Shares term encodings and declares indexes. |
packages/eql/crates/eql-bindings/README.md |
Explains typed Rust queries. |
packages/eql/crates/eql-bindings/CHANGELOG.md |
Records the new query API. |
.changeset/eql-v4-bindings.md |
Includes typed queries in release notes. |
Review effort: Balanced
![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: bcce9ff4ec
ℹ️ 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".
| label = "a typed plan for this target type answers no query", | ||
| note = "a target type answers queries on its plan when it implements `op::LabelledTarget`: its stored terms were derived under the context the plan's label names" | ||
| )] | ||
| pub trait LabelledTarget {} |
There was a problem hiding this comment.
This adds public APIs such as LabelledTarget, Supports::check_label, and the QueryToken accessors, but both packages/stack-encrypt/Cargo.toml and eql-bindings' dependency still name the already-versioned 0.3.0. When eql-bindings is packaged, Cargo strips the path and compiles its stack-encrypt feature against the registry's 0.3.0, which lacks these symbols, so the all-features publish check and the eventual release cannot compile. Publish these APIs under a new stack-encrypt version and update eql-bindings' version requirement before releasing the binding.
AGENTS.md reference: AGENTS.md:L507-L523
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I didn't change anything for this. 0.3.0 isn't a published version. crates.io has 0.1.0 and 0.2.0, and the in-tree stack-encrypt has been 0.3.0 since 0.2.0 shipped. So these APIs ship as part of 0.3.0, and eql-bindings' version = "0.3.0" requirement names that release. As AGENTS.md describes, stack-encrypt has to reach crates.io before eql-bindings is published. Until it does, the cargo publish --dry-run --all-features step warns rather than fails.
bcce9ff to
82a9043
Compare
|
Rebased onto
This PR is independent of #1243. I re-ran the checks locally. All of these passed:
|
| /// would match nothing. Before any keyset is resolved or term derived. | ||
| fn check_column(label: &Label, extended: bool) -> Result<(), Error> { | ||
| if Identifier::column_of(label).is_none() { | ||
| return Err(Error::Other( |
There was a problem hiding this comment.
The typed chain refuses a bad label / an extension with an untyped Error::Other(String), while the data path refuses the identical condition with TargetError::Context { label }. A caller (and this PR's own tests) can only recognise the refusal by substring-matching the message (reason.to_string().contains("is not an EQL column identifier")), so any wording tweak silently breaks that handling.
Since TargetError sits behind the targets feature, a small typed error here, e.g. QueryLabelError { NotAColumn { label }, Extended } boxed into Error::Other, and reused by Identifier::from_label, would give one matchable refusal for one condition.
There was a problem hiding this comment.
Fixed in 3eaeeb8. Both refusals are now eql_bindings::encryption::query::QueryLabelError (NotAColumn { label } or Extended), boxed into Error::Other. The message text is unchanged, so the wording still matches the data path's TargetError::Context. The tests now match the variant with downcast_ref, not a substring. I kept TargetError::Context for the data path, because TargetError is a published enum behind targets. Both paths now read the label with the same Identifier::column_of.
| fn column(token: &QueryToken) -> Identifier { | ||
| Identifier::column_of(token.label()) | ||
| .map(NonEmpty::into_inner) | ||
| .unwrap_or_else(|| Identifier { |
There was a problem hiding this comment.
column() fabricates an Identifier with an empty column when the label is not two segments, instead of asserting the invariant that check_label already ran. If a future Supports impl on TwinIndex (or a downstream QueryTwin user) forgets to delegate check_label to check_column, the result is a twin with "c": "" that Postgres rejects at bind time, far from the cause.
expect("check_label accepted the label") would make that a loud local failure that names the invariant.
There was a problem hiding this comment.
Fixed in 3eaeeb8. column() now uses expect with a message that names the invariant (check_label accepted the label as a column), instead of building an empty column.
| let extended = self.common.extend.is_extended(); | ||
| let verifier = match self | ||
| .index | ||
| .check_label(&label, extended) |
There was a problem hiding this comment.
check_label now runs twice per awaited query: once in check (before the keyset) and again here in prepare (after it), on top of admit re-running check. On the .await path the second call never adds a guarantee; for the match index that is two tokenisations of the query text per query. Only the "chain prepared on its own" path needs the prepare-side check.
Consider collapsing into one admit-style entry that takes (label, extended), or dropping the check-side call where prepare always re-checks.
There was a problem hiding this comment.
I've left this as it is. check_label doesn't tokenise anything: it reads the label's segments and checks one bool. The two tokenisations you mention come from check followed by admit, and that was already how this worked before the PR. check_label is called in the same two places as check for the same reason: check runs before the keyset, and prepare re-checks for a chain prepared on its own. Merging check and admit into one pass would be a change for every index, so it doesn't belong in this PR.
| #![doc = #mod_doc] | ||
|
|
||
| #answer_imports | ||
| use crate::Identifier; |
There was a problem hiding this comment.
render_answers_from gates the QueryTwin / SchemaVersion / Equality / Match imports on whether any row answers, but emits use crate::Identifier; unconditionally. In the case the gating exists for (no answering type) the generated answers.rs still has an unused import and the crate's clippy -D warnings job fails anyway; today the branches are dead complexity.
Either gate all the imports together or emit them unconditionally and drop answers_any.
There was a problem hiding this comment.
Fixed in 3eaeeb8. use crate::Identifier; is now inside the same gate as the other imports that only the answering types need, so a file with no answering type has no unused import. I kept the per-operator gates (Equality and Match), because a catalog with only Eq types would otherwise import Match and EqlMatch for nothing. The generated answers.rs is unchanged.
| value, | ||
| op, | ||
| } = self; | ||
| let (index, label) = match common.call_context() { |
There was a problem hiding this comment.
QueryBuilder::on now duplicates QueryVia::on almost line for line (destructure, call_context(), fallback index on error, build QueryOp). The next change to how a call-context error is carried into QueryOp has to be made twice.
A private QueryOp::from_parts(common, value, (index, label)) shared by both would remove the copy.
There was a problem hiding this comment.
Fixed in 2215991. Both ons now go through one private QueryOp::resolve, which reads the call context and carries an error into QueryOp in one place. QueryBuilder::on passes plan.choose and QueryVia::on passes plan.resolve with its named index.
| //! The query is derived under the plan's label, which must name the column | ||
| //! the value was written under: a two-segment label, table then column, is | ||
| //! the [`Identifier`] the value stores, so | ||
| //! `"users/email"` queries values written with |
There was a problem hiding this comment.
This says a two-segment label is the Identifier the value stores, but Label::check_segment refuses segments that start with a digit, - or the base64 prefix, which Identifier::for_column accepts. So keyset.encrypt_as(&v, Identifier::for_column("2fa_codes", "secret")?) succeeds while Plan::value::<String>().context("2fa_codes/secret") fails with a LabelError, and there is no other way to name that column on a plan. (Identifier::from_label on the data path has the same limit.)
Neither this doc nor the README section mentions it; a sentence pointing at the plain-segment rule would save a user concluding the chain is broken.
There was a problem hiding this comment.
Fixed in 3eaeeb8. The module docs and the README now say that a label's segments must be plain. So a table or column name that starts with a digit or - (which Identifier::for_column accepts) can't be named on a plan, and a column like that can't be queried this way.
| /// another type's value. | ||
| type Plaintext; | ||
| /// The twin of `term`, derived under `column`. | ||
| fn assemble(column: Identifier, term: T) -> Self; |
There was a problem hiding this comment.
QueryTwin::assemble is a second, hand-rolled assembly of the twin payload (v, i, term) beside the twin's own derived EncryptFrom, which the data path's query_named runs. The two agree only because typed_queries.rs compares bytes: a change to the twin envelope (a new field on the *Query structs, SchemaVersion moving) has to be made in the derive and in answer_impls separately, and drift shows up as a failing parity test rather than a type error.
The generator already knows the field list; emitting QueryTwin from the same field spec the EncryptFrom derive reads (or building the typed path on the twin's EncryptFrom) would keep one source.
There was a problem hiding this comment.
I've left this one. assemble is generated by the same generator from the same catalog functions the *Query struct generation uses (Term::json_key for the field and Term::binding_newtype for its type), so the names have one source. It also builds the twin with a struct literal (Self { v, i, hm }), so a new field on a *Query struct is a compile error, not silent drift. Building the typed path on the twin's EncryptFrom wouldn't work: that derive derives its term under the twin's own Identifier context through its own plan, while the typed chain must derive through the plan's index under the label, which is the point of TwinIndex.
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
High-effort review of head 82a9043 (base next, merge-base c612ed4), focused on the context/key binding of the new typed query path. No new findings. Everything I would have raised is already on the PR, so I am referencing those threads rather than repeating them, and recording what I verified as sound so the next reader does not redo it.
Already raised, still open at this head (non-blocking): the untyped Error::Other refusals in check_column (r4307064910), the empty-column fallback in column() (r4307064983), check_label running in both check and prepare (r4307065070, which the code comment says is deliberate so a chain prepared on its own gets the same refusal), the unconditional use crate::Identifier in render_answers_from (r4307065130), the duplicated on body (r4307065179), the Label plain-segment rule being narrower than Identifier::for_column (r4307065260), and the twin being assembled by hand beside its derived EncryptFrom (r4307065336). The earlier Encrypted<T> bound (r4305247343) is fixed at this head. The CHANGELOG/semver reply (r4306984205) checks out: stack-encrypt-v0.2.0's lib.rs has no op or plan module, so nothing here changes a published surface.
Verified sound
Terms are derived under the right context and key. A query on a Typed<T> plan never runs T's encryption; it derives through the index T::Via names, under the plan's (or the call's) label, with the same keyset semantics as every other query. For that to equal the stored term, a two-segment Label must produce the same context as Identifier::for_column, and it does: both go through Describe::to_context, pinned by the existing encryption.rs test (email.to_context() == label.to_context()). The tie to the stored value is pinned directly for TextEq (a_typed_eq_query_matches_the_stored_term) and transitively for TextMatch (typed bytes equal the data path's in typed_queries.rs; that query's filter is a subset of the stored filter and equal to the by-name value's in v4_types.rs). EqlMatch is 3-gram, downcase: true, k = 6, m = 2048, so MatchCase = CaseInsensitive is true and the "ALICE SMITH" needle pins it.
The opt-in cannot be bypassed from outside. Every pre-existing Answers impl (EqualityTerm, MatchTerms<O>, (StackCipherText, T), Encrypted<T>) has a LabelledTarget impl with the same bounds; the derive emits no Answers; Chooses is sealed; QueryToken::new is crate-private with private fields. .via(..) on a typed plan still requires ValueShape, so an EQL plan cannot be handed the bare Equality term, and a TwinIndex answers only its twin's own plaintext. The compile_fail doctests and the op_target_not_labelled trybuild case pin both directions.
Refusals come before any key material. check_label runs in check (before the keyset is resolved) and again in prepare (before admit), on the plan path and the .via path alike; a label that names no column and any .extend(..) are refused, and the tests count zero index-key loads on the named keyset.
SQL side. TextEqQuery and TextMatchQuery are exactly the RIGHTARG of = and @@ on the eql_v4 text domains, and hm/bf are now encoded by one function each for stored values and twins.
Plaintext. MatchTwin serializes as the twin only; its Debug goes through MatchQuery's positions-only Debug; PartialEq compares filters, not tokens; the tokens stay in Zeroizing. QueryToken carries only the label.
Rebase. No FfiValue remains; v4/answers.rs builds under stack-encrypt alone and targets.rs carries no Answers; no stale error names. Regenerating the bindings in place (eql-codegen bindings) leaves answers.rs, targets.rs and eql_gen.go unchanged.
Run locally at this head (no credentials, no Postgres): cargo test -p stack-encrypt --all-features (all suites incl. trybuild), cargo check -p eql-bindings under stack-encrypt and under targets, cargo test -p eql-bindings --features stack-encrypt --lib, cargo test -p eql-codegen, cargo test -p eql-encryption-tests (the six typed_queries tests and query_example pass), cargo fmt --check and clippy -D warnings on all of the above, and RUSTDOCFLAGS=-D warnings cargo doc -p eql-bindings --features stack-encrypt. Not run: test:encryption:postgres and the SQLx suite.
Generated by Claude Code
… to queries on its plan A typed plan answers an operator through its target type's `op::Answers` impl, but the chain required `Typed<T>: ValueShape<S>`: a label that converts into `T`'s context. An EQL v4 type is sealed under its column (`NonEmpty<Identifier>`), so no plan for one could be queried, and eql-bindings had nowhere to implement `Answers` (#1238). `op::LabelledTarget` is the opt-in: a target type says its stored terms were derived under the context a plan's label names. For such a type, `Chooses` asks only `ValueLayout<S>` (the indexes `T` declares), since a query derives through the index and never runs `T`'s encryption. Every other type keeps needing `ValueShape`, so a type sealed under, say, a tenant id cannot be queried under a label that would match nothing. This crate's term targets implement the trait, which keeps every query that compiled before compiling. `Chooses` now resolves the plan itself (`choose`) rather than through `Resolves`, which keeps `ValueShape`: a named index (`.via(..)`) on a plan whose type only opts in would send a term the type does not send. An EQL query value stores the column it queries, so its index needs the context its terms were derived under, and must refuse one it cannot record. `QueryToken`, which only the chain makes, now carries the label and whether the call extended it (`label`, `is_extended`): with `.extend(..)` the terms are derived under the label and the extension, so a term that records the label alone would match nothing. The defaulted `Supports::check_label` refuses such a context, and the chain runs it in `check`, where the label is resolved, before any keyset is loaded or term derived (and again in `prepare`, as for `check`). `Supports::query` stays infallible: the chain calls it only after `check_label` accepted the token's context. Every index here takes any context. Tests: a labelled target is queried under the plan's or the call's label, and its term reads whether the call extended it; a label its index refuses loads no keyset; and a UI case pins that a type with an `Answers` impl but no `LabelledTarget` answers no query on its plan.
…query chain Rust could store an EQL v4 type but query it only through a data plan: no v4 type implemented `op::Answers`, so `cipher.query(op::Eq, v).on(&plan)` did not compile on a plan for `TextEq`, while the Go SDK already had an `Eq` method per equality type and `Matches` with `Verify` on `TextMatch` (#1216). This gives Rust the same operators, with the same bytes (#1238). eql-codegen writes into `v4/answers.rs`, for each producible type with a query twin: `op::LabelledTarget` (it is sealed under the column a two-segment label names), the twin's assembly, and one `op::Answers` impl per operator. The operators come from the type's index keys, the input the data path's `TargetDescriptor::operators` reads: `"eq"` answers `Eq`, `"match"` answers `Matches`. It stops on any other index, or more than one, so making an ordered type producible has to extend the rule. The same rows give `ANSWERS`, which the `typed_queries` test holds to `TargetDescriptor::operators()` for every type. The file sits beside the target table rather than in it: `v4/targets.rs` is built on Stack Encrypt's `dynamic` layer and needs the `targets` feature, while a typed query needs neither, so `answers.rs` needs only `stack-encrypt`. The index each impl names is `TwinIndex<I, Q>`: Stack Encrypt's own index `I` (the same spec and derivation), whose query term is written into the twin `Q` with the column the query's label names, by the encodings the stored values use. It answers only for the twin's own plaintext (`QueryTwin::Plaintext`), so no plan can send one type's twin for another type's value, and its `check_label` refuses a label that names no column and any extension before any keyset is loaded: an extended query would be derived under more than the column the twin stores, and match nothing. Both refusals are a `QueryLabelError` in `Error::Other`, so a caller matches the variant rather than the message. `TextMatch` returns `MatchTwin<Q>`, which serializes as the twin and keeps the query's tokens for `verify` and `target_limit` under `EqlMatch`. `Hmac256` and `BloomFilter` now declare their indexes, so a type holding one declares the index a query is checked against, and `EqlMatch` implements `MatchCase`. Tests: every typed query is the data path's `query_term` bytes and domain (the float values the semantic baseline folds included), with the data path's inputs built as frames of Rust values; `TextMatch`'s `verify` and `target_limit` agree with the data path's; short text, a non-column label and an extended query are refused before any keyset is loaded; and compile-fail doctests for `op::Lt` on `TextEq`, `op::Matches` on `IntegerEq`, `.via(Equality)` and another type's `TwinIndex` on a `TextEq` plan, and `verify` on an exact term. The new module's example runs as a test in the encryption crate.
…nd pin the twin's plaintext tie `op::Answers` and the stack-encrypt CHANGELOG read as if only a type sealed under a context no label names needed `op::LabelledTarget`. Any type queried on a typed plan needs both, whatever its context; both now say so. The cross-type `TwinIndex` compile-fail doctest is rejected by `Resolves`' `ValueShape` bound before the plaintext tie is reached, so it would still pass if `QueryTwin::Plaintext` were removed. A doctest pair now checks the tie alone, through a `Supports<op::Eq, String>` bound: `TwinIndex<Equality, TextEqQuery>` meets it, and `TwinIndex<Equality, IntegerEqQuery>` fails with `<IntegerEqQuery as QueryTwin<Hmac256>>::Plaintext == String`.
82a9043 to
0e6db9a
Compare
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Incremental re-review of 0e6db9a (base next), against the full review on 82a9043 (5563047841). Diffed the two heads directly; they share the merge-base c612ed4, so git diff 82a90433f 0e6db9a92 is the whole amendment. I only reviewed the code that changed.
Prior findings at this head
- Untyped
Error::Otherrefusals (r4307064910): fixed.QueryLabelError::{NotAColumn { label }, Extended}is boxed intoError::Other. TheDisplaytext is byte-identical to the oldformat!({label:?}on the sameString). Both tests nowdowncast_refthe variant, and they pass. column()building an empty-columnIdentifier(r4307064983): fixed. It is anexpectnow, and I checked the expect cannot fire.QueryToken::newispub(crate)with one call site (chain.rs:926). That call builds the token from the samelabelthatcheck_labelaccepted one line earlier, andcolumn()reads it with the sameIdentifier::column_ofthatcheck_columnuses.check_labelrunning in bothcheckandprepare(r4307065070): still open, by choice, and I agree with the reply.check_labelonly reads the label's segments. The two tokenisations come fromcheckplusadmit, and those were already there before this PR.- Unconditional
use crate::Identifierin codegen (r4307065130): fixed. It is now inside the!answering.is_empty()gate. The committedv4/answers.rsis unchanged because rustfmt sorts the imports, and theeql-codegendrift tests pass. QueryBuilder::onduplicatingQueryVia::on(r4307065179): fixed. See below.- Label rule narrower than
Identifier::for_column(r4307065260): fixed in the module docs. One small README follow-up is inline. QueryTwin::assemblehand-rolling the twin (r4307065336): left as is, and the reply holds up. The generatedassembleis a struct literal (Self { v: SchemaVersion::default(), i, hm }), so adding a field to a*Querystruct is a compile error, not silent drift. The twin'sEncryptFromderives under its ownIdentifierthrough its own plan, so it can't stand in for theTwinIndexpath.typed_queries.rsstill pins what remains, the values, by comparing bytes.
The unified QueryOp::resolve: verified sound
- Same behaviour as both old bodies.
QueryBuilder::onwasOk(call) => plan.choose(call), Err(e) => (P::Via::default(), Err(e)), and it passes exactly those two closures with a()seed.QueryVia::onwasOk(call) => plan.resolve(index, call), Err(e) => (index, Err(e)), and it passes|index, call| plan.resolve(index, call)and|index| index. Each path runs exactly one of the twoFnOnceclosures on the seed, so nothing gets cloned or resolved twice. - A call context that is not a label is still refused before any key load. It becomes
label: Err(PlanError::ContextLabel).Operation::check, which runs before the keyset is resolved, returns that error on its first line, beforecheck_labelorcheckrun.preparereturnsPending::failedbeforeadmitand beforederive. - The fallback index never derives a term. On the
Errarm it is only stored inQueryOp.index. Every use of it incheckandpreparecomes after thelabelErrearly return. - Typed and data query paths still produce identical terms.
every_typed_query_is_the_data_paths_query,a_typed_eq_query_matches_the_stored_termanda_typed_match_query_verifies_and_sizes_as_the_data_path_doespass at this head.
New findings (all low; nothing blocking)
- Test gap: no test reaches the
Errarm throughQueryVia::on(.via(index).context(<not a label>).on(plan)), the only arm where the fallback is the caller's named index. No query sits in the keyset-load-counting refusal tests either (inline). - The README's list of names a label refuses reads as complete, but it isn't (inline).
- The CHANGELOG's list of what
encryption::queryholds leaves out the new publicQueryLabelError(inline).
Checks run (no credentials)
cargo nextest run -p stack-encrypt --all-features --lockedon Rust 1.94.1: 674 passed, 1 skipped, including the trybuilduicases.cargo clippy -p stack-encrypt --all-features --all-targets -D warningsandcargo doc -p stack-encrypt --all-features --no-depswith-D warnings: clean.- In the EQL workspace on stable:
cargo test -p eql-bindings --all-features,cargo test -p eql-codegen(drift), andeql-encryption-tests(typed_queries6/6,query_example,v4_types13 passed, 3 ignored for Postgres) all pass. cargo clippy -p eql-bindings --all-features --all-targets -D warningsandcargo doc -p eql-bindings --all-featureswith-D warnings: clean.
| ) -> Self { | ||
| let (index, label) = match common.call_context() { | ||
| Ok(call) => resolve(seed, call), | ||
| Err(error) => (fallback(seed), Err(error)), |
There was a problem hiding this comment.
Low (test gap). This Err arm is what keeps a call context that is not a label from deriving anything, and only one of its two callers is tested. plan_grammar.rs::a_call_context_that_is_not_a_label_is_refused reaches it through QueryBuilder::on. No test reaches it through QueryVia::on, where the fallback is the caller's named index (|index| index). Also, no query appears in the load-counting tests (a_context_refused_at_the_call_never_loads_the_keyset_it_names, a_refused_chain_never_loads_the_keyset_it_names).
I checked the code and it holds today: check returns the label error first. But if a later edit to resolve swapped the closures, or resolved the seed on the Err arm, nothing would fail. One case in the load-counting test would cover both gaps, e.g. cipher.query(op::Eq, &7u32).via(Equality).context("ages//x").on(&age_plan).keyset("tenant").await asserting PlanError::ContextLabel with loads == 0 (age_plan there has no context, so a call context is allowed).
| any keyset is resolved, with an `encryption::query::QueryLabelError` in | ||
| `Error::Other`. A label's segments are plain, so a table or column name | ||
| that starts with a digit or `-` (which `Identifier::for_column` accepts) | ||
| cannot be named on a plan, and such a column cannot be queried this way. |
There was a problem hiding this comment.
Nit. The module docs say "for instance", but this sentence reads as the complete rule. Label::check_segment also refuses segments that Identifier::for_column accepts: a segment containing /, (, ) or a control or invisible format character, or one that starts with b64:. A column named like that can be written but not queried on a plan either. Something like "a table or column name that is not a plain label segment (for instance, one that starts with a digit or -)" would cover it.
| and the query must not be extended, or the awaited query is refused | ||
| before any keyset is resolved. `eql_bindings::encryption::query` holds | ||
| `TwinIndex` (Stack Encrypt's index, its query sent as the twin, for the | ||
| twin's own plaintext only), `MatchTwin` and `QueryTwin`. |
There was a problem hiding this comment.
Nit. This lists what eql_bindings::encryption::query holds but not the new public QueryLabelError. That type is how a caller tells a refused label or extension apart from other Error::Others, and the README already names it. Worth adding here, e.g. "…refused before any keyset is resolved, with a QueryLabelError in Error::Other".
Summary
Rust code can now query an EQL v4 column through Stack Encrypt's typed query chain, as the Go SDK already can.
cipher.query(op::Eq, v).on(&plan)works on a plan forTextEq,IntegerEq,BigintEq,RealEqorDoubleEq.cipher.query(op::Matches, text).on(&plan)works on a plan forTextMatch.Each returns the type's query value (
TextEqQuery,TextMatchQuery, …), the operand EQL'seql_v4.query_*domains take. Its bytes are identical to what the data path (dynamic::query_term) derives. ATextMatchquery also hasverifyandtarget_limitunder EQL's pinned match options. An operator a type doesn't answer, such asop::LtonTextEq, doesn't compile.Background: EQL is the SQL library that stores and searches encrypted values in PostgreSQL, and Stack Encrypt is our Rust engine. Until now no EQL type could be queried through the typed chain at all.
This PR is based on
next. It was stacked on #1241, which has since merged. It doesn't depend on #1243, and #1243 doesn't depend on it. The two PRs only edit the same changelogs and changeset.Changes
Stack Encrypt (
packages/stack-encrypt)op::LabelledTarget: "my stored terms were derived under the context the plan's label names". A typed plan for aLabelledTargetis queried with only the indexes the type declares (ValueLayout). It doesn't need the type's own context to come from the label (ValueShape). That's what lets an EQL type be queried: it is sealed under a column identifier, which no label converts into.op_target_not_labelled) pins that.Choosesresolves the plan itself, through a newchoosemethod, instead of throughResolves.Resolves, and so.via(..), still needsValueShapeon a typed plan. So.via(Equality)and similar don't compile on an EQL plan.Supports::check_labelis a defaulted method. It lets an index refuse a label before the keyset or any term work. An EQL query value refuses a label that names no column, and any.extend(..), since an extended label would derive a term that stored rows never match.Supports::querystays infallible.QueryTokencarries the query's label and whether it was extended, so an EQL query value can record its column. It stays unforgeable.op,plan::value) and the CHANGELOG.opis unreleased, so none of this breaks anything against 0.2.0.eql-bindings and eql-codegen (
packages/eql)For each producible v4 type with a query twin, eql-codegen emits:
op::Answersimpl;LabelledTarget;QueryTwinassembly, withtype Plaintext(the Rust primitive:i32,i64,f32,f64orString);#[doc(hidden)]ANSWERStable.These go into a generated file of their own,
v4/answers.rs, which needs only thestack-encryptfeature. The by-name dispatch inv4/targets.rsis different: it needs thetargetsfeature and Stack Encrypt'sdynamiclayer. So a typed query doesn't pull indynamic.The answers come from the type's index keys (
"eq"answersEq,"match"answersMatches), the same input as the data path'sTargetDescriptor::operators. An order index, or more than one index, makes the generator panic, so whoever makes an ordered type producible has to extend the rule.New
eql_bindings::encryption::querymodule:TwinIndex<I, Q>wraps Stack Encrypt's index and writes its term into twinQ, with the column taken from the label.MatchTwin<Q>isTextMatch's query, withverifyandtarget_limit.Hmac256andBloomFilterdeclare their indexes. Before, a typedTextEqquery would have been refused withIndexNotDeclared.EqlMatchimplementsMatchCase(case-insensitive).The
hmandbfencodings are shared by the stored values and the twins, so each has one implementation.Docs: a README section "Querying an EQL v4 field from Rust" with an executable example, the CHANGELOG, and the changeset.
Verification
cargo nextest run -p stack-encrypt --all-featurespassed: 641 of 641, including the trybuild UI tests.-D warningsand rustdoc with-D warningsare clean.--in-diffagainst the base): 12 mutants, 6 caught, 6 unviable, 0 missed.packages/eql:mise run test:crates,test:encryption,check:encryption:wasiandtypes:checkpassed. The newtests/encryption/tests/typed_queries.rscovers:-0.0and NaN;TextMatch:verifyandtarget_limitagree with the data path across several needles, stats and limits;ANSWERSequalsTargetDescriptor::operators()for every target;compile_faildoctests, each paired with a passing version and checked by hand to fail for the intended reason:op::LtonTextEq;op::MatchesonIntegerEq;verifyonTextEqQuery;.via(Equality)on aTextEqplan;TwinIndex;mise run wasm:guest:test,go vet ./...andgo test ./... -count=1passed, andgo generate ./...leaves no diff. Nothing Go can see changed.next, these passed again:cargo nextest run -p stack-encrypt --all-features: 674 tests;-D warnings, fmt,doc:stack-encryptandtest:doc:stack-encrypt;packages/eql:test:codegen,test:crates,test:encryption(the 6typed_queriestests and the frozen-fixture tests),check:encryption:wasi,types:checkandcodegen:parity;test:encryption:postgresagainst a local PostgreSQL 17.5;-D warnings, understack-encryptalone and undertargets.typed_queries.rsbuilds the data path's plan withFieldPlan::with_targetand its inputs as frames of Rust values (ValueFrame::of). It doesn't useFfiValue.scripts/__tests__, because the worktree has nonode_modules. No workflow changed.Related
Closes #1238
Review notes
An independent review of the first version found two real problems:
.extend(..)silently built a query that matched nothing;.via(..)produce the wrong query type on an EQL plan.Both are fixed. A second review pass confirmed that, re-ran the probes (including a batch with one bad query) and found nothing new.
eql-bindings now needs the in-tree Stack Encrypt (
LabelledTarget,check_label,QueryToken::is_extended). Itsstack-encrypt = "0.3.0"requirement is unpublished, so Stack Encrypt releases first, as it already must forop.Ordered EQL types get no
Answersyet. They aren't producible (Stack Encrypt cannot order floats, dates, timestamps or decimals, and its integer order keys predate ADR-0002: derive every order term over vitaminc-ore #1236, stashgen refuses TextOrdOre and TextSearchOre — EQL stores block ORE terms and the engine derives CLLW ORE #1132).https://claude.ai/code/session_01BZhkvKLpNtKuzj3HCfMXoL