Repository navigation
Conversation
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed head 671548fd, against release/2.2. The parser fix and the COPY FROM check are sound, and every mutation I tried made a test fail. CI passed on PostgreSQL 14 to 17. The Rust and Python COPY integration tests ran in CI and passed.
Fix in this PR, or create a linked follow-up before 2.2 ships
B1. COPY (INSERT/UPDATE … RETURNING …) TO STDOUT sends plaintext to an encrypted column.
check_copy_from returns early unless the COPY is to: false with a CopySource::Table (frontend.rs:989-999). Also, eql_mapper::requires_type_check does not include Statement::Copy, so Proxy never type checks the inner query. A probe test through Connection::encrypted() showed that Proxy forwards COPY (INSERT INTO users (email) VALUES ('alice@example.com') RETURNING id) TO STDOUT unchanged, with no error. COPY (UPDATE users SET email = '…' RETURNING id) TO STDOUT is the same. This PR does not cause the problem. But the CHANGELOG says that Proxy now refuses a COPY into an encrypted column, and this COPY writes to one. #484 found the same problem on main. Fix: when encrypted columns are configured, refuse a CopySource::Query whose body is an INSERT, UPDATE, DELETE or MERGE, or type check the inner query.
Should fix
S1. A COPY with a schema-qualified table name loses the schema, so a table on the same connection can hide the real table.
frontend.rs:1009 resolves only the last part of the name (table_name.0.last()), but schema_delta.rs keys overlays by the full ObjectName. So Proxy forwards CREATE TEMP TABLE users (id int, email text); COPY public.users FROM STDIN. This COPY has no column list, and it loads into public.users, which has the encrypted email. A probe test confirmed this. PostgreSQL makes it hard to exploit, because the copy text must parse as eql_v2_encrypted and pass the EQL CHECK constraint. So a leak needs crafted JSON, not an accidental CSV load.
The opposite case fails closed: Proxy refuses CREATE TABLE public.staging (…); COPY staging FROM STDIN as an unknown table. That disagrees with the docs, which say that Proxy knows a table created on the same connection.
Fix: when the name has a schema, require the overlay and the base schema to agree, or refuse the COPY.
S2. A merge with #481 can bring back the bypass.
#481 removes the loop that this PR uses at frontend.rs:507-519, which runs check_copy_from on the statements after a type-check failure. #481 returns Ok(None) there.
- If the merge keeps #481's version, Proxy forwards
SELECT * FROM unknown; COPY users FROM STDINinto an encrypted column. a_copy_after_a_statement_that_fails_type_checking_is_still_checkedcatches this. Keep that test, and keep a pass over the later statements.- After #481, each statement is written again with
Display, soCopyWithEncryptedStatement(frontend.rs:609-615) is no longer necessary. It is safe to keep, because it fails closed. If you remove it, add a test that shows the COPY survives the rewrite. - I checked that
Displaywrites a COPY with emptyvalueswithout a trailing;\n\., and thatWITH (FORMAT csv, DELIMITER ';', NULL '')andWITH CSV HEADERround-trip.
S3. The CHANGELOG and docs say that with no encryption config, Proxy forwards every COPY. That is not fully true.
Proxy still parses the Query, and it refuses as invalid SQL the COPY options that sqlparser 0.56 does not support: FROM STDIN WHERE id > 1, HEADER match, (ON_ERROR ignore). This fails closed, and it is not a regression, because the base branch refused every COPY … FROM STDIN without a ;. Please add a short note about it.
Nits
- N1.
CREATE TABLE staging (LIKE users)andCREATE TABLE staging AS SELECT * FROM usersgive an overlay with no columns. After either, Proxy forwardsCOPY staging FROM STDIN, but in PostgreSQLstaginghaseql_v2_encryptedcolumns.stagingis not in the encrypt config, so INSERT behaves the same. It would be stricter to treat an overlay with no columns as unknown when the COPY has no column list. - N2. Proxy does not check a COPY again at Bind or Execute when it was prepared while
UNSAFE_DISABLE_MAPPINGwas on and mapping was then turned back on (frontend.rs:870-879). All prepared statements work this way. - N3. In
tests.rs,a_simple_query_copy_into_an_encrypted_column_is_refusedhas the comment "The only error" aboveerrors_sent_to_client() == 0, aftertry_recvhas already taken the error. The assertion is correct, but the comment makes it hard to understand. COPY users TO STDOUTis forwarded and returns ciphertext that is not decrypted. This fails safe, but the docs do not say it. Please add a sentence.
What I checked and found correct
- The splitter (
parser.rs:115-170) splits tokens, not text. It handles correctly:;inside'…',E'\';',$$;$$,/* ; */and-- ;DELIMITER ';'- lowercase keywords and odd whitespace, and a leading comment
- more than one COPY in one Query
- a COPY at the end with no
; COPY (SELECT 1 FROM stdin) TO STDOUT
- Forwarding after a COPY. Proxy checks every statement after a COPY, and never forwards one as plaintext. When the parse succeeds, Proxy forwards the original bytes, so the split never changes what PostgreSQL gets.
- The encrypted-column check:
- A COPY with no column list is refused.
- Column names follow the case rules for quoted and unquoted identifiers.
- An unknown table is refused, and so is a table name that is in two schemas.
- Views and foreign tables are refused as unknown.
- FROM PROGRAM, or from a file, into an encrypted column is refused.
ALTER TABLE … ADD COLUMNkeeps the existing EQL kinds.- The simple and the extended protocols both refuse.
- The protocol after a refusal:
- For a refused Query, Proxy sends ErrorResponse and ReadyForQuery, and nothing goes to the server, so there is no stray CopyInResponse.
- For a refused Parse, Proxy sets
error_stateand drops all messages until Sync, including pipelined CopyData. - The Sync sent during a copy (CIP-5015) is covered.
- Errors and docs. The three errors are in
MappingErrorwith doc links. The anchors indocs/errors.mdmatch. The CHANGELOG has an entry.
What I ran
cargo test -p cipherstash-proxy --libon671548fd: 188 passed, including a temporary module of about 28 probe Queries. Clippy reports no problems.- 5 mutations, and each one made at least one test fail:
- removing
check_copy_fromafter a type-check failure - removing
CopyWithEncryptedStatement - making
contains_copy_from_stdinalways false - treating a COPY with no column list as having no columns
- forwarding a COPY into an unknown table
- removing
- CI run 39724849485:
copy_into_a_plaintext_table,copy_into_an_encrypted_column_is_refusedandtests/test_copy_from_stdin.pypassed.
|
Pushing one merge commit now: it merges release/2.2 (#481) and addresses B1, S1, S2, S3 and N1 from the review. It is a fast-forward with no history rewrite. A reply to each review item will follow once it is up. Generated by Claude Code |
Brings in #481 (per-statement settings and refused-message rejection) and keeps the COPY check on every path the merge touches. Review fixes (CIP-5101): - Refuse COPY of a query that writes (INSERT/UPDATE/DELETE, or a writing CTE) when encryption is configured, since Proxy does not encrypt the query of a COPY. - Check COPY statements that follow a statement that failed type checking with mapping errors off. - A schema-qualified COPY target checks both the edited and the loaded table, so a TEMP table cannot hide an encrypted one. - A COPY into a table with no known columns (such as CTAS) is refused. - Document COPY TO returning ciphertext and unsupported COPY options. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Review of commit The COPY handling introduces two correctness issues involving recovery after rejected queries and resolution of newly created schema-qualified tables. Overall impact: medium. The change affects bulk-loading workflows through both PostgreSQL protocols; the identified defects affect connections recovering from mixed encrypted/COPY queries and loads into schema-qualified tables created on the same connection. [P2] Reject mixed COPY queries before enqueueing execution statepackages/cipherstash-proxy/src/postgresql/frontend.rs:614 This error is returned after [P2] Preserve qualified names when resolving DDL overlayspackages/cipherstash-proxy/src/postgresql/frontend.rs:1009 Dropping the schema before consulting the resolver prevents it from finding same-connection DDL overlays, which Validation: read-only source review of the complete pinned diff (12 files, 676 additions, 16 deletions). Tests were not run. |
|
One more fast-forward push, no rewrite: a commit that refuses a COPY into a table created with Generated by Claude Code |
|
Thanks for the thorough review. Everything is addressed in 9fecfee (the merge with release/2.2, plus fixes) and b727fb3.
Also, a schema-qualified COPY now checks both the table created under that name and the loaded table, so a TEMP table of the same name cannot hide an encrypted one. Generated by Claude Code |
|
Pushing ed39baf as a fast-forward with no rewrite. It moves the check for a COPY mixed with an encrypted statement ahead of portal registration, and adds a regression test. Generated by Claude Code |
|
Both findings were against 671548f. Here is where each stands now:
Generated by Claude Code |
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟡 merge after changes
Change before merge: two paths let a false table overlay hide an encrypted table from the COPY check (see the comments on frontend.rs lines 554 and 1086). Also add two tests for refusal branches that no test covers (lines 1122 and 1486). The other comments can wait.
Other findings not posted as comments
- Optional:
writes()sits betweenproduces_ready_for_queryand its doc comment. So the line "Sync,QueryandFunctionCallare each answered with exactly oneReadyForQuery" now documentswrites(). Move that line back abovefn produces_ready_for_query(frontend.rsaround line 1479). - Optional: no test refuses a
COPYinto a table name that two schemas have. SupposeSchema::resolve_tableineql-mapperlater returns the first match. Then a plaintext table could hide an encrypted one. Use a schema with twouserstables, one plaintext and one encrypted. ExpectCopyIntoUnknownTable. - Optional: no test forwards a
COPYinto a table that has a quoted"like"column. So no test checks thequote_style.is_none()check (frontend.rsaround line 1104). - A test for the
check_for_schema_changecall in the fallback loop is not needed if the first comment is applied, because that fix removes the call.
The test code in comments 2, 3, 5 and 6 uses helpers from tests.rs: Connection::encrypted, query, refused and forwarded_unchanged. Each comment says which helpers it uses.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 6 found, 4 posted |
| claude | claude-opus-5-5 | rust | 3 found, 2 posted |
| codex | gpt-5.6-terra | test-gap | 1 found, 1 posted |
| codex | gpt-5.6-terra | rust | 1 found, 1 posted |
Synthesis: claude-sonnet-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-sonnet-5-5 read every comment as a new reader would. 4 comment(s) had a problem that stopped the reader acting; it rewrote 4. It also rewrote the review body.
Stack: not part of a stack.
Context loaded: the description, 0 linked issue(s) and 7 discussion entries.
|
Pushing 8a9546a as a fast-forward with no rewrite. It addresses the latest review: every COPY now checks the loaded table too, nothing after a failed statement is recorded, and it adds the missing tests. Generated by Claude Code |
|
The review's three findings not posted as comments are also in 8a9546a:
Generated by Claude Code |
…into encrypted columns (CIP-5101) The SQL parser expects the copy data of a COPY ... FROM STDIN to follow it in the SQL, as in a psql script. It refused the statement without a `;` after STDIN, so psql's \copy and driver COPY APIs could not load any table through Proxy. With a `;`, it read the rest of the Query as copy data, so a statement after the COPY was forwarded unchecked, including a plaintext INSERT into an encrypted column. A Query or Parse with a COPY ... FROM STDIN is now parsed one statement at a time, with each COPY ending at its `;`. A COPY ... FROM into an encrypted column is refused with MappingError::CopyIntoEncryptedColumn, as Proxy does not encrypt copy data. A table or column Proxy does not know is handled as a statement that fails type checking. A Query that Proxy rewrites to encrypt cannot keep a COPY, so a COPY in such a Query is refused with MappingError::CopyWithEncryptedStatement. fix(proxy): refuse a COPY into a table Proxy cannot check (CIP-5101) A COPY into a table or column missing from the loaded schema, or into a table name more than one schema has, was forwarded unless mapping errors were enabled, so it could send plaintext to an encrypted column. With encrypted columns configured it is now refused with MappingError::CopyIntoUnknownTable. A table created earlier on the connection, or earlier in the same Query, is known. A COPY after a statement that fails type checking in the same Query is now checked too, as the Query is forwarded unchanged. fix(proxy): name a COPY's encrypted column without quotes (CIP-5101) Proxy loads column names from the database as quoted identifiers, so the CopyIntoEncryptedColumn error named the column '"encrypted_text"'. The test harness now quotes column names as the schema loader does. fix(proxy): refuse a COPY into a table created with LIKE (CIP-5101) The parser reads the LIKE clause of CREATE TABLE t (LIKE u) as a column named LIKE, so Proxy does not know the columns the table copies and forwarded a COPY into it. PostgreSQL reserves LIKE, so an unquoted column of that name is always the clause, and such a table is now unknown. fix(proxy): refuse a mixed COPY query before adding its portal (CIP-5101) A Query with a COPY and an encrypted statement was refused after its portal and execution were queued. Since the merge with #481 the forwarded rejection drains them, so the next Query was not affected, but the check now runs first, as the other refusals do. A test pins that the next Query is not decrypted. fix(proxy): check the loaded table for every COPY target (CIP-5101) Proxy records a table created on the connection before PostgreSQL runs the CREATE, which may then not create it (IF NOT EXISTS, a refused Query, a rollback). A COPY trusted that record for a name without a schema, so a plaintext record could hide an encrypted table. A COPY is now refused if either the recorded or the loaded table has an encrypted column it fills. After a statement that fails type checking, the CREATE statements after it are no longer recorded, as PostgreSQL does not run them. Also adds tests for an unknown column of a known table, a parenthesized writing query, a name two schemas have, a quoted "like" column, COPY with mapping disabled, COPY of a table TO STDOUT, and two COPY FROM STDIN statements in one Query.
8a9546a to
cab9b97
Compare
Requested by Toby
Fixes CIP-5101 on
release/2.2: aCOPY ... FROM STDINwas refused because Proxy's SQL parser rejected it.Cause
The SQL parser expects the copy data of a
COPY ... FROM STDINto follow it in the SQL, as in apsqlscript.Without a
;afterSTDINit refuses the statement, sopsql's\copyand driverCOPYAPIs could not load any table through Proxy.With a
;, it reads the rest of the Query as copy data, so a statement after theCOPYwas forwarded unchecked.For example,
COPY users (id) FROM STDIN; INSERT INTO users (email) VALUES ('alice@example.com')sent the plaintextINSERTto an encrypted column.Changes
SqlParserparses a Query or Parse that has aCOPY ... FROM STDINone statement at a time, with eachCOPYending at its;. Other SQL is parsed as before, from one tokenization.COPY ... FROM(STDIN, file or program) into an encrypted column is refused withMappingError::CopyIntoEncryptedColumn, which names the column. ACOPYwith no column list fills every column.COPYinto a table or column Proxy has not loaded, or into a table name more than one schema has, is refused withMappingError::CopyIntoUnknownTable. This is stricter than a failed type check, which is forwarded unless mapping errors are enabled. A table created earlier on the connection, or earlier in the same Query, is known. With no encryption config, everyCOPYis forwarded.COPYwould be dropped. ACOPYin such a Query is refused withMappingError::CopyWithEncryptedStatement.docs/errors.md, and there is a CHANGELOG entry.Tests
postgresql::tests::copy_from_stdincover the simple and extended protocols:COPYis forwarded with its copy data;COPYinto an encrypted column, an unknown table, or from a file is refused;COPYinto only the plaintext columns of an encrypted table, or into a table created in the same Query, is forwarded;COPYare not forwarded as plaintext.Connection::encrypted()andforwarded(), ported from fix(proxy): keep CIPHERSTASH settings in step with PostgreSQL transactions (2.2 backport, CIP-5073) #481 to ease the merge. Its test encryption service now returns stub ciphertexts, so a rewrite can be tested.COPY ... FROM STDIN.copy_from_stdin.rsuses tokio-postgrescopy_in, which sends the extended protocol;tests/python/tests/test_copy_from_stdin.pyuses psycopgcursor.copy, which sends a simple Query.Notes for review
release/2.2, not on fix(proxy): keep CIPHERSTASH settings in step with PostgreSQL transactions (2.2 backport, CIP-5073) #481, because the bug predates fix(proxy): keep CIPHERSTASH settings in step with PostgreSQL transactions (2.2 backport, CIP-5073) #481. Expect conflicts with fix(proxy): keep CIPHERSTASH settings in step with PostgreSQL transactions (2.2 backport, CIP-5073) #481 infrontend.rsandpostgresql/tests.rs.mainuses the same parser (sqltk-parser0.56), so it probably has the same problem. I haven't checked it.SETbefore an encryptedINSERT. That is whyCopyWithEncryptedStatementexists.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