fix(proxy): refuse an unmappable statement that touches an encrypted column (CIP-3680) - #436
Draft
freshtonic wants to merge 3 commits into
Draft
Conversation
Checkpoint of in-progress work. Removes CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS and makes a type-check failure fatal when the statement touches an encrypted table, with a new eql_mapper::may_touch_eql_columns to make that distinction. Unverified: does not necessarily compile or pass. Committed so the work survives an interruption.
… column `CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS` defaulted to false, so when Proxy could not type check a statement it logged a warning and sent the statement to PostgreSQL unmapped. That is not a degraded answer, it is a wrong one, and none of the ways it goes wrong announce themselves: an unmapped SELECT returns the raw EQL payload instead of the decrypted value, an unmapped WHERE compares a plaintext literal against a jsonb payload so it matches nothing or the wrong rows, and an unmapped INSERT/UPDATE writes the value to disk unencrypted. The ticket's reproducer, `SELECT DISTINCT encrypted_bool FROM encrypted`, returned rows of ciphertext to the client with no error at all — `eql_v3_boolean` is storage-only and carries no equality term, so DISTINCT cannot be keyed on it. The flag is gone and there is no way to restore the old behaviour. Not fatal outright, though, which is the part worth explaining. The ticket flagged the risk that unmappable statements are not all encryption-related, and they are not: `requires_type_check` is purely syntactic, so every query is type checked whether or not encryption is involved, and the mapper's SQL coverage is narrower than PostgreSQL's. Measuring it settled the question. Running the existing suite against a Proxy with the flag forced on — which is exactly unconditional fatality — fails 253 of 349 tests, because tokio-postgres queries `pg_catalog.pg_type` to resolve the OID of an EQL v3 domain, so under EQL v3 an ordinary client issues an untypeable statement on the way to nearly every encrypted one. psql fares no better: `\d`, `\dt`, `\l`, `\dn` and `\df` all fail with `Table not found: pg_catalog.pg_*`. Rejecting any of that buys no security, because none of those statements contain encrypted data to get wrong. So the check is narrowed rather than removed. `may_touch_eql_columns` resolves every ObjectName in the statement against the schema and asks whether any names a table with an EQL column. A name that does not resolve is not evidence of encryption — the schema has never heard of it, so it has nothing to expose — which is what lets pg_catalog through. Over-collecting is the safe direction and the only direction this can err in: a function name is an ObjectName too. Qualified names are matched on their last identifier rather than passed to `resolve_table` whole. `resolve_table` only accepts a bare name and answers TableNotFound for `public.encrypted`, so taking it at face value would have waved through the exact shape the check exists to catch. STATEMENTS_UNMAPPABLE_TOTAL still counts every statement that failed to type check, so the metric stays comparable across the change; the ones now forwarded also increment STATEMENTS_PASSTHROUGH_TOTAL, like every other passthrough. Two of the new integration tests duplicated existing coverage and are dropped: `passthrough_invalid_statement` already pins that an unknown table is reported with PostgreSQL's own error, and `passthrough_select_with_cardinality` already pins that an untypeable plaintext-only query still runs. `unmappable_table_not_found` absorbs the first: it asserted only `is_err()`, and now asserts the error text, which is what actually distinguishes forwarding from refusing.
Two consequences of making an unmappable statement fatal, both found by running the suite rather than by reading the diff. `encrypted_column_not_defined_in_schema` asserted PostgreSQL's `column "..." does not exist`, which it only ever saw because the statement was forwarded. It names `encrypted`, so it is now refused by Proxy and the error is a mapping error. Worth being explicit about why this one is not carved out: a column that does not exist cannot leak, so forwarding looks safe — but that only holds if Proxy's schema is current, and it is reloaded on an interval. Between an ALTER TABLE adding an encrypted column and the next reload, "Proxy has not heard of this column" and "this column does not exist" are different claims, and acting on the first would write plaintext into the new column. The message is worse; the alternative is occasionally wrong. The second is not mine and is not fixed here, but it has to be recorded because removing the flag is what exposes it. On a connection that has already run a statement — every connection in a pool — a refusal reaches the client as tokio-postgres' `unexpected message from server` rather than the mapping error, and the connection is left broken. Frontend and backend are separate tasks writing to one unbounded channel, so an ErrorResponse synthesised on the frontend is queued at once and overtakes responses the server still owes for earlier messages. For `Close(s0) Sync | Parse(s1) Describe(s1) Sync` the client receives ErrorResponse, ReadyForQuery, CloseComplete, ReadyForQuery when it is waiting for CloseComplete first. The flag previously kept that path from being reached by default, so the ticket's premise — that Proxy already produces a good error in every one of these cases — holds only for a connection's first statement. The predecessor on this branch read the same failure and put it down to a connection-reuse artifact of the test; it is not, it is a Proxy defect, and probing it shows a fresh connection reports the error correctly while any reused one does not. So there are now two tests. The fresh-connection one keeps the strong assertion on the message. The reused-connection one asserts only that the statement is refused, which is the security property, and carries the byte order in its doc comment so the weak assertion is not mistaken for the intended behaviour. Fixing the ordering needs the frontend to withhold a synthetic error until the backend has drained, which is a change to the proxy's concurrency model.
|
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 |
freshtonic
marked this pull request as draft
July 30, 2026 02:16
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.
Removes
CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS. It defaulted to false — including in the proxy container — so when Proxy could not type check a statement it logged a warning and sent the statement to PostgreSQL unmapped.That is not a degraded answer, it is a wrong one, and none of the ways it goes wrong announce themselves:
SELECTreturns the raw EQL payload instead of the decrypted valueWHEREcompares a plaintext literal against a jsonb payload, so it matches nothing or the wrong rowsINSERT/UPDATEwrites the value to disk unencryptedThe ticket's reproducer,
SELECT DISTINCT encrypted_bool FROM encrypted, returned rows of ciphertext to the client with no error at all:eql_v3_booleanis storage-only and carries no equality term, soDISTINCTcannot be keyed on it.The flag is gone and there is no way to restore the old behaviour.
The narrowing, which is the part to review
Not fatal outright. The ticket flagged the risk that unmappable statements are not all encryption-related, and they are not —
requires_type_checkis purely syntactic, so every query is type checked whether or not encryption is involved, and the mapper's SQL coverage is narrower than PostgreSQL's.Measuring settled it. Forcing the flag on — which is exactly unconditional fatality — fails 253 of 349 tests, because tokio-postgres queries
pg_catalog.pg_typeto resolve the OID of an EQL v3 domain, so under EQL v3 an ordinary client issues an untypeable statement on the way to nearly every encrypted one.psqlfares no better:\d,\dt,\l,\dnand\dfall fail withTable not found: pg_catalog.pg_*. Rejecting any of that buys no security, because none of those statements contain encrypted data to get wrong.So the check is narrowed rather than removed.
may_touch_eql_columnsresolves everyObjectNamein the statement against the schema and asks whether any names a table with an EQL column. Two deliberate details:pg_catalogthrough. Over-collecting is the safe direction and the only direction this can err in — a function name is anObjectNametoo.resolve_tablewhole.resolve_tableaccepts only a bare name and answersTableNotFoundforpublic.encrypted, so taking it at face value would have waved through the exact shape the check exists to catch.STATEMENTS_UNMAPPABLE_TOTALstill counts every statement that failed to type check, so the metric stays comparable across the change; the ones now forwarded also incrementSTATEMENTS_PASSTHROUGH_TOTAL, like every other passthrough.Known residual gap
A statement touching no schema-known table is still forwarded unmapped, on the argument that it has no encrypted data in it. That holds for
pg_catalogintrospection, but it rests on the schema being complete and current — andschema_reload_intervaldefaults to 60 seconds, so there is a window afterCREATE TABLE … eql_v3_*where a genuinely encrypted table is not yet known and would be forwarded rather than refused. Whether that window is acceptable is a deployment-model call worth a reviewer's opinion.Testing
cargo fmtandclippy --all --all-targets -D warningsclean.docs/errors.mdandCHANGELOG.mdupdated; this is a breaking behavioural change.unmappable_table_not_foundabsorbs the first and now asserts the error text rather than onlyis_err(), which is what actually distinguishes forwarding from refusing.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.