Repository navigation
perf(eql, stack-encrypt): store the json_tree projection, walk by parent map, budget the subtree - #1325
perf(eql, stack-encrypt): store the json_tree projection, walk by parent map, budget the subtree#1325coderdan wants to merge 13 commits into
Conversation
…ent map, budget the subtree The first measurements of the json_tree family (cipherstash/benches, branch feat/json-tree-benches) put it on par with the v3 SteVec on a flat corpus and one to four orders of magnitude behind native jsonb on a nested one: containment 70-80 ms a query, a 244-entry subtree 10 s, the GIN index 44 s to build on 10,000 rows. Three mechanisms accounted for all of it. The containment projection was rebuilt from the group tags by a recursive plpgsql function on every GIN recheck and again in the Filter, about 3 ms per 220-entry document. The client already derives that shape for the needle, so the document now writes it to the wire as `q` beside `sv`, the domain CHECK validates it as a group, and `eql_v4.json_tree_query(doc)` reads `doc -> 'q'`. It returns jsonb rather than the needle domain, since a domain cast runs the needle's recursive CHECK and the recheck would pay it per row. `q` is the selectors already in `sv`, regrouped; nothing new is revealed, and the field is required (the family is unreleased). The walk evaluated the parent tag inside a LATERAL scan of `sv`, once per (node, ordinal, entry): ~670k SHA-256 calls for the 244-entry subtree. It now builds one jsonb map of the chained entries keyed by `p` and looks each computed tag up, so a level costs its fan-out in hashes, which is the design's W x F as written. 10 s becomes ~50 ms; children and child_count 7 ms become under 3. And with the root sealed, the root's ciphertext is the whole document at about 1.3 times the plaintext, where the walk returns ~440 characters per entry and pays the fan-out per node. So `json_tree_subtree` and `json_tree_subtree_at` take a `budget` (default 64) and, past it, return the root entry alone with `d = 0`; the client opens it whole and takes the walked path over it (`JsonSubtree::at`, `JsonTreeSubtree::new(rows).at`, over the new `JsonPath::select`). Budget 0 never falls back, a token that names nothing still yields nothing, an unsealed root is always walked out. Also `json_tree_path_query_first` is costed at 1000: under a prepared statement's generic plan the default cost chose a sequential scan with the extraction as a Filter over the functional btree (17 ms against 0.12 ms). Measured in the benches repository; the Postgres oracle (`mise run test:encryption:postgres`) covers the budget, the fallback's opening at the path, and containment against `jsonb @>` with the stored projection. The design document's implementation notes record Phase 3. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
|
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 |
|---|---|---|---|
| 18 | 0 | 6 | 0 |
Every mutant in the changed lines was caught by a test.
…IN index wins over a TOASTed seq scan Re-measured after the projection moved to the wire, the nested corpus's containment queries ran as sequential scans: 1.5 s for a one-row match on 10,000 rows, 17 s on 100,000, with the GIN index present and usable (`enable_seqscan = off` took it at 0.8 ms). Inlined to `value -> 'q'`, the filter is costed as one operator per row, and a column of 97 KB documents is TOASTed out of the heap, so the main relation is a few hundred pages and the sequential scan is costed below the bitmap scan. The planner does not cost the decompression that reading `q` from each row actually pays. So `eql_v4.json_tree_query` is now plpgsql, which the planner cannot inline, with `COST 1000`. The index expression still matches the call, the call keeps its cost, and the planner takes the index on every nested containment scenario at both tiers. The per-row call is a few microseconds. The oracle suite passes against a fresh install. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
🦋 Changeset detectedLatest commit: 53a63fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…st the walk-and-projection branch The second measurement, against cipherstash/stack#1325. Every nested operation is now index-served and within an order of magnitude of native jsonb except the subtree walks, which are per-node hashing on the server and sit at 3 to 13 ms per row (23 ms walked out in full). The report records the four mechanisms the two runs found and what each measures now, with the one this run found itself: a TOASTed column makes a sequential scan look free to the planner, so the projection reader has to stay out of its inlining to keep the GIN index. Also the v3 SteVec and plaintext baselines at 100k rows, re-measured today on the same cluster. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…e fallback's refusal messages CI found two gaps. The DB-free round-trip test still asserted the stored document's keys as `h, i, sv, v`; `q` is one of them now. And the in-diff mutants gate deleted the fallback opening's `0 =>` arm and survived: both refusals are `Error::OpenedType`, and the test checked only the variant. It now asserts each refusal's `expected` text, so a path that reaches nothing and a wildcard that reaches two nodes are told apart. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…-and-projection Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
d80a33a to
cfd5dea
Compare
6427424 to
b9ac367
Compare
…-and-projection Brings up the review fixes to the json_tree design record and its SQL prototypes from docs/json-index-stack-encrypt-spike. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…-and-projection Brings up the #1288 review fixes: the 127-level depth bound, the keyed token step and member rank, the bounds on reading a stored document, the JsonTerms Debug, AtPath<Eq> as equality, the entry-versus-jsonb blockers and the changelog. One conflict, in functions.sql: this branch rewrote json_tree_subtree and json_tree_subtree_at around the parent map and the budget, and the fix added a depth bound to the old walks. This branch's walks are kept, with the bound re-applied to their recursive terms (WHERE c.entry IS NOT NULL AND w.depth < 127), since they still unwrap w and a cyclic document could otherwise keep an uncapped walk recursing. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…-and-projection Brings up the array case of the rebuild-bound test from #1288, which the mutants gate asked for. No conflicts. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Review of the walk-and-projection pass
Scope: what this PR adds over #1288 (git diff origin/feat/json-index-stack-encrypt...origin/perf/json-tree-walk-and-projection). Background for a reader new to EQL: a json_tree column stores one encrypted entry per JSON node. The database answers containment (@>) from selectors it cannot read, and walks a subtree from a "token" the client derives from a path. This PR makes three changes. The client now stores the containment projection q beside the entries. The walk looks children up in a map instead of rescanning the document. And past a budget, a walk returns the sealed root (the whole document in one ciphertext) for the client to open.
Findings by severity
| # | Severity | Finding |
|---|---|---|
| 1 | High | When the server falls back to the sealed root, opening json_tree_subtree rows as Vec<Value> gives a different shape. A caller reading an encrypted array's elements gets back the whole array as a single element once the subtree passes 64 entries. No error is raised. |
| 2 | Medium | A document whose wrapped tokens loop back makes json_tree_subtree / json_tree_subtree_at produce 2^127 rows. The depth bound does not stop a loop that branches. Demonstrated with a three-entry document: the query runs until statement_timeout. |
| 3 | Medium | The walk probes as many ordinals as the document's largest o asks for. One entry with o = 2147483646 makes json_tree_children / json_tree_child_count and every walk level hash two billion times. Demonstrated: the query runs until the timeout. |
| 4 | Medium (owner decision) | The budget is on by default (DEFAULT 64). A caller that never attaches a path works in tests with small fixtures and fails with OpenedType in production, on the first document whose subtree passes 64 entries. |
| 5 | Low (owner decision; docs fixed) | The server now trusts q, and nothing ties it to sv. A row written by anything but Stack Encrypt can answer @> differently from ->, the walks and decryption. |
| 6 | Low | The Postgres suite does not cover three of the new shapes: an unsealed root under a budget (the SQL's flag bit against the Rust header), Vec<Value> over a fallen-back json_tree_subtree, and hostile documents. |
| 7 | Low | The numbers in the design doc and the changeset do not match the report this PR cites (cipherstash/benches 57abcf9), and neither document cites it. |
| 8 | Low | The fallback has the client decrypt the whole document to answer a subtree read. That costs the client something the benchmarks do not measure (no decrypt). It also widens the "plaintext copies are not zeroized" note from one subtree to the whole document. |
| 9 | Nit | stack-encrypt's CHANGELOG does not mention JsonSubtree::at / JsonPath::select / q. eql-bindings' CHANGELOG has no json_tree entry at all. operators.sql:66 still says json_tree_query(a)::jsonb. The PR body's test counts predate the #1288 merges. |
Each finding has an inline comment that says what breaks for a user.
What I checked and found sound
- No security property is loosened by the projection.
qholds the same{s, vs}pairs assv, grouped exactly assv's ownn/gtags already group them. Neither this projection nor the old server-built one was authenticated, so storing it reveals nothing new and skips no AAD.JsonDocumentignores a storedqand derives it again from the entries, so the client never trusts it. - The fallback opens under the same AAD as any root read (
j = 0, the document's header). A server that falls back when it need not still produces the right answer. The fallback takes the path on the client, so it is not exposed to the open issue thatJsonNode/JsonSubtreeopen under whichever selector the server's row carries (that issue is still the owner's). The walked rows still are. - The map-based walk matches the Rust derivations.
json_tree_by_parentkeys onp. The tag and unwrap formulas did not change.doc.meta || entry || {d}is whatjson_tree_graftbuilt, because the map addsj. The sealed-root bit (byte 1, bit 0) matchesJsonHeader::to_bytes. - #1288's bounds survived the merge. Both walks still carry
AND w.depth < 127. The extra comparison costs nothing measurable, so the merge did not undo the performance work. cargo nextest run -p stack-encrypt --all-featurespasses (755 passed, 1 skipped). Clippy with-D warningspasses with all features and with none.cargo fmt --checkis clean.- The report at cipherstash/benches 57abcf9 reproduces the PR body's table: 68 → 3.2 ms, 0.70, 0.50, 2.8 ms, 9.5 / 23 ms, 1.6 ms, 44 → 5 s. One exception: the report's table counts 122 rows for the walked
$.orderssubtree, where the PR body and the report's prose say "the same 244 entries". Please check which is right; finding 7 has the details.
I'm pushing fixes for 1, 2, 3, 6, 7, 8 and 9 to this branch, plus the documentation half of 5. Findings 4 and 5 need a decision from you.
| type Context = AeadContext; | ||
| fn decryption<K: 'static>(self, context: Self::Context) -> Decryption<Vec<Value>, K> { | ||
| Decryption::from_open(move |cipher| { | ||
| if self.is_sealed_root() { |
There was a problem hiding this comment.
[High] When the server falls back to the sealed root, Vec<Value> over json_tree_subtree rows changes shape
What breaks for a user: they keep a list in an encrypted JSON column (say $.orders) and read its elements with json_tree_subtree(doc, token_of("$.orders")), opening the rows as Vec<Value>. Walked, the rows open to the elements: [o1, o2, …]. Once the list grows past 64 entries, the server returns the sealed root instead. The caller attaches the path, as the docs say (.at("$.orders")), and the same call now returns [[o1, o2, …]]: one element, the whole array. No error is raised. The code takes the other branch only on large documents, so tests with small fixtures never see it.
Cause: on the walked branch, Vec<Value> opens json_tree_subtree rows to the node's children (the forest is rooted at d = 1). On the fallback branch, path.select(&whole) returns the node itself. For json_tree_subtree_at rows the two branches agree, because there the forest roots are the matched nodes. The rows cannot tell the fallback which walk produced them. A positional path does not tell it either: #1329 walks positional paths through gates, so the shape of the path does not decide the walk.
Value has the same problem in the other direction: walked json_tree_subtree_at rows assemble into {"b": v}, while the fallback gives v. #1329 already fixes the walked side of that.
Fix: say which walk the rows came from. JsonSubtree::below(path) marks json_tree_subtree rows: Vec<Value> opens to the children of the one node at path, and Value to that node. at(path) stays as it is, for json_tree_subtree_at. The same two methods go on JsonTreeSubtree. Tests open the same subtree walked and fallen back and require the two results to be equal.
There was a problem hiding this comment.
Fixed in 923364d: JsonSubtree::below(path) (and JsonTreeSubtree::below) marks json_tree_subtree rows, so a fallen-back list opens to the node's children, as the walked rows do; at keeps the json_tree_subtree_at meaning. the_fallback_opens_to_what_the_walk_opens_to opens an array, an object and a scalar both walked and fallen back; 31d5da4 pins that the last attachment decides. Carried to #1329 in fee616f, where the test walks chain heads, since only a head's token is derived there.
| -- whose tags form a cycle or a longer chain, which whoever holds a | ||
| -- token can write, stops here instead of recursing without end. | ||
| WHERE w.depth < 127 | ||
| WHERE c.entry IS NOT NULL AND w.depth < 127 |
There was a problem hiding this comment.
[Medium] A looping chain makes the walk exponential; the depth bound does not stop it
What breaks for a user: one row stops a query. Nothing authenticates w, and the unwrap is public (w XOR H("w" ‖ iv ‖ t ‖ c)). So whoever holds a token can write a child whose unwrapped token is its parent's own token. Tokens are sent to the database in queries, so anyone who can read query logs and write a row holds one. Two such children per node double the rows at every level, and the depth bound allows 127 levels: 2^127 rows. Demonstrated on this branch against PostgreSQL 17 with a three-entry document (a root and two looping children): json_tree_subtree(doc, token, 0) runs until statement_timeout.
It applies to json_tree_subtree with budget = 0 or an unsealed root, and to json_tree_subtree_at under any budget when no looping entry carries the selector. That is because the budget counts only emitted rows, and the LIMIT sits after the hit filter. The comment above this line ("stops here instead of recursing without end") holds only for a loop that does not branch.
The walk was like this in #1288's version too. This PR rewrote it and kept the bound, so this is the place to close it. The Rust walk already stops at the document's entry count (design doc, Phase 1-2 notes).
Fix: an honest walk visits each entry at most once. So read the walk through LIMIT jsonb_array_length(sv) (the seed row plus at most entries - 1 non-root entries) before the hit filter and the budget. PostgreSQL evaluates a recursive CTE only as far as its reader pulls rows, so the LIMIT stops the recursion as well as the output. The Postgres suite gets the looping document, under a statement_timeout so a regression fails instead of hanging.
#1329 derives a child's token by hashing (H(t ‖ o)), so a loop there would need a SHA-256 cycle. That walk needs neither bound.
There was a problem hiding this comment.
Fixed in c1059d7: both walks read the recursion through LIMIT the entry count, which stops the recursion and not just the output. The three-entry looping document now returns 2 rows in about 1 ms. postgres_walks_end_on_hostile_documents builds it sealed and unsealed, under budgets 0 and 64, for both walks. In #1329 (fee616f) the ordinal chain cannot loop, because a child's token is a hash of its parent's, so that branch keeps its own walk. Its version of the test is a 300-deep stored chain, which still ends after one row per entry.
| ) | ||
| SELECT (doc.meta || c.entry || '{"d": 1}'::jsonb)::public.{{prefix}}_json_tree_entry | ||
| FROM doc | ||
| CROSS JOIN generate_series(0, doc.fanout - 1) AS o |
There was a problem hiding this comment.
[Medium] The document chooses how many ordinals every walk probes
What breaks for a user: the same as finding 2, with one entry. json_tree_fanout is max(o) + 1 over the document. A stored entry with "o": 2147483646 makes generate_series(0, doc.fanout - 1) produce two billion ordinals, and each costs a SHA-256. That cost lands on every node of a walk, and on json_tree_children and json_tree_child_count. Demonstrated: a two-entry document makes json_tree_child_count run until statement_timeout.
The #1288 review closed this on the Rust side (children() checks each entry at its own ordinal instead of looping over a fan-out the document chooses), but not in SQL.
Fix: an honest document's ordinals under any parent are below its entry count, so json_tree_fanout takes LEAST(max(o) + 1, entries). An honest walk then costs at most entries × fanout hashes. #1329 probes ordinals until the first miss and has no fan-out, so it needs no bound.
There was a problem hiding this comment.
| CREATE FUNCTION {{prefix}}.json_tree_subtree(e public.{{prefix}}_json_tree, token text) | ||
| --! depth `d` below the node, in depth-first order; or the root | ||
| --! entry alone, with `d = 0`, past the budget. | ||
| CREATE FUNCTION {{prefix}}.json_tree_subtree(e public.{{prefix}}_json_tree, token text, budget integer DEFAULT 64) |
There was a problem hiding this comment.
[Medium, needs your decision] The budget is on by default, and a caller that does not know about it fails only on large documents
What breaks for a user: they call json_tree_subtree(doc, token) and open the rows with JsonTreeSubtree::new(rows), as #1288 taught them. That works on every fixture and in staging. In production, the first document whose subtree passes 64 entries comes back as the sealed root, and the open fails with OpenedType ("the walked path, attached with JsonSubtree::at, …"). The error message is clear. The problem is when it appears: only on large documents, and at runtime.
Options:
- (a) Keep
DEFAULT 64and accept this. The family is unreleased, so no existing caller exists yet, and the docs and changeset say it. - (b) Default to
0. The fallback becomes opt-in for the caller who attaches a path, and the performance win becomes opt-in with it. - (c) Keep 64, and make the client API make a path-less open impossible. For example,
JsonTreeSubtree::newtakes the path, or the key returns the subtree wrapper.
I have not changed this. (a) and (c) keep the PR's headline numbers. (b) is the conservative choice. I'd lean to (c) in Phase 4 with (a) until then, but it is your call.
There was a problem hiding this comment.
Not changed; this needs your decision. I recorded it as open in the design doc's Phase 3 notes (014d25d).
There was a problem hiding this comment.
The owner chose option (c): a path-less open is now impossible. This is f1f27f2 on #1329's branch (feat/json-tree-ordinal-chain). JsonSubtreeKey now carries the JsonPath it was derived from and builds the wrapper itself: key.open(rows) in stack-encrypt, JsonTreeSubtree::from_key(&key, rows) in eql-bindings. The key also decides between below and at from its gates: no gates means the rows came from json_tree_subtree, gates mean json_tree_subtree_at. The path-less constructors and the below and at attachments are gone, so a caller can no longer forget the path. A new test, the_key_built_fallback_opens_to_what_the_walk_opens_to, checks ten paths, gated and ungated. For each, the wrapper the key builds opens past the budget to the same value as the walked rows.
| AS $$ | ||
| SELECT {{prefix}}_internal.json_tree_group(e::jsonb -> 'sv', NULL)::{{prefix}}.query_json_tree | ||
| BEGIN | ||
| RETURN e::jsonb -> 'q'; |
There was a problem hiding this comment.
[Low, needs your decision; docs fixed] The server trusts q, and nothing ties it to sv
What breaks for a user: before this PR, the server derived the projection from sv, so @> and the entries could not disagree. Now @> reads q as written. A row written by anything other than Stack Encrypt (a hand migration, another client, a future client with a bug) can answer containment from q while ->, the walks and decryption answer from sv, and nothing notices. This is not a confidentiality loss: the old projection was not authenticated either, and a database that wants to lie can always lie. It is a consistency guarantee that the old design had and this one drops.
Options: (a) accept and document it; (b) have the domain CHECK rebuild q from sv at write time and compare. (b) moves the old recursive projection from every recheck to every write, roughly the 44 s per 10k rows the index build used to cost, now paid at insert.
I've documented (a) in types.sql and the design doc, so the trust is stated rather than implied. Whether to pay for (b) is your decision.
There was a problem hiding this comment.
| }; | ||
| assert_eq!(walked(&mut db, 6).len(), 6, "at the budget, the subtree"); | ||
| assert_eq!(walked(&mut db, 0).len(), 6, "budget 0 never falls back"); | ||
| let root = walked(&mut db, 5); |
There was a problem hiding this comment.
[Low] The Postgres suite misses three of the new shapes
This suite is what holds the SQL and the Rust together, and it does not exercise:
- An unsealed root under a budget. Nothing checks that
json_tree_sealed_rootreads the same bitJsonHeader::to_byteswrites. A wrong bit would turn the fallback on for unsealed documents, whose root has no document to return, so every large walk would come back unreadable, or it would turn the fallback off everywhere. Tests below the budget pass either way. Vec<Value>over a fallen-backjson_tree_subtree: finding 1.- Hostile documents: findings 2 and 3.
Fix: add all three, the hostile ones under a statement_timeout so a regression fails instead of hanging the suite.
There was a problem hiding this comment.
Fixed in c1059d7. The suite now pins the budget to the header's sealed-root flag (with the flags byte cleared, the walk returns all 6 rows), opens the list walked and fallen back to the same elements, and builds both hostile documents under a statement_timeout. #1336 runs this suite in CI on every supported PostgreSQL, so the hostile test runs there too after cd8c060.
|
|
||
| The family was measured at 10,000 rows in the `cipherstash/benches` repository (branch `feat/json-tree-benches`, report `report/JSON_TREE_COMPARISON.md`) against the v3 SteVec and native `jsonb`, on a flat five-node corpus and a nested corpus of about 220 nodes per document. The flat corpus was on par with v3 (containment 0.47 ms against 0.23 ms, equality at a path 0.70 against 0.81 ms, the containment index eleven times smaller). The nested corpus was not: containment 70 to 80 ms per query, a 244-entry subtree 10 s, the GIN index 44 s to build. Three mechanisms accounted for it, and the fixes landed in the pull request stacked on the family's: | ||
|
|
||
| - **The projection is stored, not rebuilt.** Decision 9 above chose to build the element-nested projection server-side from the tags (`eql_v4_internal.json_tree_group`, recursive plpgsql). That function ran once per candidate row in the lossy GIN recheck, and again in the Filter, at about 3 ms per 220-entry document: the whole cost of containment on the nested corpus, and of the index build. The client already derives the shape for the needle, so the document now writes it to the wire as `q` beside `sv`, the domain CHECK validates it as a group, and `eql_v4.json_tree_query(doc)` is `doc -> 'q'`. It returns `jsonb` rather than `eql_v4.query_json_tree`, because a domain cast runs the needle's recursive CHECK, and the recheck would pay it per row. And it is plpgsql with `COST 1000`, not an inlinable SQL function, for a reason the first re-measurement found: inlined to `value -> 'q'` the filter is costed as one operator, and the nested table's rows are TOASTed, so its main heap is a few hundred pages and a sequential scan over it was costed below the GIN bitmap scan and chosen, at 1.5 s for a one-row match on 10,000 rows (17 s on 100,000), because every row is decompressed to read `q`. Un-inlined, the call keeps its cost, the index expression `eql_v4.json_tree_query(col)` still matches it, and the planner takes the index: 0.8 ms. Nothing new is revealed: `q` is the selectors already in `sv`, regrouped, at about seven per cent of the document. The deviation this replaces, "9, the projection is built from the tags server-side", is withdrawn; the `groups_ok` fallback remains unneeded. |
There was a problem hiding this comment.
[Low] The design doc and the changeset quote numbers the cited report does not contain
The PR body cites cipherstash/benches 57abcf9 (report/JSON_TREE_COMPARISON.md), and its table matches that report. The design doc and the changeset carry numbers from an earlier interim run, and neither cites the commit:
| claim | design doc / changeset | report at 57abcf9 |
|---|---|---|
| 244-entry subtree walked | about 50 ms | 23 ms |
| six-entry subtree | 3.7 ms | 2.8 ms |
| sealed root returned | about 8 ms | 9.5 ms |
one-row @> through the index |
0.8 ms | 0.5–0.7 ms |
q's share of the document |
about 7% | 19,100 of 114,309 characters, about 17% |
| nested document vs plaintext | 24× | 29× in characters, 34× on disk |
The "7%" matters most: it is the storage cost of this design choice, and the measured figure is more than twice that.
Separately, the report's own table counts 122 rows for subtree/orders_walked at 10k (130 at 100k). Its prose, and the PR body, call it "the same 244 entries" as the first run's 10.2 s row. If the two runs sampled different rows, the 10.2 s → 23 ms comparison is not like for like. Please check which count is right.
Fix: align the doc and the changeset to the report, cite the commit, and leave the 244/122 question to you.
There was a problem hiding this comment.
Fixed in 014d25d. The design doc and the changeset now quote the report at 57abcf9 and name both bench commits. The 244 vs 122 count is flagged in the doc as still to be confirmed, because the report itself gives both numbers.
| /// Open the sealed root a walk fell back to: the whole document, with the | ||
| /// path the rows were keyed from, which the caller selects over. Refused | ||
| /// without the path, since nothing in the rows records it. | ||
| fn open_fallback<'a, K>( |
There was a problem hiding this comment.
[Low] The fallback has the client decrypt the whole document for a subtree read
What changes for a user: past the budget, a request for $.orders makes the client decrypt the entire document (every field, including ones the caller never asked for), parse it into a serde_json::Value, and select the path from it. Access does not widen: it is one data key, and the caller could read the root anyway. Two things are not said, though:
- Cost. "Cheaper on both sides" is measured only on the server side. The report states "no decrypt", so the client's AES-GCM pass and JSON parse over a document of about 1.3× the plaintext size are not counted.
- Plaintext in memory. The Phase 1-2 notes record that the walk's plaintext copies are not zeroized. Under the fallback that now covers the whole document on every large subtree read, not one subtree.
Fix: say both in the design doc's Phase 3 notes. No code change.
There was a problem hiding this comment.
Fixed in 014d25d. The Phase 3 notes now state the client-side cost that the benchmarks leave out, and that past the budget the whole document's plaintext is in client memory (not zeroized).
There was a problem hiding this comment.
Related, on #1329's branch in 63bec8f: the fallback's whole document is now held under vitaminc's Protected (Protected<PlainJson>, a Zeroize newtype over serde_json::Value). The walked path is selected inside the wrapper, so only the selected nodes reach the caller, and the document is wiped when the selection is done. Every other plaintext copy in the json module is held the same way, on both the seal side and the open side.
|
|
||
| Also: `eql_v4.json_tree_path_query_first`, the function `->` and the ordering comparisons extract through, is costed at 1000 so a prepared statement's generic plan keeps the functional btree on `eql_v4.ord_term(col -> sel)` instead of filtering a sequential scan (17 ms down to 0.1 ms after the fifth execution). `eql_v4_internal.json_tree_group`, the server-side projection builder, is removed. | ||
|
|
||
| The `eql-bindings` crate's `JsonTreeDocument` gains the `q` field and `JsonTreeSubtree` becomes a struct with `rows` and an optional `path`; `stack-encrypt`'s `JsonDocument` writes `q` on the wire and ignores it on the way in, `JsonPath::select` evaluates a path over a value, and `JsonSubtree::at`, `path` and `is_sealed_root` carry the fallback. EQL v4 only; the v3 bundle is unchanged. |
There was a problem hiding this comment.
[Nit] Release notes and stale text
packages/stack-encrypt/CHANGELOG.md's unreleased JSON index entry does not mentionJsonSubtree::at/below,JsonPath::select, or theqfield on the wire. The crate's public API grew in this PR.packages/eql/crates/eql-bindings/CHANGELOG.mdhas nojson_treeentry at all, so this PR's change toJsonTreeSubtree(from a tuple struct to{rows, path}) has nowhere to be recorded.packages/eql/src/v3/json_tree/operators.sql:66still says the operator inlines overjson_tree_query(a)::jsonb.- The PR body's "734 passed" predates the feat(stack-encrypt, eql): the JSON index and the json_tree EQL v4 domain family #1288 merges (755 now), and the Changes list will need
below.
… walked rows open to Past its budget a walk returns the sealed root, and the client selects the attached path over the whole document. For json_tree_subtree rows that gave the wrong shape as a list: the walked rows open to the children of the token's node, the fallback opened to the node itself. So a caller reading an encrypted array's elements got the whole array as one element once the subtree passed 64 entries, silently. The rows cannot say which walk produced them, and neither can the path's shape (the ordinal chain walks positional paths through gates), so the caller says it: JsonSubtree::below(path) for json_tree_subtree rows, where a list opens to the node's children, and at(path), as before, for json_tree_subtree_at rows, where it opens to the nodes the path reaches. The same pair is on JsonTreeSubtree. A test opens each shape both walked and fallen back and requires the two to agree. Also records the fallback and the stored projection in the stack-encrypt CHANGELOG, and gives eql-bindings' CHANGELOG the json_tree entry it never had. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…its depth A stored document's chain fields are not authenticated, and two of them let a row stop a query. `w` wraps a child's token under its parent's with a public unwrap, so whoever holds a token can write children whose tokens are their parent's: two per node double the walk's rows at each of the 127 levels the depth bound allows. And the fan-out every walk level probes was the document's largest `o` plus one, so one entry claiming o = 2147483646 made json_tree_children hash two billion tags. Both ran until statement_timeout on a three-entry document. A sealed document's walk visits each entry at most once, and a node's ordinals sit below the entry count, so both caps cost an honest walk nothing: the walks read their recursion through LIMIT entries (which also stops the recursion, since PostgreSQL computes a recursive query only as far as its reader pulls), and json_tree_fanout takes the least of max(o) + 1 and the entry count. The Postgres suite now builds both hostile documents, sealed and not, under a statement_timeout so a regression fails instead of hanging; and pins the two fallback shapes the suite did not cover: a list opened walked and fallen back, and the budget gated on the header's sealed-root flag. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…e, and state what q is trusted for The design doc and the changeset quoted an interim run (50 ms for the walked subtree, 0.8 ms for a one-row match, q at seven per cent of the document, the document at 24 times its plaintext) where the report the PR cites, cipherstash/benches 57abcf9, says 23 ms, 0.5 to 0.7 ms, about 17 per cent and 29 times. They now quote the report and name its commits, and flag the one count the report disagrees with itself on. They also say what the numbers leave out: the fallback's cost on the client, which the benchmarks do not decrypt, and that it puts every field of the document in the client's memory for a subtree read; and that the server trusts q as written, so a row not written by the index can answer @> differently from its entries. Whether to verify q at write time, and whether the budget should be on by default, are left open for the owner. operators.sql still described the index expression with the ::jsonb cast the projection no longer needs. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…k's walk `at` and `below` each set both the path and which walk the rows came from; a test that only compared paths would let either keep the other's walk. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…-ordinal-chain Carries the #1325 review fixes up. The ordinal chain already replaces the code two of them fixed, so this branch keeps its own behaviour there: - functions.sql is this branch's. The fan-out cap and the walk's entry-count limit guard the wrapped-token chain and its stored fan-out, neither of which exists here: a child's token is a hash of its parent's, so a chain cannot loop, and the walk probes to the first miss. The hostile-document test is rewritten for this chain: a stored chain of 300, deeper than any sealed document, still ends after one row per entry, under every budget and root option. - JsonSubtree::below and the fallback's list shape apply unchanged. The engine test walks chain heads, since json_tree_subtree takes a token that names its node and only a head's is derived; the Postgres test opens `$.nested` through json_tree_subtree with `below` and `$.nested.list` through the gated walk with `at`, each walked and fallen back. - types.sql keeps this branch's description of q (the only place the value selectors live) and adds that its CHECK does not tie it to sv. - The CHANGELOG and changeset keep this branch's text, add the fallback attachment, and state the walk's bound without the mechanism the ordinal chain replaced. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
Carries the #1325 review fixes up through #1329. One conflict: both branches appended a PostgreSQL test at the end of the json_tree suite (this branch's planner test, the hostile-document test from below), and both are kept. Since this branch runs the suite in CI on every supported PostgreSQL, the hostile-document test now runs there too. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…gh the key that keyed it Past its budget a walk returns the sealed root, and the rows open only at the walked path, below the path's node for json_tree_subtree and at the nodes it reaches for json_tree_subtree_at. The caller had to attach that path with `below` or `at`, and a caller who did not knew nothing of it until a document grew past the budget and the open failed with OpenedType. The owner chose option (c) from the #1325 review: make a path-less open impossible. The path is in the caller's hand when the key is derived, so the key now carries it (`JsonSubtreeKey::path`) and builds the wrapper: `key.open(rows) -> JsonSubtree` in stack-encrypt and `JsonTreeSubtree::from_key(&key, rows)` in eql-bindings. The key knows from its gates which walk the rows came from. `JsonSubtree::new`, `below` and `at` are gone, as are `JsonTreeSubtree::new`, `below`, `at` and its public fields; neither wrapper deserializes any more, since that was a path-less constructor too. The path is not part of the key's wire form. The test that the fallback refuses without a path is now a compile-time property. the_key_built_fallback_opens_to_what_the_walk_opens_to holds, for ten paths with and without gates, that the key-built fallback opens to what the walked rows open to, as a list and as one value; the gated rows are walked in the test as the SQL walks them. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…-and-projection Carries the base's merge of next up the stack, so the pull request diff reads against one merge base again. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
…-ordinal-chain Carries origin/next up the stack from #1250's branch through #1288's and #1325's, so this PR reads against one merge base. Content-empty: this branch already had next. Claude-Session: https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu
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. |
Summary
Stacked on #1288. The
json_treefamily (EQL v4's encrypted JSON document type, written by Stack Encrypt's JSON index) was benchmarked at 10,000 rows against the v3 SteVec type and nativejsonb. On a flat five-field document it is on par with v3; on a nested document of about 220 nodes the shipped SQL was one to four orders of magnitude behind nativejsonb. Three mechanisms accounted for all of it, and this PR fixes them. One is a wire change, which is why it must land before the family is released: a stored document now carries its own containment projection.In user terms: a containment query (
@>) on a nested document goes from about 70 ms to the low milliseconds and the GIN index builds in seconds rather than minutes per ten thousand rows; fetching a subtree by token goes from ten seconds to tens of milliseconds, or single digits when the server hands back the sealed root instead; and a range comparison keeps its index under a prepared statement.Changes
EQL SQL (
packages/eql/src/v3/json_tree/)types.sql: the stored document requiresq, its containment projection, validated as a group by the same validator the needle uses (moved ahead of the document validator so it is defined first).functions.sql:json_tree_query(doc)readsdoc -> 'q'and returnsjsonb(a domain cast would run the needle's recursive CHECK per recheck row); it is plpgsql atCOST 1000rather than an inlinable SQL function, because inlined tovalue -> 'q'the planner costed a sequential scan over the TOASTed nested table below the GIN bitmap scan and chose it (1.5 s for a one-row match at 10k rows, 17 s at 100k; 0.7 ms through the index).json_tree_groupis removed. The walk (json_tree_children,json_tree_child_count,json_tree_subtree,json_tree_subtree_at) builds one map of entries keyed by parent tag (json_tree_by_parent) and looks computed tags up in it, so a level costs its fan-out in hashes rather than fan-out times document size.json_tree_subtreeandjson_tree_subtree_attake abudget(default 64): past it, when the root is sealed, they return the root entry alone withd = 0. Every walk is bounded by the document's size as well as its depth: the fan-out it probes is capped at the entry count, and it returns at most as many rows as the document has entries, so a stored document whose unauthenticatedoorwis hostile cannot keep a query running.json_tree_path_query_firstisCOST 1000.stack-encrypt (
packages/stack-encrypt/src/json/)JsonDocumentwritesqon the wire (itsprojection()) and ignores it on the way in.JsonPath::selectevaluates a path over a value (keys, positions, wildcards).JsonSubtree::below(path)(forjson_tree_subtreerows) andat(path)(forjson_tree_subtree_atrows),path(),is_sealed_root(); opening a sealed-root fallback takes the path over the whole document and opens to what the walked rows open to (forbelow, the node as aValueand its children as aVec<Value>; forat, one value per node reached), and is refused without a path.eql-bindings (
packages/eql/crates/eql-bindings/src/v4/json_tree.rs)JsonTreeDocumentgainsq;JsonTreeSubtreeis now{ rows, path, below }withnew,belowandat.Tests and docs
packages/eql/tests/encryption/tests/json_tree.rs: the budget at, under and off; the fallback opens at the path for both walks, and as a list to the same elements as the walk; the budget is gated on the header's sealed-root flag; a wrong token still yields nothing; hostile documents (a looping chain, a huge ordinal) end under astatement_timeout.packages/stack-encrypt/tests/json_index.rs:qon the wire equals the needle of the whole document; the fallback's openings and refusals; each walk shape opened walked and fallen back gives the same value.@cipherstash/eql; the design document's "Implementation notes (2026-10-21, Phase 3)", with the report's numbers and commits;stack-encryptandeql-bindingsCHANGELOG entries.Verification
mise x --env test -- cargo nextest run -p stack-encrypt --all-features: 756 passed, 1 skipped (after the feat(stack-encrypt, eql): the JSON index and the json_tree EQL v4 domain family #1288 merges and the review fixes).cargo mutants --in-diffover the review fixes: 9 caught, 4 unviable, none missed.cargo test -p eql-bindings --features stack-encrypt,targetsinpackages/eql: all passed.EQL_TEST_DATABASE_URL=… mise run test:encryption:postgresinpackages/eqlagainst a fresh PostgreSQL 17 database: every suite passed, including the threejson_treetests (containment oracle againstjsonb @>with the GIN index shown inEXPLAIN, the walks, the budget and the fallback, and the hostile documents).mise run test:lint,test:schema,test:public_identifiers,test:symbol_order_v3,test:v4_isolation,test:installer_completeinpackages/eql: passed.docs:validatepassed its coverage and tags steps; itsdocumented-sqlstep needs the Docker database on port 7432, which was not running locally, so CI is what runs it here.-D warningsonstack-encrypt,eql-bindings(stack-encryptfeature) and the encryption test crate: clean.cargo fmt --check: clean.feat/json-tree-benches, commit 57abcf9,report/JSON_TREE_COMPARISON.md) at 10,000 and 100,000 rows against the v3 SteVec type and nativejsonb, every containment scenario checked againstjsonb @>over a plaintext twin before timing. Nested corpus (about 215 nodes per document), 10k rows, first run → this branch → native:@>deep object LIMIT 10, 68 ms → 3.2 ms → 0.36 ms;@>one array element, 12.7 ms → 0.70 ms → 0.10 ms; equality at a nested path, 12.9 ms → 0.50 ms → 0.10 ms; subtree of 6 entries, 292 ms → 2.8 ms; subtree of 244 entries, 10.2 s → 9.5 ms with the sealed root returned, 23 ms walked out;children, 7.1 ms → 1.6 ms; GIN build per 10k rows, 44 s → 5 s. Flat corpus against v3: equality at a path 33% faster at 10k and 82% at 100k, range at a path 87% faster at both tiers (the generic-plan fix), containment index a tenth the size;ORDER BYat a path and bare containment of a whole small document remain behind v3 (json_tree: the smaller performance findings from the first benchmarks #1324). The 100k tier holds the same shape throughout.Review
The review of this PR found nine things, posted above. The fixes are on this branch: 923364d (the fallback opens to the walk's shape), c1059d7 (the walks are bounded by the document's size), 014d25d (the documentation matches the cited report), and 31d5da4 (a test). Two need your decision: whether the budget should be on by default, and whether to verify
qagainstsvat write time. One question is open on the report: its table counts 122 rows for the walked$.orderssubtree, while its prose says 244.Related
Refs #1288 (the family), #1324 (the smaller findings left open).
Review notes
Start with
functions.sql: the projection section and the walk section are each small and self-contained. The wire change is one field;qis the selectors already insvregrouped, so it reveals nothing the entries do not. The fallback's client side isderive.rs(open_fallback) and the twoDecryptIntoimpls above it.https://claude.ai/code/session_01N7bCqa3ZVJMcEE99RzVqEu