fix(proxy): stop the JSON selector fusion leaking plaintext (CIP-3682) - #435
Open
freshtonic wants to merge 12 commits into
Open
fix(proxy): stop the JSON selector fusion leaking plaintext (CIP-3682)#435freshtonic wants to merge 12 commits into
freshtonic wants to merge 12 commits into
Conversation
`WHERE col -> 'a' -> 'b' = $1` on an encrypted JSON column emitted `eql_v3.jsonb_contains(col -> 'a', <needle>)`. The rewrite took its container from the original AST, which for a single accessor is the bare column — but for a chain is another accessor. So the plaintext field name `a` shipped in the statement text PostgreSQL received, and native jsonb `->` was applied to an encrypted payload, which matched nothing. Swapping the container alone would not fix it: the needle is keyed on the path, so containment against the root column has to be keyed on the WHOLE path (`$.a.b`), not on the last step. A chain is one path into one document — the intermediate values have no independent existence, because the payload between the steps is encrypted and cannot be traversed. So the path becomes a sequence. `JsonSelectorSource` now holds the steps of the chain, each independently a literal or a placeholder, and the inference walks the accessor chain to collect them; the rewrite strips the whole chain back to its root. One unresolvable step declines the fusion exactly as one unresolvable selector did before — a partial path is not a path — and every placeholder step is an input the param plan consumes, so none is silently dropped. The chain walk only recognises the accessor spellings by name (`->`, `->>`, `jsonb_path_query`, `jsonb_path_query_first`). Accepting any two-argument call, as the previous single-step match did, would read the second argument of `coalesce(a, b)` as a selector and strip the call. Refs CIP-3682
…step The mapper now hands the proxy the STEPS of a JSON accessor chain rather than a single selector, because a chain is one path into one document. Resolve each step — a literal from the SQL or a bind param read straight off the wire — and compose them into one eJSONPath before keying the value-selector needle on it. Composition is the same normalisation a lone selector always got, applied per step and spliced onto the path so far: a step already written as a path (`jsonb_path_query_first(col, '$.a') -> 'b'`) is rooted at the path so far rather than at a second `$`, and a subscript step keeps its bracket. `col -> 'a' -> 'b'` and `jsonb_path_query_first(col, '$.a.b')` therefore key the same needle, which is what makes the two spellings of one path interchangeable. The chained-accessor integration tests are un-ignored, and assert on what the DATABASE holds: they read the stored row on a connection that bypasses Proxy, so a payload carrying any field name or value the client wrote in the clear fails the test. Refs CIP-3682
`WHERE col -> $1 = $2` with `$1` bound NULL builds no needle: there is no path to key the value selector on, so `$2` is never encrypted. The Bind rebuild then forwarded the param it was built around unchanged — which for this shape is the client's PLAINTEXT comparand. The value crossed the wire in the clear, and landed in the server log when the column's domain CHECK rejected it. An output param the plan says must be encrypted, but for which no ciphertext was produced, is now bound NULL instead of inheriting bytes. That is exactly what the SQL means — `col -> NULL = x` is NULL, so the predicate matches nothing — and it holds the invariant in one place rather than enumerating the ways a needle can fail to build (NULL selector, NULL step of a chain, NULL value). An already-NULL param is left alone rather than dirtied, so a Bind message that has not changed is not re-sent. Refs CIP-3682
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`stored_payload` connected to `PG_PORT` to read the row back on a connection that bypasses Proxy. `PG_PORT` is the non-TLS PostgreSQL on 5532, but the TLS suite CI runs puts Proxy in front of `postgres-tls` on 5617 — so the read went to a different database, found no row, and the test panicked indexing an empty result rather than asserting anything. It passed locally only because a single-database setup makes the two ports the same, which is exactly the condition that hid the bug. Now uses `get_database_port()`, which prefers `CS_DATABASE__PORT` and falls back to `PG_PORT` — the same helper `query_direct_by`, `reset_schema` and the resilience tests already use for this. This test was the only place in the suite reaching for `PG_PORT` directly. The connection is TLS now too, matching every other direct-to-PostgreSQL path.
`json_accessor_chain` matched `Expr::BinaryOp` and the accessor functions
but never `Expr::Nested`, so a single pair of brackets stopped the walk.
That is not just a missed fusion, it is the CIP-3682 leak again by
another route. On `(j -> 'foo') -> 'bar' = $1` the walker stopped at the
bracket, took the parenthesised accessor to be the ROOT container, and
emitted:
eql_v3.jsonb_contains((j -> 'foo'), $1::JSONB::eql_v3.query_json)
The plaintext selector 'foo' ships to PostgreSQL, native jsonb -> is
applied to the encrypted payload, and the needle is keyed on `$.bar`
when the real path is `$.foo.bar` — so it matches nothing either. Outer
brackets instead declined the fusion altogether and fell through to
whole-document equality over doubly-nested `->` calls.
Parentheses now unwrap at the root, at every step, and in the selector
position, so every bracketing of the same query decomposes to the same
root and the same path. `Expr::Nested` carries no meaning beyond
recording that the author typed brackets; every consumer of a chain
wants what is inside them.
Both the inference hook and the rewrite rule reach chains through this
one function, so both are fixed by it.
`->` was declared `<T>(T -> <T as JsonLike>::Accessor) -> T`, so the
result of an extraction had the SAME type as the document it came from.
An extracted SteVec entry carries no `sv` array, so traversing one
selects nothing — but nothing in the type system said so, and the only
thing standing between a user and a wrong answer was a syntactic walker.
A walker can always be defeated by moving the halves apart:
SELECT a -> 'foo' FROM (SELECT j -> 'bar' AS a FROM t) s
`a` is syntactically far from the `->` that produced it, so the path
`$.bar.foo` cannot be composed. It emitted a second entry-scoped accessor
over an entry and returned NULL, silently. Written as a predicate it was
worse: the fusion claimed it, rooted a needle at `a`, and keyed it on
`$.foo` when the real path was `$.bar.foo` — wrong rows, no error.
`EqlTerm::JsonExtracted` now types that result, with no capabilities at
all, so no operator or function can require anything of it. A type
crosses a subquery boundary where a syntactic pattern does not, which is
the whole reason to carry this in the type system.
Inference has to be fusion-aware for that to work. The operator
declaration is compositional — it sees only its immediate left operand —
but a chain is not: `j -> 'a' -> 'b'` is ONE path into ONE document.
Typing it step by step would make the first link `JsonExtracted` and the
second link fail, rejecting every chain including the ones that fuse
correctly. So the `Arrow`/`LongArrow` rule consults the chain BELOW the
node and asks whether its ROOT is a document. Within one expression the
walker always reaches the root, so an extracted intermediate is fine;
across a subquery it cannot, the root is itself extracted, and the access
is refused with a message naming the fix.
`eql_json_field_access` likewise resolves the root as a document rather
than reading the node's own type, which is what stops the fusion claiming
a chain rooted at an entry.
The declaration is left alone: it is correct for native `jsonb`, which
legitimately chains, and every encrypted case is now handled before it.
Proxy side: `JsonExtracted` reaching the encrypt path is refused rather
than defaulted to `EqlOperation::Store`. It is the result of a read, not
an operand, and storing it would encrypt the wrong payload shape and
silently return the wrong rows — the failure this term exists to prevent.
`->` and `->>` were declared to return `T`, so the result of traversing
native jsonb and the result of traversing an ENCRYPTED document had the
same type as their input. The two are not the same thing: native jsonb
yields traversable jsonb, while an encrypted document yields one SteVec
entry with no `sv` of its own, which cannot be traversed again.
`JsonLike` gains an `Output` associated type so the trait carries that
difference instead of a special case at the call site: Native resolves it
to Native, and an encrypted document resolves it to
`EqlTerm::JsonExtracted`. Plaintext chains keep working; encrypted ones
are refused wherever the chain cannot be composed into a single path.
Deliberately NOT applied to `jsonb_path_query`/`jsonb_path_query_first`.
Doing so broke two supported shapes, so it needs prior work:
- `jsonb_array_elements` and `jsonb_array_length` CONSUME an extracted
entry rather than traversing it, which is a legitimate operation the
canonical array recipe depends on. They require `JsonLike`, which an
extracted value deliberately does not satisfy.
- the rewrite that retargets these functions to their `eql_v3` twins and
encrypts their Path operand keys off the result type. Changing it sent
the caller's literal jsonpath to PostgreSQL unencrypted, which
PostgreSQL then rejected: `@ is not allowed in root expressions`.
Both point at the same missing distinction — traversing an extraction
versus consuming one — which is worth settling before the path-query
functions join this.
`->` now returns `<T as JsonLike>::Output`, so an encrypted extraction is
unqueryable — but that left the projection chain still passing, because
the fusion-aware branch intercepted before the declaration could object.
It had no way to tell `j -> 'a' -> 'b' = $1`, which fuses, from
`SELECT j -> 'a' -> 'b'`, which has no rewrite at all.
The reason is ordering. Typing is post-order, so when a chain's outermost
`->` is typed its parent does not exist yet — and the parent is precisely
what decides whether the chain is legal. `infer_enter` runs on the way
DOWN, before any child is typed, so the comparison can mark the chain
first and the `->` rule can then ask whether anything above it will
collapse the chain.
What follows from that:
- A multi-step chain is permitted only where it is marked. Unmarked, it
is refused with a message naming the fix, rather than emitting nested
entry-scoped accessors over an entry and returning NULL.
- Only EQUALITY marks. Ordering types the scalar operand but leaves the
chain standing in the emitted SQL, so a multi-step ordering comparison
has no correct rewrite either and is now refused. Single-field
ordering is unaffected.
- The whole chain SPINE is marked, one step at a time. `j->'a'->'b'->'c'`
is three nested `BinaryOp`s and every one of them is typed, so marking
only the outermost left the intermediates looking doomed and refused
the query. `json_accessor_chain` jumps to the root, so this walks
`json_accessor` instead.
- Marks are recorded against the UNNESTED node, since that is what the
`->` rule is handed for `((j -> 'a') -> 'b') = $1`.
A SINGLE access needs none of this: the declaration types it, so it stays
legal anywhere, projectable and decryptable as before. Only traversing an
extraction is refused.
A multi-step chain was refused everywhere except exact equality. Refusing
was the right call against the alternative it replaced — nested accessors
over an entry, answering NULL with no error — but it was a limitation,
not a necessity. The correct emission exists for every context, and it is
the same one equality already builds a path out of.
`eql_v3."->"(doc, sel)` searches the document's `sv` array and returns one
ENTRY, which has no `sv` of its own. So `j -> 'a' -> 'b'` cannot be two
hops; it is ONE accessor keyed on the composed path:
eql_v3."->"(j, <selector for $.a.b>)
which is exactly the shape a single access already emits. Since a chain
and a single access are both typed `JsonExtracted`, they can reach exactly
the same contexts — so once a chain collapses to that shape, legality is
uniform and there is nothing left for the refusal to protect.
What that took:
- `CollapseJsonAccessorChain` rewrites the outermost node of a chain in
one step from its ROOT, discarding the intermediate accessors whole so
no plaintext selector is left behind (CIP-3682). It runs BEFORE
`RewriteContainmentOps` and emits the finished call, which makes that
rule decline: it would otherwise key its cast decision to the type of
the original left operand, which for a chain is the accessor being
discarded rather than the operand that survives.
- A parallel path-only channel, `JsonAccessorPaths`. The operand that
survives is the outermost selector, and its own text is ONE segment of
the path it has to key — nothing the proxy is handed at encryption
time could recover the rest. It is a separate channel from the fused
equality one because the two mean different encryption ops
(`SteVecSelector` versus `SteVecValueSelector`), and a separate TYPE,
so passing one where the other belongs does not compile.
- The proxy composes. Where a `JsonAccessor` operand became
`json_selector_path(val)` it now consults the record and composes every
segment, falling back to the single-segment behaviour when there is no
record — which is what keeps a single access on exactly its old path.
Param steps resolve at Bind; literal steps at Parse.
The `infer_enter` mark is REPURPOSED, not removed. It no longer decides
whether a chain is legal — it decides which channel the path goes in.
Under `= $1` the accessor is absorbed into the needle and never appears,
so the path belongs to the value operand; anywhere else the accessor
survives and carries the path itself. Recording into both would be worse
than neither: the accessor channel resolves a literal operand at Parse
time, so `j -> $1 -> 'b' = $2` would start failing a query that works.
Equality keeps fusing. Its needle MACs path and value together and its
presence in the stored `sv` IS the match, which is strictly stronger than
an accessor plus a comparison — and `eql_v3.eq_term` has no overload for a
JSON query operand anyway. The collapse rule still fires on the accessor
below the comparison; the equality rule then discards that result and
re-roots the containment at the bare column, as before.
Two shapes stay refused, both because they are impossible rather than
unimplemented:
- Split across a subquery, CTE or view. `JsonExtracted` does not carry
the path that produced it, and the root column is not even in scope in
the outer query, so there is nothing to root a composed path at.
`UnqueryableJsonExtraction`, whose message now describes only this.
- A placeholder step in front of a LITERAL final step
(`j -> $1 -> 'b'`). The surviving operand is the literal, encrypted at
Parse time, before `$1` is bound. Same limitation the fused equality
has for `col -> $1 = 'value'`, and refused the same way — composing
only what is known would key `$.b` and read a different field.
Also fixed, because the new channel made it reachable and the old one
already had it: one placeholder used as the selector of two chains with
DIFFERENT paths silently kept whichever was recorded last, answering one
occurrence from the wrong field. The path is keyed by param number
because that is all Bind has, so there is no key that could tell the two
apart — it is now an error naming the param. The same path twice is not a
conflict.
An unresolvable step (a column reference, a function call) is refused
outright rather than declined. Unlike the fused case there is no
capability check to fall through to: the chain collapses either way, so
the step would simply vanish from the statement.
…Proxy
Type-checking is not evidence here. The failure this change fixes is not
an error — it is a query that runs perfectly, decrypts nothing, and
answers NULL. So these assert the VALUE that comes back through a live
Proxy, not merely that nothing blew up.
What each one pins that the mapper tests cannot:
- a two-level projection returns "world", the field at `$.nested.string`
- all four spellings of that path — bare, parenthesised, `->>`, and the
`jsonb_path_query_first` step — return the SAME value, so a walker
that missed one cannot pass by composing a short path
- the composed path is not a prefix of itself: `$.string` holds "hello"
and `$.nested.string` holds "world", so dropping the inner step would
read the wrong field and still look like a success
- a param step resolves at Bind, with one step bound and with both
- ordering compares the field at the path, including at the boundary
(42 >= 42), because a chain left as two hops would compare NULL and
read as "no rows" rather than as a failure
- equality still matches, unchanged
- `j -> $1 -> 'b'` never answers from a truncated path
- a chain split across a subquery is never rewritten into a value
The document is inserted through Proxy and read back through Proxy, so
the selector each query composes has to key the same path the stored
`sv` entries were keyed on — which is the part no unit test can check.
…inert `EqlTerm::JsonExtracted` was given no capabilities at all, on the theory that nothing further can be done with an extracted value. That over-reached: a SteVec entry carries ordering and equality terms, so `ORDER BY col -> 'field'` is a legitimate, previously-working query that sorts by `ord_term(eql_v3."->"(col, sel))` — and it started failing the `Ord` bound. CI caught it in the showcase, not the test suites, and the failure mode is worth recording: with mapping errors disabled (the proxy container's default) the refused statement was forwarded to PostgreSQL unmapped, so native jsonb `->` ran over ciphertext and the "active Aspirin prescriptions" query returned 0 rows with no error anywhere. The type error existed; the flag threw it away. (Exactly the failure CIP-3680 removes.) The entry's capabilities are now `eq` and `ord`, MASKED from the column's own traits rather than granted outright — a JSON domain that carries no ordering term must not gain one by extraction. The JSON capabilities stay off: an entry has no `sv`, so traversal remains refused, which is the distinction this type exists to draw. Also: the showcase's ports are now environment-overridable, mirroring the integration suite. Diagnosing this required running the showcase against an isolated Proxy, and its hardcoded 6432 was the last thing standing in the way — the same problem bf72e2f solved for the integration crate.
…he real limits
The showcase's CLAUDE.md declared chained `->` a critical limitation of
searchable encryption, to be lifted in a future release, and steered every
nested access through the jsonb_path_query* workaround. That release is
this branch: a chain written in one expression collapses into a single
keyed path against the root document, in every position, so the guidance
was steering examples away from syntax that now works — and would have
made Claude generate needlessly contorted showcase examples.
The section now documents what is actually true, including the limits
that remain and why each one exists rather than a bare "does not work":
- a path split across a subquery/CTE/view is impossible, not pending —
the extracted entry has nothing left to traverse and the root column
is out of scope; it is rejected with a type error
- an extracted field is orderable and equatable but not a document
- a placeholder step followed by a literal step is refused, because the
literal is encrypted before the placeholder is bound
- nested jsonb_path_query* calls are not collapsed; write one path
Every claim was verified against the mapper this session, including the
multi-step ORDER BY shape, which was probed rather than assumed.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes two independent ways the N:1 JSON value-selector fusion sent plaintext to the database. Both were known and both were covered by
#[ignore]d tests asserting the intended behaviour; those tests are now un-ignored and passing.1. A chained accessor shipped the selector in cleartext
emitted
eql_v3.jsonb_contains(encrypted_jsonb -> 'nested', '{…}').RewriteJsonValueSelectorEqbuilt the containment call from a container taken from the original AST: for a single-level accessor that is the bare column, which is what the rewrite wants, but for a chained accessor it is itself a->expression. So'nested'went to PostgreSQL as plaintext, and native jsonb->ran against the encrypted payload, returning 0 rows.This was not the one-line container swap it resembles. The needle is keyed on the path, so containment against the bare column has to be keyed on the whole path. A chain has no intermediate values the database can hold — the payload between steps is encrypted — so
col -> 'a' -> 'b'is one path,$.a.b, into one document.The path is therefore now a sequence of steps.
JsonSelectorSourcechanged fromenum {Literal, Param}to a struct holdingVec<JsonSelectorSegment>, each step independently a literal or a placeholder, so mixed chains likecol -> 'a' -> $1 -> 'c' = $2work. A shared syntactic walker decomposes an expression into(root container, selectors root→leaf)and is used by both sides: inference collects every selector and declines the whole fusion if any step is unresolvable (a partial path is not a path), and the rewrite strips the chain back to its root. Every placeholder step is reported byOutputParamSource::inputs(), soParamPlan::check_coversstill catches a dropped input.col -> 'a' -> 'b'andjsonb_path_query_first(col, '$.a.b')key the same needle, and single-selector behaviour is unchanged by construction.Incidental fix: the chain walker matches accessor function names. The previous code matched any two-argument call, which walked recursively would have read the second argument of
coalesce(a, b)as a selector.2. A NULL selector param forwarded the value in cleartext
WHERE encrypted_jsonb -> $1 = $2with$1bound to NULL: selector extraction yielded nothing, encryption was skipped, and the rebuild path passed the client's raw bytes for the value operand straight through.Fixed as an invariant rather than a special case: in
Bind::rewrite, an output param the plan says must be encrypted but for which no ciphertext was produced is bound NULL. That covers NULL selector, NULL step of a chain, and NULL value with one rule, and matches SQL semantics (col -> NULL = xis NULL, so no rows).Proof at the database
With
log_statement=all, the statement PostgreSQL logged for the chained-accessor query:Bare column, no
-> 'nested', andnestedappears nowhere in the log. The NULL-selector case logsParameters: $1 = NULL, where it previously logged the client's plaintext value.Testing
#[ignore]dchained_json_accessor_does_not_emit_the_plaintext_selectornow asserts the exact emitted SQL, plus new coverage for depth 3 and 4,->>, mixed spellings,jsonb_path_query_first, value-on-the-left,<>negation, per-step path recording, an unresolvable step being rejected, and unit tests for the chain walker including thecoalescecase.cargo fmt --all --checkandcargo clippy --all --all-targets -- -D warningsclean.Run the suite with
--test-threads=1:clear()truncates shared tables, so parallel execution produces meaningless failures.Acknowledgment
By submitting this pull request, I confirm that CipherStash can use, modify, copy, and redistribute this contribution, under the terms of CipherStash's choice.