Repository navigation
fix(proxy): refuse COPY FROM into encrypted columns, check statements after COPY FROM STDIN - #488
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1695ceb40f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cipherstash-agent
left a comment
There was a problem hiding this comment.
Review attestation — automated review by Claude Code (/code-review medium, then company-skills:cross-check-review-findings) at head 1695ceb.
Result: no critical, blocking or new findings.
- Checked:
COPY ... FROMrouting infrontend.rs(route,check_copy_into), the batch ruleCopyWithEncryptedStatement,requires_schemaat all call sites, and theCOPY ... FROM STDINstatement splitting inparser.rs. - Candidate defects were traced and none had a reachable trigger. Existing PR feedback holds no findings (only a Codex status stub), so nothing is duplicated or contradicted.
- Not verified here: the integration tests (Rust
copy_in, Python psycopg). They were still running at review time, so treat CI as the gate.
Generated by Claude Code
… after COPY FROM STDIN Proxy does not encrypt COPY data, but forwarded a `COPY ... FROM` (a client stream, a file or a program) into an encrypted column, storing plaintext in it. The SQL parser also expects a `COPY ... FROM STDIN` to be followed by its copy data in the SQL: without a `;` it refused the statement, so plaintext bulk loads failed, and with one it read every later statement in the simple Query as copy data, so they were forwarded unchecked. - Parse each `;`-separated statement on its own when the SQL has a `COPY ... FROM STDIN`, so the statements after it are checked. - Route a `COPY ... FROM` through its own schema check against the batch view: refuse it when any column it fills is encrypted, when the table is unresolvable or ambiguous, or a named column is unknown, whatever the mapping errors setting. Without encrypted columns it is forwarded. - Refuse a simple Query that has a `COPY` and a statement Proxy maps, as the rewritten Query cannot carry the `COPY`. - Treat a `COPY ... FROM` as needing the schema (`requires_schema`), so it waits for in-flight DDL and fails closed after DDL or settings changes, like a type-checked statement.
5d40c36 to
e4cad15
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed head e4cad150. I found no fail-open path. The port also covers the points from my #483 review: schema-qualified COPY uses the same resolver as a write (public.encrypted and archive.encrypted are tested), and COPY (INSERT … RETURNING) TO was already refused on main.
Merge hazard with #489
- The two PRs merge cleanly as text, but not as behaviour.
copy_from_stdin_into_encrypted_column_is_rejected_by_postgresin #489 expects that Proxy forwardsCOPY … FROM STDIN;into an encrypted column and that PostgreSQL rejects it with23514. After this PR, Proxy refuses the Parse withCopyIntoEncryptedColumn. Thecopy_in(…).await.unwrap()in that test then panics.- The new Stale route entry in #489's
CONTEXT.mdalso says that the domain CHECK is what stops plaintext inCOPY … FROM STDIN. After this PR, that is false. - Whichever PR merges second must change that test to expect Proxy's refusal (or delete it, because
copy_from_stdin.rshere covers it), and must remove the COPY sentence fromCONTEXT.md.
Nits
- The empty-target refusal has no test. In
check_copy_into(frontend.rs:1426), I changedif targets.is_empty()toif false, and all COPY tests still passed. I think the branch cannot be reached for theCREATE TABLE … AScase in the comment: in the same Query, that DDL fails closed first, and across Queries a permanent table is loaded from the catalog with its columns. Keep it as a defence, but change the comment so that it does not nameASas the case it handles. - "Without encrypted columns every COPY is forwarded" (PR description, and
docs/errors.md). This is not fully true.requires_schemamakes a COPY count insimple_query_requires_fail_closed,settings_simple_query_requires_fail_closedandensure_schema_modelled, and those checks do not look athas_encrypted_columns. So with no encrypted columns,CREATE TABLE t AS SELECT 1 AS a; COPY t FROM STDINis now refused. This agrees with how INSERT behaves, so I think the behaviour is correct, but the docs should not say "every". is_copy_from_stdinalso matchesCOPY (SELECT * FROM stdin) TO STDOUT, because it findsFROMfollowed bySTDINanywhere in the statement. The only effect is that the Query is parsed one statement at a time, so aBEGIN ATOMICbody in the same Query fails to parse and is refused. It fails closed, so a comment is sufficient.
What I checked and found correct
- Statement splitting. The split is on tokens, so a
;in a literal, a quoted identifier, a dollar-quoted body or a comment does not split. PostgreSQL parses the whole Query string as SQL, so Proxy now sees the same statements that PostgreSQL runs. Bodies that contain several statements (BEGIN ATOMIC,CREATE RULE … DO (…; …)) fail to parse and are refused. - INHERITS, LIKE, CLONE and AS. In the same Query, these are encryption-changing DDL, so a later COPY gets
DependentStatementAfterDdl. A temporary table created with them is unmodelled, so the connection fails closed.CREATE TABLE … (LIKE …)is refused by theLIKEcheck. - The route order.
UNSAFE_DISABLE_MAPPINGstill forwards first, as intended. A COPY never reaches the type check or the fallback. A COPY in a mapped Query is refused before anything is encrypted (the encrypt counter is 0). - Extended protocol. A COPY sent with Parse is routed by the same function.
needs_mappingis now true for it, so the prepared-statement staleness check covers it. - Mutations. I removed each of these and its test failed: the
LIKEcheck, therequires_schemachange insimple_query_requires_fail_closed, and theCopyWithEncryptedStatementbatch rule. - CI: all 9 checks pass.
What I ran
cargo test -p cipherstash-proxy --lib: 475 of 476 passed. The one failure,config::tandem::tests::prometheus_config, is the environment race. Theconfig::tests with--test-threads=1: 37 of 37 passed.cargo test -p eql-mapper --lib: 225 passed.- The mutations above. I reverted all changes after the runs.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks @freshtonic. I pushed 1904b7a, a fast-forward with no rewrite, for the three nits:
On #489: I agreed with that PR's thread that whichever of the two merges second removes the COPY test and the COPY sentence in the Stale route entry. Generated by Claude Code |
Address the skill-driven review of the routing model. - Split the unchecked statement kind into CopyFrom, NestedWrite and Opaque, and route them as Frontend::route does since #488: check_copy_into refuses COPY ... FROM into an encrypted or unresolvable column, and a nested write is refused while encrypted columns are configured. The Bind settings check is gated by requires_schema. - A3 and F2 now cover only DO, CALL and SQL EXECUTE. New assumption A6: a nested write takes no parameters. A5 is documented as an over-approximation, which widens F1. Decision D4 keeps Bind and Execute as one step. - Parse is only of an unused name, a refused Parse leaves it unused, and an unknown Bind under UNSAFE_DISABLE_MAPPING is forwarded, as in Proxy. - New witnesses for Bind, Close, transaction end and the mapping setting, so every action has one only it can reach. New mutations M5 to M8. - Two-statement batches hold type-checked statements only, which keeps check.sh all at about 6 minutes. - Fill in the source-file correspondence table and the AGENTS.md pointer. No property changes. TLC: asBuilt 56,994 states, concurrentDdl 445,094, safety holds in both; all expected violations and M1 to M8 reproduce. Signed-off-by: Claude <noreply@anthropic.com>
Requested by Toby
Ports the
COPY ... FROMfix from #483 (release/2.2) tomain.main's frontend has a single per-statement route decision, a batch view and schema-qualified resolution, so this is a re-implementation rather than a cherry-pick.The problem
COPY ... FROM(client stream, file or program) into an encrypted column, so plaintext was stored in it.COPY ... FROM STDINto follow in the SQL. Without a trailing;the statement was refused as invalid SQL, so plaintext bulk loads (\copy, driver COPY APIs) failed. With a;every later statement in the simple Query was read as copy data and forwarded unchecked, soCOPY notes FROM STDIN; INSERT INTO users (email) VALUES ('plaintext')reached PostgreSQL in plaintext.The change
parser.rs): when the SQL has aCOPY ... FROM STDIN, parse each;-separated statement on its own, ending the COPY with the;the parser expects. Statements after it are now parsed and routed like any other. RESET normalisation still applies.frontend.rs): aCOPY ... FROMinto a table is checked against the same resolver a write would use (the batch view, including temporary tables). With encrypted columns configured it is refused when any column it fills is encrypted (or legacy EQL v2), the table is unresolvable or ambiguous, a named column is unknown, or the table's columns are unknown (CREATE TABLE ... (LIKE ...), which the parser misreads as a column namedLIKE). This holds whatever the mapping-errors setting is. Otherwise it is forwarded asNothingEncrypted. Without encrypted columns the COPY's table is not checked, though a COPY after encryption-changing DDL in the same Query is refused, as an INSERT would be.COPYand a statement Proxy maps is refused (CopyWithEncryptedStatement), as the rewritten Query cannot carry the COPY.eql_mapper::requires_schema:requires_type_checkwas also used as "this statement needs the schema" (waiting for in-flight DDL, failing closed after encryption-changing DDL or settings changes in a batch, and the prepared-statement staleness check). ACOPY ... FROMnow counts too.CopyIntoEncryptedColumn,CopyIntoUnknownColumn,CopyWithEncryptedStatement, documented indocs/errors.md. Unresolvable and ambiguous tables reuse the existing errors.Out of scope: COPY stream encryption and decryption, and
COPY ... TO STDOUTof encrypted columns, which returns ciphertext as before.COPY (INSERT ... RETURNING) TOwas already refused on main.One deliberate difference from #483: there, Proxy recorded DDL before PostgreSQL ran it, so the COPY check also checked the loaded table. On main, overlays are confirmed after execution and the batch view already handles
IF NOT EXISTS, so the COPY check uses the batch view on its own, the same as INSERT.Tests
Written first and seen failing:
COPY ... FROM STDINare statements.LIKEtables are refused, statements after a COPY are checked, COPY in a mapped Query is refused with nothing encrypted, and COPY after encryption-changing DDL in the same Query is refused.copy_in, and Python via psycopg): plaintext COPY round-trips, and COPY into an encrypted column is refused while the connection stays usable.cargo test -p cipherstash-proxy --lib(476) and-p eql-mapperpass locally, and clippy is clean. Integration tests run in CI.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