Repository navigation
test(integration): prove PostgreSQL rejects plaintext on stale routes - #489
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation: automated /code-review medium, with findings cross-checked against this PR's existing feedback (cross-check-review-findings), at head 59f893b.
Critical / blocking: none.
Findings: none survived verification. The only existing feedback is a Codex status comment, with no findings to overlap.
Checked against head:
- The helpers the tests use (
connect,connect_with_tls,get_database_port,random_id,PROXY) exist incommon.rs. - The
bytesdependency is already declared, and the newfutures-utilis already locked at 0.3.31. - The SQLSTATE and message expectations match PostgreSQL's domain CHECK and
jsonbinput errors. - Each test also checks directly against PostgreSQL that the table is empty.
Not done: I did not compile or run the integration tests, which need PostgreSQL, EQL and Proxy. CI is the check for that.
Generated by Claude Code
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed head 02b9b606. The tests are good: each one first shows that Proxy chose the native route (for example, insert.params() == [INT8, JSONB]), and then it checks the table directly against PostgreSQL. But the CONTEXT.md entry says more than the tests show, and one test conflicts with #488.
Should fix
S1. The Stale route entry describes a guard for writes only, but it is written as a general rule.
CONTEXT.mdsays that neither a prepared statement nor a pinned transaction "sees a column that became encrypted", and that "a plaintext forwarded on a stale route is stopped by PostgreSQL". The domain CHECK runs only when a value is stored in the column.- The same stale route also applies to reads and predicates, and there PostgreSQL stops nothing:
- A prepared
SELECT secret FROM t, routed at Parse whilesecretwasjsonb, returns the EQL ciphertext to the client without decryption. - A prepared
SELECT … WHERE secret = $1sends the plaintext parameter to PostgreSQL. The comparison then runs withjsonbsemantics over the ciphertext and silently matches nothing.
- A prepared
- Neither case stores plaintext. But the first returns ciphertext undecrypted, and the second puts plaintext in a statement that the database can log.
- Fix: make the entry say that the domain CHECK guards writes. Record the read and predicate behaviour as a known limitation of stale routes, so that no one later reads this entry as "stale routes are safe".
S2. Merge hazard with #488.
- #488 makes Proxy refuse
COPY … FROMinto an encrypted column (CopyIntoEncryptedColumn), and it also acceptsCOPY … FROM STDINwithout the trailing;. - After #488 merges,
copy_from_stdin_into_encrypted_column_is_rejected_by_postgresfails: Proxy refuses the Parse, socopy_in(…).await.unwrap()panics. The two PRs merge cleanly as text, so only the integration run will show it. - These lines also become false: the
CONTEXT.mdsentence that the domain CHECK "is what stops plaintext in …COPY … FROM STDIN", the module doc ("Proxy also forwards some writes it never type checks"), and the comment about the trailing;. - Fix: if #488 merges first, rebase, then delete this test (
copy_from_stdin.rsin #488 covers the refusal) or change it to expect Proxy's error, and remove the COPY text. If this PR merges first, #488 must do the same.
Nits
- "an EQL column is a domain over
jsonb" is true for EQL v3 only. A legacyeql_v2_encryptedcolumn is a composite type, and the CHECK on it exists only wheneql_v2.add_encrypted_constraintwas run. Proxy refuses v2 columns when it knows about them, but a stale route does not know. Write "an EQL v3 column". connect_for_testcopies the TLS switch fromschema_change.rs. Both modules now have it, so it can move tocommon.
What I checked and found correct
- The mapping-disabled test correctly asserts Proxy's own refusal (
26000, "no longer match") and not a PostgreSQL error. Theprepare_typedwithJSONBis needed and is explained. - Both rejection paths are covered: a JSON-shaped plaintext gives
23514from the domain CHECK, and a non-JSON plaintext gives22P02. So the tests prove that the CHECK, notjsonbparsing, rejects the value. - The pinned-transaction test reads an unrelated table before the DDL, so it tests the pinned snapshot and not a cold start. It rolls back before it checks the table.
- CI: all 9 checks pass.
|
Thanks @freshtonic. Pushing one fast-forward commit now (no rewrite) that addresses this review:
I ran the four tests through a Proxy built from this branch against PostgreSQL 17 with EQL 3.0.4, and all four pass. Generated by Claude Code |
26116c3 to
8f8bbe1
Compare
|
Force-pushing now. The branch keeps the same base and the same tree for the existing commits, plus one new commit:
I ran the three remaining tests and Generated by Claude Code |
8f8bbe1 to
08e74bd
Compare
Move connect_for_test from schema_change.rs into common.rs so other integration tests can connect the same way as the Proxy under test.
08e74bd to
b769e2c
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Re-reviewed head b769e2c8, rebased onto main after #488 merged. (I reviewed 08e74bd7. b769e2c8 has the same tree on the same base, with the history squashed into two commits.) I read the full diff against main and the delta from 02b9b606. All the items from my last review are fixed:
- S1: the Stale route entry and the module doc now say that the domain CHECK guards writes only. The entry records stale reads (ciphertext returned undecrypted) and stale predicates (plaintext sent to PostgreSQL) as a known limitation.
- S2: the COPY test, the COPY line in the module doc, the comment about the trailing
;and the COPY sentence inCONTEXT.mdare removed.copy_from_stdin.rsfrom #488 now covers the refusal. Thefutures-utildependency also left this diff, because #488 already added it. - Nits: the text now says "EQL v3 column", and
connect_for_testmoved tocommon.rs, whereschema_change.rsalso uses it.
The three remaining tests did not change, apart from the shared connect_for_test. Both commits show as verified on GitHub.
Nit
lib.rs:32: the module doc still says "PostgreSQL rejecting plaintext that Proxy forwards on a stale or unchecked route". With the COPY case gone, every test is about a stale route, and the mapping-disabled test is about Proxy's own refusal. Something like "Guards against plaintext on a stale route" is more accurate.
CI
All 9 checks pass on b769e2c8, including the integration jobs on PostgreSQL 14 to 17 and the encrypted burn-in.
|
Thanks for the re-review. Pushing one fast-forward commit for the Generated by Claude Code |
Proxy routes a prepared statement at Parse and a transaction against the schema pinned when it started, so DDL committed by another connection can make a column encrypted after Proxy chose to forward a write to it. Add integration tests showing that PostgreSQL rejects the plaintext through the EQL domain's CHECK in both cases, and that nothing is stored. A JSON-shaped plaintext shows the CHECK, not jsonb parsing, does the rejecting. A statement prepared with mapping disabled is refused by Proxy at Bind once mapping is re-enabled, so it never reaches PostgreSQL. Record the stale route in the Proxy context, including the known limitation that the CHECK guards writes only, not reads or predicates.
72448e5 to
79ff83d
Compare
Requested by Toby
Proxy routes a prepared statement once, at Parse, and a transaction routes against the schema pinned when it started.
If another connection's committed DDL makes a column encrypted in between, Proxy forwards the write as written, with plaintext.
We decided not to change routing for this and to rely on PostgreSQL instead: an EQL v3 column is a domain over
jsonbwhose CHECK accepts only an EQL payload.This PR proves that guard with integration tests and documents it.
Changes
New
packages/cipherstash-proxy-integration/src/postgres_rejects_plaintext.rs, run through Proxy:INSERT ... VALUES ($1, $2)whilesecretisjsonb, B runsALTER TABLE ... ALTER COLUMN secret TYPE eql_v3_text_search, then A executes with plaintext.BEGINand reads an unrelated table, B makessecretencrypted, then A sends the INSERT as a simple Query.SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = true, executed after it is turned off again. Here Proxy, not PostgreSQL, is the guard: it refuses the Bind because the mapping setting changed since the Parse, so this test asserts Proxy's refusal.The other two assert PostgreSQL's own error (SQLSTATE and message) and, directly against PostgreSQL, that the table is empty.
Each writes a JSON-shaped plaintext (
{"name": "alice"}), which parses asjsonb, so the domain CHECK is what rejects it.packages/cipherstash-proxy/CONTEXT.mdgains a Stale route term that records the reliance on PostgreSQL's domain check and points at the tests.Notes
CS_DEVELOPMENT__ENABLE_MAPPING_ERRORSis set intests/mise.*.tomlbut not passed into the proxy containers intests/docker-compose.yml, and it defaults to off. None of these routes depends on it anyway.COPY ... FROMinto an encrypted column itself, andcopy_from_stdin.rscovers that.jsonb.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.
Generated by Claude Code