[Andrei Kvapil (kvaps)]

Problem

A SELECT whose result set is assembled by a join left the read path without a decrypt plan, and every PII column in it reached Keycloak as a raw $KKP$ envelope.

The read planner keyed its plan on Analysis.Table, the single relation behind a statement. A join has none — the first FROM item is the join expression rather than a range variable — so Table stayed empty, PlanRead returned an empty plan, and the row went out untouched.

Keycloak takes that path for getGroupMembersStream, the admin REST group-members listing, which selects the full USER_ENTITY row through USER_GROUP_MEMBERSHIP join USER_ENTITY. The proxy was already reporting it on a live deployment: kkp_ciphertext_passthrough_total{reason="empty-plan"} and kkp_unrecognized_pii_sql_total rising together, with one ciphertext passed through undecrypted warning per member row. Ciphertext handed to Keycloak this way does not stay confined to the request that leaked it — it is what the user cache stores, and later reads serve it back.

The stored data is unaffected: this is a read-path gap, not a write-path one, and no double-encrypted rows were produced.

Change

Analysis now carries FromTables, every base relation a statement reads, with join trees, sub-selects, set-operation arms and WITH (CTE) references walked. The read planner resolves each result column against that set instead of a single table.

A column that two joined relations both configure can't be attributed to one table by name, so it is left as a raw envelope rather than decrypted under the wrong relation (a wrong key fails the authentication tag and would error the whole query). Such columns are not dropped silently: they are carried through the read plan (ReadPlan.Ambiguous) and counted and warned in DecryptDataRow as kkp_ciphertext_passthrough_total{reason="ambiguous-column"}. Resolving them exactly — via RowDescription's TableOID, which would let them decrypt rather than pass through — is tracked as a follow-up in #5.

Table keeps its previous meaning, so the write path is unchanged — parameter encryption and the fail-loud rules still decline to act on a statement whose target relation is ambiguous. As an added guard, encryptParam now refuses a parameter that already parses as a $KKP$ envelope, so a cached ciphertext written back can't silently become a double-encrypted row.

Conformance gate

The runtime-SQL conformance test keyed off the table recorded at capture time, which meant a statement whose table the analyser had already failed to extract was skipped by the very test meant to catch that failure. The group-members join was captured with an empty table column and never checked.

The gate now derives the expected relation set from an independent parse-tree walk of the statement's FROM clauses — a second implementation, not a text scan — and every PII relation it finds must appear in the statement's FromTables. Deriving from the parse tree rather than scanning text means it sees comma joins, excludes WHERE sub-queries, and is not fooled by string literals. The group-members join is added to the golden corpus (the FEDERATED_IDENTITY lookup was already present), and a uniqueness test pins the corpus to what its generator can reproduce.

Testing

go test ./... (incl. -race) and golangci-lint run pass.

New regression coverage, all of it red before the fix:

• wire-level replay of the group-members statement, in both the described and the warm server-prepared pgjdbc flows, asserting every PII column decrypts and the passthrough counter stays flat;
• a wire replay of a join with one ambiguous and one unambiguous PII column, asserting the ambiguous one stays a raw envelope and kkp_ciphertext_passthrough_total{reason="ambiguous-column"} increments (fails without the fix);
• analyser coverage for joins, nested joins, comma joins, sub-selects, set operations, self-joins and CTEs (incl. recursive-CTE termination);
• planner coverage for multi-relation resolution and for the ambiguous-column case;
• a double-encryption guard test and a corpus-uniqueness test.

All test data is synthetic.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f4557470-148d-485a-9f9e-24708d4815a6

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Kirill Ilin (sircthulhu)]

Verified on 711e1f9. go test ./... and golangci-lint run pass — that claim holds.

The core idea is right: Analysis.Table was never going to describe a join, and threading the relation set through to PlanRead is the correct shape. The wire-level replay in internal/wire/joined_read_test.go is good — it covers both the described and the warm server-prepared pgjdbc flow, and asserts on the incident counter rather than just the output bytes.

Two things block: the ambiguous-column branch reintroduces the same failure mode in a quieter form, and the new conformance gate does not gate what its comments say it gates.

1. The ambiguous-column path is a silent ciphertext leak

internal/rewrite/read.go:

// Leave those alone — the read-path leak detector flags them instead.

It does not. Both detectors gate on the plan being absent or empty — transform.go:268 (flagCiphertextLeak(dr, "empty-plan") under if plan.IsEmpty()) and session.go:312 (if (plan == nil || plan.IsEmpty()) && piiTouchedButNotPlanned(...)). In a join where at least one other column resolves, the plan is non-empty, so the skipped column leaves the proxy as a raw envelope with nothing counted and nothing logged.

Reproduced with a throwaway wire test — a FieldSet configuring EMAIL on two relations and FIRST_NAME on one, over FED_USER_ENTITY join USER_ENTITY:

PROBE FromTables=[fed_user_entity user_entity]
PROBE email out      = "$KKP$1.n.1.AcQpkTxpJgtqbLG6U2SpiggSdI9m1_K65pY4VVWSV1oh..."
PROBE first_name out = "Ivy"
PROBE passthrough[empty-plan] = 0
PROBE passthrough[no-plan]    = 0
PROBE passthrough[no-portal]  = 0
PROBE unrecognized delta      = 0

Zero metrics, zero warnings — quieter than the bug being fixed, which at least rang empty-plan loudly enough to be noticed in the first place. TestPlanReadAmbiguousColumnSkipped only exercises the single-column case, where the plan is empty and the detector does fire, which is why the suite doesn't see the hole.

Not reachable under config.Default() today, since the default column sets are disjoint per table. It becomes reachable with any FieldSet covering the federated tables — which piiTables and piiTouchedButNotPlanned both treat as in scope, and which this PR's own test constructs.

Please carry the unresolved field through the plan (or return the ambiguous columns alongside it) so DecryptDataRow/learnColumns can count and WARN, and assert that in a test: a join with one ambiguous and one unambiguous PII column must increment kkp_ciphertext_passthrough_total. The same claim in the PR description ("the existing passthrough detector flags those rows instead") needs correcting too.

2. The added FEDERATED_IDENTITY corpus line is a byte-identical duplicate

testdata/keycloak/26.0.0/runtime-sql.txt is now 70 lines, 69 unique:

2  SELECT  federated_identity  select fie1_0.IDENTITY_PROVIDER,fie1_0.USER_ID,...

That line already existed verbatim, so "Both reads the proxy warned about … are added to the golden corpus" is only half true — nothing new is covered by it. And since cmd/capture-keycloak-sql accumulates into map[stmt]struct{} and writes the sorted set, the golden file can no longer be reproduced by its own generator; the next capture will silently drop the line. Please drop the duplicate, and consider a corpus test asserting line uniqueness — the generator guarantees it, so it's cheap.

3. The new gate is blind to comma joins

readsFrom splits on whitespace and only inspects the token after a literal from/join, so a comma join hides the second relation inside r,USER_ENTITY:

PROBE readsFrom("select r.NAME,u.EMAIL from REALM r,USER_ENTITY u where u.REALM_ID=r.ID", "user_entity") = false
PROBE Table="realm"  FromTables=[realm user_entity]

The analyser gets it right; the gate doesn't see it. This is a false negative, so it weakens the gate rather than breaking the build — but the gate exists specifically to close this class of blind spot, and Hibernate does emit comma joins for Keycloak (from IDENTITY_PROVIDER ipe1_0,IDENTITY_PROVIDER ipe2_0 is already in the corpus, caught only because it happens to be a self-join). Split on commas as well, or drop the text scan and derive the expected relation set from the parse tree.

4. readsFrom's documented WHERE-subquery rule is not implemented

// A mention inside a WHERE sub-query does not count: those rows never reach the client, so they need no decrypt plan.

The code text-scans the whole statement, so it does count:

PROBE readsFrom("select g.NAME from KEYCLOAK_GROUP g where g.ID in (select ua.USER_ID from USER_ATTRIBUTE ua ...)", "user_attribute") = true
PROBE FromTables=[keycloak_group]

No corpus statement has that shape today, so the test passes by luck. When one arrives the gate fails spuriously, and the tempting fix at that point — teaching collectSelectTables to walk WHERE sub-selects — is the wrong one: it would attribute result columns to relations that contribute none, i.e. decrypt under the wrong AAD. Excluding WHERE sub-selects from FromTables is the right call; please make readsFrom implement the rule it documents, with a test.

5. CTEs are not walked, contradicting the FromTables doc

collectSelectTables never touches sel.GetWithClause(), and a CTE reference in FROM parses as a RangeVar, so the alias is recorded as a base relation:

PROBE Table="m"  FromTables=[m]
  for: with m as (select u.EMAIL from USER_ENTITY u) select m.EMAIL from m

The field doc promises "every base relation the statement reads, with join trees, sub-selects and set-operation arms walked". For a CTE that is untrue, and reporting m as a relation is actively misleading. Today's outcome is an empty plan and a loud empty-plan passthrough rather than a silent leak, so this is a contract/doc defect rather than a leak — but it is the same shape-of-FROM hazard class this PR exists to close, one WITH away. Walk the with-clause and resolve CTE references, or at minimum stop reporting a CTE alias as a relation.

6. Post-incident remediation

The PR description notes that leaked ciphertext "is what the user cache stores, and later reads serve it back". Given that:

• encryptParam (transform.go:223) encrypts whatever it is handed with no envelope check, so a cached $KKP$… value written back on the next admin save produces a double-encrypted row — which is exactly why kkp_double_encrypted_total and its WARN exist.
• "The stored data is unaffected … no double-encrypted rows were produced" therefore holds only for a deployment where no user whose entity was cached from a poisoned read was subsequently written. Nothing in the diff establishes it.
• docs/runbook.md has the right mechanism (clear-user-cache, section 4) but only under "Recovery from out-of-band SQL changes". This fix needs a note tying the two together: flush the Keycloak user cache after rollout, and check kkp_double_encrypted_total before declaring the incident closed.

A guard in encryptParam that refuses (or escapes) a parameter already parsing as an envelope would be cheap and would make the write path fail loud instead of compounding a read-path leak.

7. Worth an issue: RowDescription already carries the authoritative answer

OnRowDescription (session.go:265) keeps only f.Name and discards TableOID and TableAttributeNumber — exactly the owning-relation identity columnOwner is currently guessing at by name. Resolving OID to relname once against pg_class and caching it would make attribution exact, dissolve finding 1, and remove the "column claimed by two relations" concept entirely. Bigger than this PR should take on, but it's the right direction.

8. Smaller items

• targetTables' comment promises "a RETURNING result set decrypts like any other read", but UPDATE … FROM other yields FromTables=[keycloak_group] only (verified). Keycloak doesn't emit that shape, so no code change needed — the comment should stop promising it.
• Analysis.Table's new doc says "Empty if no single table applies — notably when the FROM clause is a join". For a comma join it is not empty and names one of two relations (Table="realm" above). Please reword.
• The header comment of runtime_corpus_test.go still enumerates only the original assertions; TestRuntimeCorpusFromTableCoverage is missing from the list.
• observe.UnrecognizedPIISQL.Add(0) in joined_read_test.go is a no-op on a plain Counter — drop it.
• TestJoinedSelectLeavesNoCiphertextPassthrough mutates process-global Prometheus state and deliberately omits t.Parallel(). That is safe today only because Go serializes non-parallel top-level tests against paused parallel ones, and because no other test in internal/wire touches those collectors. The invariant is invisible — worth a comment so nobody adds t.Parallel() later.
• readsFrom scans string literals too: select 'copied from user_entity today' … from REALM r returns true. Cosmetic against today's corpus, but another argument for the parse tree.

[Andrei Kvapil (kvaps)]

Thanks for the thorough review — all addressed in 3820b6bd:

1. Ambiguous-column leak — ambiguous PII columns are now carried through the read plan (ReadPlan.Ambiguous) and counted + WARNed in DecryptDataRow as kkp_ciphertext_passthrough_total{reason="ambiguous-column"}; added a wire regression test (one ambiguous + one unambiguous PII column over a join) that fails without the fix. Fixed the false columnOwner comment and the PR description.
2. Dropped the duplicate FEDERATED_IDENTITY corpus line; added a uniqueness test on the generator's dedup key.
   3 & 4 & string-literals — replaced the whitespace readsFrom scan with an independent parse-tree relation oracle, so comma joins are seen, WHERE sub-queries are excluded, and string literals no longer match.
3. CTE — the FROM walk now resolves WITH references to their base relations (with a recursive-CTE cycle guard) instead of reporting the alias; tests added.
4. Added an encryptParam guard that refuses an already-$KKP$ envelope (fail loud instead of a double-encrypted row), plus a runbook section (flush user cache + check kkp_double_encrypted_total).
5. Filed as Resolve column ownership from RowDescription TableOID instead of by-name guessing #5 — resolving ownership from RowDescription's TableOID is a bigger change and would let ambiguous columns actually decrypt rather than pass through; agreed it's the right direction.

go test ./... -race + golangci-lint run green. PR description updated. Ready for another look.

[Kirill Ilin (sircthulhu)]

Re-reviewed 3820b6b. go test ./... -race, golangci-lint run (0 issues) and go vet clean here as well. Every finding from the previous round is genuinely closed — verified with probes rather than by reading the diff:

• Ambiguous-column leak — the plan carries Ambiguous, and a join with one ambiguous plus one resolvable PII column gives kkp_ciphertext_passthrough_total{reason="ambiguous-column"} = 1 with the unambiguous column decrypted and the ambiguous one left as a raw envelope. The regression test fails without the fix.
• Corpus duplicate — gone, 69/69 unique, and TestRuntimeCorpusUnique keys on the same tuple the generator dedups on.
• Comma joins, WHERE sub-queries, string literals — all three confirmed fixed by the parse-tree oracle.
• CTEs — FromTables resolves WITH references, including WITH RECURSIVE (cycle guard works), transitive CTEs, and a CTE shadowing a real table name.
• Double-encryption guard — crypto.Parse's strictness makes a false positive on real input effectively impossible, and I confirmed the guard does not touch cmd/backfill: it connects to PostgreSQL directly and calls cipher.Encrypt itself, so DEK rotation is unaffected. That was the main risk I looked for and it isn't there.
• Runbook, comments, nits — all addressed.

Merging this. One thing for a follow-up, deliberately not blocking:

The leak detector is a case list, not a post-condition

flagCiphertextLeak only ever runs when the plan is absent or empty, so a column that no relation configures as PII yet still holds an envelope is invisible whenever anything else in the row resolves. That predates this PR for the ordinary single-table read; this PR extends it to one more case, because IsEmpty() is now false for an ambiguous-only plan and flagAmbiguousPassthrough inspects only the ambiguous indices:

FED_USER_ENTITY join USER_ENTITY, EMAIL claimed by both, email NULL,
first_name holding a stale envelope (no relation configures FIRST_NAME):

  with this PR (ambiguous-only plan):
    first_name out = "$KKP$1.n.1.ATCGsfk12_VBZAcG66fJT5t_DbTKWCbfTnpRM7Q89AWx1a-7"
    passthrough[empty-plan] = 0  [no-plan] = 0  [no-portal] = 0  [ambiguous-column] = 0

  same row, nothing configured (empty plan):
    passthrough[empty-plan] = 1

That state is reachable without anything exotic: the shipped field profiles (-fields email-only|default) mean narrowing default to email-only leaves FIRST_NAME/LAST_NAME encrypted but unconfigured.

The cheap fix covers both the old gap and the new case: scan the outgoing row for any remaining envelope after the decrypt loop and count it there. The detector then becomes a post-condition — no row leaves carrying ciphertext — instead of an enumeration of the gaps found so far, which is the shape that let the original joined-read leak go uncounted in the first place. Each reason label stays useful for attribution; it just stops being the thing that decides whether the row is checked at all.