Skip to content

refactor(proxy): route each statement through one refuse-by-default decision - #484

Merged
tobyhede merged 1 commit into
mainfrom
claude/project-thread-hqgv43
Oct 23, 2026
Merged

tobyhede merged 1 commit into
mainfrom
claude/project-thread-hqgv43

Conversation

@tobyhede

@tobyhede tobyhede commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Requested by Toby

Stacked on the CIP-5102 branch (claude/project-thread-2yjn9s). Also closes CIP-5109. Retarget to main once that merges.

Why

The recent chain of fail-open fixes (CIP-5101, CIP-5102, #482) shares a cause: forwarding a statement as written was the default, and refusing was opt-in.
Query and Parse each copied the same ladder of checks, and anything that fell through was forwarded.
Error::must_fail_closed was a deny-list, so every new refusal had to be added to it or it silently became a passthrough.
One new rule (CIP-5102) touched 8 to 10 call sites.

What changes

  • Frontend::route is the only place that decides what happens to a statement: Route::Map, Route::Forward(Forward) with a named reason (MappingDisabled, NotTypeChecked, NothingEncrypted, Fallback), or Err to refuse.
  • Error::must_fail_closed (deny-list) is replaced by Error::is_coverage_gap (allowlist): only StatementCouldNotBeTypeChecked and Internal may fall back. Any error added later fails closed without being listed.
  • The fallback also requires that the statement can't write to an encrypted column: with encrypted columns configured, a write (eql_mapper::writes_data) that fails type checking is refused. Reads keep the fallback so catalog queries from drivers, ORMs and psql keep working (Toby's call).
  • A simple Query routes every statement before acting on any. A statement after a fallback is now checked instead of being forwarded unexamined, and the rule that a fallback can't carry a mapped statement lives in one place, replacing any_requires_mapping and the mapped flag.

Behaviour changes

With encrypted columns configured and mapping errors off (the production default), these were forwarded with plaintext values and are now refused:

  • MERGE into a resolvable table.

  • SQL PREPARE of an INSERT/UPDATE/DELETE, as a simple Query or a Parse.

  • Such a write after a statement that falls back in the same simple Query.

  • INSERT INTO encrypted (id, encrypted_unconfigured) ... (an unknown column) is now refused by Proxy instead of reaching PostgreSQL; the integration test is updated.

Still forwarded: reads that can't be type checked.
A write never falls back (CIP-5109).
Instead, Proxy's schema now covers the tables the old exceptions were for:

  • Native temporary tables live in a per-connection pg_temp layer that resolves before the catalog.
    They are added once PostgreSQL confirms the CREATE, and removed by DROP, DISCARD TEMP/ALL, ON COMMIT DROP at commit, rollback, and rollback to a savepoint.
    A bare name that is both a temporary and a permanent table is ambiguous, so a write to it is refused with a new Ambiguous table error that says to qualify it.
  • In a simple Query, each statement is type checked against the schema plus the encryption-neutral DDL before it (the batch view), on a copy.
    CREATE TABLE (including CREATE TABLE pg_temp.name), ALTER TABLE ... ADD COLUMN and RENAME followed by a write in the same batch are now forwarded; a write placed before its CREATE TEMPORARY TABLE is refused.
    CREATE TABLE IF NOT EXISTS is not modelled, because the table may already exist with encrypted columns Proxy has not loaded, so a write after it is refused unless an earlier statement in the batch created the table.
  • INSERT INTO encrypted ... SELECT ... FROM <table created earlier in the query> is now encrypted rather than refused.

Tests

New unit tests in frontend.rs (unresolvable_table_tests), written first and confirmed failing on the base branch:

  • simple_write_that_fails_type_checking_is_refused
  • extended_write_that_fails_type_checking_is_refused
  • statements_after_a_passthrough_fallback_are_checked
  • write_that_fails_type_checking_is_forwarded_without_encrypted_columns
  • extended_read_on_an_unresolvable_table_is_forwarded

CIP-5109 adds resolver tests in eql-mapper and temporary-table lifecycle and batch-view tests in the schema middleware.

All 457 cipherstash-proxy lib tests and 225 eql-mapper tests pass, and clippy is clean with -D warnings.

Not in this PR

  • Bind still reads Option<Statement> from the prepared statement. Since writes that fail type checking are now refused at Parse, a None there can only come from a read.
  • COPY, SQL EXECUTE and CALL are still Forward::NotTypeChecked, except that a write nested in one, such as COPY (INSERT ... RETURNING ...) TO STDOUT, is now refused while any column is encrypted. DO is refused because it can't be parsed.

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

@tobyhede tobyhede self-assigned this Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Supported pooler compatibility (PostgreSQL 17) is failing on the Go pgx suite's direct baseline (GOLANG | direct | FAIL: Go gate failed). The job log doesn't include the Go test output, and this container has no Docker daemon, so I couldn't reproduce it end to end.

The likely cause comes from the base branch (CIP-5102), not from this PR's commit. TestPgx… in tests/integration/golang/pgx_test.go runs CREATE TEMPORARY TABLE t (…); INSERT INTO t … in one simple query. Proxy deliberately doesn't model native temporary tables (proxy/schema/middleware.rs, is_schema_ddl), so t is unresolvable. The base branch's MappingError::UnresolvableTable guard therefore refuses the INSERT whenever encrypted columns are configured. A unit-test probe with the encrypted harness confirms the refusal on both the base and this branch:

Statement refers to table 't', which is not in the database schema Proxy has loaded, …

This PR doesn't change that outcome; the same write would be refused by the base's guard. The fix belongs with CIP-5102. One option is to have the schema middleware remember the native temporary tables created on the connection, so writes to them count as native rather than unresolvable. I've raised it there, and I'm not re-running the job because the failure is deterministic.


Generated by Claude Code

@tobyhede
tobyhede force-pushed the claude/project-thread-hqgv43 branch from 9b0dc9a to d8691ec Compare October 22, 2026 06:57
@tobyhede
tobyhede marked this pull request as ready for review October 22, 2026 08:29
@tobyhede
tobyhede requested review from auxesis, coderdan and freshtonic and removed request for auxesis October 22, 2026 08:29

@cipherstash-agent cipherstash-agent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review attestation — automated review by Claude Code (/code-review medium, findings cross-checked against the PR head 8e15737 and existing PR feedback via company-skills:cross-check-review-findings).

Result: no critical or blocking issues; no new findings.

Checked at head:

  • Frontend::route is the only decision point; the Query and Parse paths no longer carry their own check ladders.
  • is_coverage_gap is an allowlist (StatementCouldNotBeTypeChecked, Internal), so other errors fail closed. InvalidInboundEqlPayload and the legacy EQL v2 refusal are not on it.
  • A fallback cannot be forwarded alongside a Route::Map statement. All statements are routed before any is acted on, so statements after a fallback are now checked.
  • With encrypted columns configured, a write falls back only when every target is a known-native table (write_targets returns None for non-plain targets, so those are refused). An unfalsified fallback still returns the original bytes (Ok(None)), not a re-serialised statement.
  • Tests cover the refusal, the extended-protocol path, the no-encrypted-columns case, and native-table-created-earlier edge cases. CHANGELOG, docs/errors.md and CONTEXT.md match the behaviour.

Observation, not a finding: the PR's own comment reports the Go pgx pooler job failing on the base branch (CIP-5102). It is not caused by this change.


Generated by Claude Code

tobyhede pushed a commit that referenced this pull request Oct 22, 2026
Proxy's view of a connection's tables lagged PostgreSQL's inside a simple
Query and for native temporary tables, and #484 patched the gap at the
fallback layer with two side channels. Fix the schema instead:

- The schema middleware keeps each connection's native temporary tables
  as a pg_temp layer the type checker resolves first. A table is added
  when PostgreSQL confirms its CREATE, and removed by DROP, DISCARD,
  ON COMMIT DROP at commit, and rollback of its transaction or savepoint.
- Each statement in a simple Query is type checked against the schema as
  the encryption-neutral DDL before it in the Query leaves it.
- Writes therefore type check normally, so a write never falls back while
  encrypted columns are configured. The earlier-statements slice, the temp
  table side set, writes_only_native_tables, creates_native_table,
  object_names_equal and eql_mapper::write_targets are gone.

A same-batch ALTER TABLE ADD COLUMN or RENAME followed by a write now
type checks instead of being refused, and a write before its CREATE TEMP
TABLE, or after its DROP, is refused.

Closes CIP-5109

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
tobyhede pushed a commit that referenced this pull request Oct 22, 2026
A design-level model of how Proxy maps, forwards or refuses each
statement, checked with TLC: plaintext must never reach an encrypted
column except with UNSAFE_DISABLE_MAPPING. Covers the refuse-by-default
route from #484, simple Query batches, Parse and Bind, transactions and
concurrent DDL.

TLC confirms the safety properties for clients that never disable
mapping, and records three findings as expected counterexamples:
stale routes under concurrent DDL, statement kinds Proxy never type
checks, and a Parse under UNSAFE_DISABLE_MAPPING bound after mapping is
enabled again. Three injected faults are each caught.

Adds a non-required CI workflow. No production code changes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@freshtonic freshtonic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head e06e125a, against the base claude/project-thread-2yjn9s. I first reviewed 88f73d70, and then the two new commits. The core refactor is sound: Frontend::route is the one place that decides, and the allowlist makes refusal the default. The tests guard the refactor well. a113b8fb (AmbiguousTable) is correct. But e06e125a adds a new fail-open path.

Blocking

B1. CREATE TABLE IF NOT EXISTS for a table that PostgreSQL has and Proxy has not loaded forwards plaintext.
batch_models (proxy/schema/middleware.rs:900-920) treats a name that the loaded schema cannot resolve as a new plain table. Its doc comment says the table "has no encrypted columns, or the batch view would not get here". That is true only for tables that are in Proxy's encrypt config. A table that Proxy has not loaded is not in the schema and not in the encrypt config, so neither guard sees it.

Scenario: late already exists in PostgreSQL with an EQL column, but Proxy has not loaded it yet. The client sends one simple Query, CREATE TABLE IF NOT EXISTS late (secret text); INSERT INTO late (secret) VALUES ('plaintext').

  • Proxy models a new plain late, so it forwards the INSERT with the plaintext.
  • PostgreSQL skips the CREATE with a notice and does not abort the batch. Then it runs the INSERT against the real late.
  • Only the EQL domain CHECK stops the value, and the PostgreSQL error and logs then contain the plaintext.
  • CREATE TABLE IF NOT EXISTS public.late … has the same problem.

This is a regression: on 88f73d70, Proxy refused the same Query as an unresolvable table. A plain CREATE TABLE late … is safe, because PostgreSQL fails on the existing table and aborts the batch. Only IF NOT EXISTS lets the batch continue.

This can happen after a migration that runs directly against PostgreSQL, until the next schema reload (config_reload_interval, 60 s by default). It can also happen on a connection in an open transaction whose snapshot is older than a change from another connection. docs/errors.md:438 still says that a table "created on another connection since Proxy last loaded the schema cannot be resolved", so the docs now disagree with the code.

A probe test on e06e125a showed that the Query is forwarded as written, with nothing encrypted.

Fix: revert e06e125a, or model IF NOT EXISTS only for a name that an earlier statement in the same batch created. Please add a test for this case. The new test write_after_create_if_not_exists_of_an_encrypted_table_is_not_forwarded_as_plaintext covers only a table that Proxy already knows.

Checked and safe in the new commits:

  • Ambiguous names: batch_models uses the scratch catalog of permanent tables, so a temporary table cannot make a lookup fail there. A write is refused with AmbiguousTable.
  • CREATE TABLE IF NOT EXISTS on an encrypted name, with or without public. or pg_temp., and CREATE TEMP TABLE IF NOT EXISTS encrypted: the ddl_may_change_encryption guard refuses them.
  • AmbiguousTable reaches every path that used as_table_not_found. A read on an ambiguous name is still forwarded.

Should fix (not blocking)

1. Proxy now refuses every write that fails type checking, also writes to native tables. The CHANGELOG does not say this.
may_fall_back (middleware/frontend.rs:1278-1280): with encrypted columns configured anywhere in the database, Proxy now refuses these statements:

  • UPDATE encrypted SET id = v.x FROM (VALUES (1)) AS v(x) WHERE encrypted.id = v.x ("no matches for identifier v.x")
  • CREATE TEMP TABLE n (id int); INSERT INTO n SELECT x FROM unnest(ARRAY[1,2]) AS x ("Unsupported table source: UNNEST")
  • INSERT INTO encrypted (id) SELECT x FROM unnest(...)
  • INSERT INTO <temp> SELECT * FROM pg_catalog.pg_class

The base branch forwarded the first three. Bulk UPDATE … FROM (VALUES …) is a common pattern in ORMs and in hand-written SQL. The compatibility note in the CHANGELOG names only MERGE and SQL PREPARE, so it does not tell customers the full effect. You can do one of these:

  • Bring back the fallback for "all write targets are known native tables" (write_targets, removed in 1fb2a48).
  • Keep the refusal, and add these examples to the CHANGELOG compatibility note and to docs/errors.md.

A probe test confirmed this on head and on base.

2. COPY (INSERT … RETURNING …) TO STDOUT sends plaintext to an encrypted column.
The Forward::NotTypeChecked arm (frontend.rs:1249-1251) returns before the write check. So Proxy forwards COPY (INSERT INTO encrypted (secret) VALUES ('plaintext') RETURNING id) TO STDOUT unchanged, and PostgreSQL accepts DML with RETURNING in COPY. Only the EQL domain CHECK constraint stops it. This existed before this PR, but it disagrees with "a write never falls back". eql_mapper::writes_data already returns true for this statement. So the fix is small: with encrypted columns configured, route refuses a statement that is not type checked and writes data. A probe test confirmed this.

3. CREATE TABLE pg_temp.n (…); INSERT INTO n … is refused. Proxy treats a CREATE with the pg_temp. prefix and no TEMP keyword as a permanent table. Please document this or model it. (a113b8fb fixes the misleading message for a name that is both a temporary and a permanent table.)

Nits

  1. The temp-table layer can forget a table that PostgreSQL keeps. Temporary::failed covers the time between two ReadyForQuery messages, not one transaction (proxy/schema/middleware.rs:195-211). So in BEGIN; CREATE TEMP TABLE t …; COMMIT; SELECT 1/0 (one simple Query), Proxy forgets t, but PostgreSQL keeps it. This causes over-refusal only. BEGIN; DROP TABLE t; COMMIT; <error> has the same cause and leaves a stale t; the shadowing rule makes that case safe. ROLLBACK AND CHAIN is not handled. Found by reading the code.
  2. The batch view ignores ROLLBACK and COMMIT. Proxy forwards BEGIN; CREATE TEMP TABLE t …; ROLLBACK; INSERT INTO t …. PostgreSQL then fails with "relation does not exist", so this is safe.
  3. No unit test for the allowlist. If is_coverage_gap matches every MappingError, all lib tests still pass. Only the integration test legacy_v2_column::select_is_refused catches it. A unit test that sends a SELECT on a legacy v2 column with mapping errors off would guard it.
  4. No test exercises the encryption guard in batch_resolvers. If you skip ddl_may_change_encryption there, all tests still pass, because simple_query_requires_fail_closed refuses the case first. This is good defence in depth, but the inner guard has no test.
  5. The PR body is wrong about DO. sqlparser cannot parse DO, so Proxy refuses it. CALL and SQL EXECUTE are forwarded, as before.
  6. A read that falls back still sends plaintext query operands to PostgreSQL, for example SELECT * FROM encrypted WHERE secret = 'plaintext' AND nosuch(id). This is how the read fallback is designed to work, and it existed before this PR. But StatementCouldNotBeTypeChecked also covers bound (capability) failures, so the allowlist is wider than its name suggests.

What I checked and found correct

  • One decision point. route decides for a simple Query with one or more statements, for Parse, for SQL PREPARE and for MERGE. When the config disables mapping, intercept returns early for the whole connection. When SET disables mapping, route returns MappingDisabled. No path forwards a statement without a decision.
  • Internal can safely fall back, because may_fall_back requires !writes_encrypted.
  • Write detection. writes_data detects CTE writes, EXPLAIN ANALYZE of a write, PREPARE of a write and MERGE. CTE and EXPLAIN writes are encrypted correctly when they type check. SELECT INTO and CREATE TABLE AS copy only ciphertext.
  • Multi-statement Query. Proxy routes every statement before it acts on any. If the batch has a fallback and a mapped statement, Proxy refuses the whole batch.
  • Temp-table lifecycle. Proxy adds a temp table only after PostgreSQL confirms the CREATE. It handles a failed CREATE, DROP, DISCARD TEMP and DISCARD ALL, ON COMMIT DROP, rollback, rollback to a savepoint, and a pipelined CREATE then Parse.
  • Name resolution. A shadowed name does not resolve. pg_temp.t resolves only to the temp table, and public.t never resolves to it.
  • Batch view. DDL that changes encryption is refused. After ADD COLUMN or RENAME COLUMN on the encrypted table, the write to the encrypted column is still encrypted. Quoted names stay distinct.
  • CI. The pooler failure with Go pgx is fixed on head. All 8 checks pass.

What I ran

  • On e06e125a: cargo test -p cipherstash-proxy --lib: 458 passed, except the known random failure. On 88f73d70: 457 passed (one known test that fails at random, config::tandem::missing_database_config, failed in one run). cargo test -p eql-mapper: 225 passed.
  • About 60 probe SQL cases on 88f73d70, 5 on base and 16 on e06e125a.
  • 12 mutations: 10 make tests fail. The two that no test catches are nits 3 and 4.

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Here is how each point is handled. The commits are about to be pushed.

B1. Agreed, that is a regression. 59f5c424 reverts the IF NOT EXISTS modelling. A write after CREATE TABLE IF NOT EXISTS in the same batch is refused again, unless an earlier statement in the batch created the table. The new cases CREATE TABLE IF NOT EXISTS late …; INSERT INTO late … and the public.late variant are in write_that_is_not_only_to_a_table_created_earlier_in_the_query_is_refused, and both failed before the revert. docs/errors.md matches the code again and names this exception.

Should fix 1. Kept the refusal, since refusing every write that fails type checking is the policy chosen for this PR. 807352dc adds your examples (UPDATE … FROM (VALUES …), INSERT … SELECT … FROM unnest(…), INSERT … SELECT … FROM pg_catalog…) to the CHANGELOG compatibility note and to docs/errors.md.

Should fix 2. d1b558a7: with encrypted columns configured, a statement that skips type checking but writes data is now refused. COPY (INSERT … RETURNING …) TO STDOUT is in UNCHECKED_WRITES, so the simple, extended and no-encrypted-columns tests cover it. COPY … FROM STDIN is not affected, because writes_data is false for it.

Should fix 3. 7fda1d92 treats a CREATE TABLE qualified with pg_temp as temporary, in both the overlay and the batch view.

Nits

  1. Agreed, failed is scoped to the time between two ReadyForQuery messages, not to one transaction. It only over-refuses, and the shadowing rule covers the stale-DROP case. I've left it as is; ROLLBACK AND CHAIN belongs with it.
  2. Left as is. PostgreSQL rejects the write, as you note.
  3. f6fdea38 adds read_of_a_legacy_column_is_refused_rather_than_falling_back. It fails if is_coverage_gap matches every MappingError, which I checked.
  4. f6fdea38 adds a_batch_view_leaves_out_ddl_that_may_change_encryption. It fails if batch_resolvers skips the guard, which I checked.
  5. Fixing the PR body: DO is refused because it can't be parsed. CALL and SQL EXECUTE are forwarded.
  6. Agreed, this is how the read fallback is designed and it predates this PR. I've left it as is.

Generated by Claude Code

@tobyhede
tobyhede requested a review from freshtonic October 22, 2026 23:45
tobyhede pushed a commit that referenced this pull request Oct 22, 2026
A design-level model of how Proxy maps, forwards or refuses each
statement, checked with TLC: plaintext must never reach an encrypted
column except with UNSAFE_DISABLE_MAPPING. Covers the refuse-by-default
route from #484, simple Query batches, Parse and Bind, transactions and
concurrent DDL.

TLC confirms the safety properties for clients that never disable
mapping, and records three findings as expected counterexamples:
stale routes under concurrent DDL, statement kinds Proxy never type
checks, and a Parse under UNSAFE_DISABLE_MAPPING bound after mapping is
enabled again. Three injected faults are each caught.

Adds a non-required CI workflow. No production code changes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>

@freshtonic freshtonic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed head 4d14630b, against the base claude/project-thread-2yjn9s (now at 025dba5b). Thank you for the quick fixes. B1, the COPY write and pg_temp. are fixed, and the two missing guard tests are added. But the merge of 025dba5b from the base creates a new fail-open path.

Blocking

B2. If every encrypted column has a domain conflict, writes to those columns are forwarded as plaintext.
025dba5b removes a column's encrypt config when tables with the same name in two schemas declare different domains. If those are the only encrypted columns, the encrypt config becomes empty. Then has_encrypted_columns() (context/mod.rs:1748, !encrypt_config().is_empty()) is false, so the write refusal (frontend.rs:1273) and the refusal of a write in a statement that is not type checked (frontend.rs:1253) both turn off. But the schema still marks the columns as EQL.

Scenario: app.users.email is eql_v3_text_search, public.users.email is eql_v3_text, and no other column is encrypted. Then Proxy forwards these as written, with the plaintext:

  • UPDATE app.users SET email = v.x FROM (VALUES ('plaintext')) AS v(x) …
  • MERGE INTO app.users … INSERT (email) VALUES ('plaintext')
  • COPY (INSERT INTO app.users (email) VALUES ('plaintext') RETURNING id) TO STDOUT
  • INSERT INTO users (email) VALUES ('plaintext'). The bare name matches two schemas, so the statement falls back.

Only the EQL domain CHECK in PostgreSQL stops these. Proxy refuses INSERT INTO app.users … and SELECT email FROM app.users correctly. The 025dba5b test adds another encrypted column "so encrypted columns are configured", and that hides this case. A probe test on 4d14630b confirmed it.

Fix: make has_encrypted_columns true when the effective schema has any EQL column, or when the encrypt config is not empty or the schema has an EQL column. Add a test with no other encrypted column.

Fixed since my last review (checked by probe tests)

  • B1: Proxy refuses CREATE TABLE IF NOT EXISTS late …; INSERT INTO late … and the public.late variant again. The exception for a table that an earlier statement in the same Query created is safe. If the first CREATE fails because the table exists, PostgreSQL skips the rest of the Query.
  • The COPY write (d1b558a7): Proxy refuses COPY (INSERT/UPDATE/DELETE … RETURNING) TO STDOUT in both protocols, also after a SELECT in the same Query. It still forwards COPY (SELECT …) TO, COPY t TO, EXECUTE and CALL.
  • pg_temp. (7fda1d92):
    • Proxy treats CREATE TABLE pg_temp.n as a temporary table, also with other letter case and with quotes.
    • Proxy removes it after DROP, ON COMMIT DROP, rollback to a savepoint, a failed transaction and DISCARD TEMP.
    • When a permanent table has the same name, a write to that name is refused as ambiguous.
  • Guard tests (f6fdea38): M1 and M12 now make tests fail, and so does removing the d1b558a7 check.
  • The merge: none of 21f623dc's write_targets fallback came back. Other statements cover the cases that its tests covered. proxy/schema/manager.rs matches 025dba5b. When another column is encrypted, a write to app.users never uses the config of public.users.
  • Over-refusal: you chose to keep the refusal and document it. I accept that.

Nits

  1. The CHANGELOG entry now has the examples, but its "Compatibility note" line (CHANGELOG.md:82) still names only MERGE and SQL PREPARE. Add the native-table writes and COPY (…) TO.
  2. The list of refusals at docs/errors.md:436 does not have COPY (…) TO, but docs/errors.md:250 now lists it.
  3. The temporary-table layer and the batch view always resolve a bare name to the temporary table first, and Proxy does not track SET search_path. With search_path = public, pg_temp, CREATE TEMP TABLE x …; CREATE TABLE IF NOT EXISTS x …; INSERT INTO x … writes to public.x, which Proxy may not have loaded. The default search path is safe. Please at least document this.
  4. Proxy still models IF NOT EXISTS for a temporary table (CREATE TEMP TABLE IF NOT EXISTS late …, or pg_temp.late). This is unsafe only if a temporary table with an EQL column exists that Proxy did not see, for example one created inside a function. That is unlikely, because Proxy does not support encrypted temporary tables.
  5. Your reply says that COPY FROM STDIN is "not affected because writes_data is false". On this head, it never reaches route. The parser refuses it in both protocols with "Expected: ;, found: EOF". #483 fixes this on 2.2.
  6. Earlier nits 1 and 6 are still open, as you said: failed covers the time between two ReadyForQuery messages, not one transaction, and Proxy does not handle ROLLBACK AND CHAIN. A read that falls back can send plaintext operands. These can wait.

What I ran

  • On 4d14630b: cargo test -p cipherstash-proxy --lib 464 passed, cargo test -p eql-mapper 225 passed, and cargo fmt --check and cargo clippy --workspace --all-targets -D warnings report no problems.

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. These commits are about to be pushed.

B2. Agreed. af2dcb05 makes has_encrypted_columns true when the loaded schema has any encrypted column (EQL or legacy v2), not only when the encrypt config is non-empty. write_is_refused_when_only_columns_with_no_config_are_encrypted uses your scenario with an empty encrypt config. It covers the bare-name INSERT, UPDATE … FROM (VALUES …), MERGE and the COPY write, and it failed before the fix.

Nits

  1. cb5c9212: the compatibility note now lists the native-table writes and COPY (…) TO.
  2. cb5c9212: added COPY (…) TO to the Unresolvable table notes.
  3. cb5c9212: documented in CONTEXT.md and under Ambiguous table that Proxy assumes pg_temp is searched first and does not track SET search_path.
  4. Leaving this as is: it needs an encrypted temporary table that Proxy didn't see, and Proxy doesn't support encrypted temporary tables.
  5. You're right, my earlier note was wrong. COPY … FROM STDIN is refused by the parser on this branch before it reaches route, and fix(proxy): forward COPY FROM STDIN into plaintext tables, refuse it into encrypted columns (2.2, CIP-5101) #483 fixes that on 2.2.
  6. Agreed that these can wait. The plaintext-operand question for reads is with Toby.

Generated by Claude Code

@tobyhede
tobyhede requested a review from freshtonic October 23, 2026 00:27

Copy link
Copy Markdown
Contributor Author

Pushed d8d1d784, which resolves nit 6 per Toby's decision. A statement that uses an operation an encrypted column's EQL type does not support, such as secret > 'x' on a column with no ordering index, now fails with a new UnsupportedEncryptedOperation error. Proxy refuses it instead of letting it fall back with the operand in plaintext. This applies to reads too, and whether or not mapping errors are enabled. It is documented in docs/errors.md#mapping-unsupported-encrypted-operation and covered by read_using_an_operation_an_encrypted_column_does_not_support_is_refused.


Generated by Claude Code

tobyhede added a commit that referenced this pull request Oct 23, 2026
Proxy's view of a connection's tables lagged PostgreSQL's inside a simple
Query and for native temporary tables, and #484 patched the gap at the
fallback layer with two side channels. Fix the schema instead:

- The schema middleware keeps each connection's native temporary tables
  as a pg_temp layer the type checker resolves first. A table is added
  when PostgreSQL confirms its CREATE, and removed by DROP, DISCARD,
  ON COMMIT DROP at commit, and rollback of its transaction or savepoint.
- Each statement in a simple Query is type checked against the schema as
  the encryption-neutral DDL before it in the Query leaves it.
- Writes therefore type check normally, so a write never falls back while
  encrypted columns are configured. The earlier-statements slice, the temp
  table side set, writes_only_native_tables, creates_native_table,
  object_names_equal and eql_mapper::write_targets are gone.

A same-batch ALTER TABLE ADD COLUMN or RENAME followed by a write now
type checks instead of being refused, and a write before its CREATE TEMP
TABLE, or after its DROP, is refused.

Closes CIP-5109

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@tobyhede
tobyhede force-pushed the claude/project-thread-hqgv43 branch from d8d1d78 to 1190a91 Compare October 23, 2026 00:56

@freshtonic freshtonic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed head 1190a910. Its file tree is byte-for-byte the same as the tree of d8d1d784, which is the commit I reviewed. I reviewed af2dcb05 and cb5c9212 first; the merge a6dd4f91 changes no file. Then I reviewed d8d1d784. B2 is fixed, so I approve.

B2 (af2dcb05)

  • has_encrypted_columns (context/mod.rs:1749) is now also true when the loaded schema has a column that is not native, even when the encrypt config is empty. This includes EQL columns and legacy v2 columns.
  • Both write refusals use it: frontend.rs:1253 and :1273. These are its only callers. Only the backend uses is_passthrough, which is still based on an empty config, so it does not change the frontend decision.
  • write_is_refused_when_only_columns_with_no_config_are_encrypted covers my scenario with an empty encrypt config: the bare-name INSERT, UPDATE … FROM (VALUES …), MERGE and the COPY write. I removed the new check locally, and the test failed. With the check, all 25 unresolvable_table_tests pass.
  • The test harness now adds the legacy table only when encrypted is set. This is necessary, because a legacy v2 column now counts as an encrypted column, so the tests without encryption would refuse writes.

Docs (cb5c9212)

  • The compatibility note in the CHANGELOG now lists the native-table writes and COPY (…) TO.
  • The "Unresolvable table" section now lists COPY (…) TO.
  • CONTEXT.md and the "Ambiguous table" section say that Proxy assumes pg_temp is searched first and does not track SET search_path.

Unsupported operations (d8d1d784)
This fixes my earlier nit 6 for operations that a column does not support. mapping_error (frontend.rs:1329) now refuses an UnsatisfiedBounds error as UnsupportedEncryptedOperation, and does not treat it as a coverage gap that falls back. It does this before the unresolvable-table check and in both protocols, because both protocols use mapping_error. UnsatisfiedBounds comes only from bound checks on EQL types: native types meet every bound. So catalog queries and reads of plaintext columns still fall back. The error text names the type and the missing bound, not the operand. read_using_an_operation_an_encrypted_column_does_not_support_is_refused fails if the check is removed (I tried this).

Nits

  • From reading the type checker (I did not probe these), UnsatisfiedBounds also comes from projections, for example SELECT DISTINCT or UNION over an encrypted column that has no Eq. It also comes from ORDER BY or GROUP BY on a column with no ordering or equality index. These reads send no plaintext operand. Before this commit they fell back, and PostgreSQL ordered or compared the ciphertext as jsonb, which gives wrong results with no error. So refusing them is better. But the CHANGELOG names only <. Please add the projection and ORDER BY cases to the compatibility note, because applications that send them will now get an error.
  • The new test covers only the simple protocol. The extended protocol uses the same mapping_error, so a short Parse test would guard it.
  • A database that has only legacy v2 columns, and no encrypt config, now refuses writes that Proxy cannot type check. This is correct, because Proxy cannot map those columns. But the CHANGELOG says "while encrypted columns are configured", so a user with only v2 columns might not see that this applies to them. A few words, such as "including legacy EQL v2 columns", would make it clear.

What I ran

  • cargo test -p cipherstash-proxy --lib on d8d1d784: 466 passed. cargo fmt --check and cargo clippy -D warnings report no problems.
  • cargo test -p eql-mapper: 225 passed.

Conditions

  • This approval is conditional on CI passing. GitHub runs no CI on 1190a910, because the PR now conflicts with its base branch, so the last CI result for this tree comes from d8d1d784. That run was still in progress when I approved.
  • Please resolve the conflict, and make sure CI passes on the new head before merge.

Approved, on the condition that CI passes.

Copy link
Copy Markdown
Contributor Author

Thanks for approving. I pushed a68b07b3. It merges the latest base (2ca6d255), which resolves the conflict, and handles your three nits:

  • extended_read_using_an_operation_an_encrypted_column_does_not_support_is_refused covers the Parse path.
  • The CHANGELOG now names ORDER BY, GROUP BY, DISTINCT and UNION over a column without the index they need, both in the entry and in the compatibility note.
  • The CHANGELOG now says the write refusal also applies to legacy EQL v2 columns.

I'll confirm here when CI on the new head is green.


Generated by Claude Code

@tobyhede
tobyhede force-pushed the claude/project-thread-2yjn9s branch from e0fb5da to 6143e4e Compare October 23, 2026 02:14
@tobyhede
tobyhede force-pushed the claude/project-thread-hqgv43 branch from fb28477 to 089c427 Compare October 23, 2026 02:22
@tobyhede
tobyhede force-pushed the claude/project-thread-2yjn9s branch from 6143e4e to e9d509a Compare October 23, 2026 03:41
Base automatically changed from claude/project-thread-2yjn9s to main October 23, 2026 03:41
@tobyhede
tobyhede force-pushed the claude/project-thread-hqgv43 branch 3 times, most recently from 35ca8e8 to 7aea8ff Compare October 23, 2026 04:47
Frontend::route is now the only place that decides what happens to a
statement: map it, forward it for a named reason, or refuse it. Only a
coverage gap (a statement EQL Mapper could not type check, or an internal
error) may fall back to forwarding the statement as written, and only for
a read while mapping errors are off. Any other error, including one added
later, refuses. While any column is encrypted, a write never falls back.

So that writes to native tables still type check instead of being
refused, Proxy's schema now covers them:

- A connection's native temporary tables live in a temporary layer that
  resolves before permanent tables. A table is added once PostgreSQL
  confirms its CREATE, including CREATE TABLE pg_temp.name, and removed by
  DROP, DISCARD, ON COMMIT DROP, rollback and rollback to a savepoint. A
  bare name that also names a permanent table is ambiguous and refused.
- In a simple Query, each statement is type checked against the schema
  plus the encryption-neutral DDL before it in the same Query. CREATE
  TABLE IF NOT EXISTS is not modelled, since the table may already exist
  with encrypted columns Proxy has not loaded.

Also refuses a write nested in a statement Proxy does not type check, such
as COPY (INSERT ... RETURNING ...) TO STDOUT; an operation an encrypted
column's EQL type does not support, which would otherwise reach
PostgreSQL with a plaintext operand or compare ciphertext; and counts
encrypted columns in the loaded schema even when the encrypt config has
none for them.
@tobyhede
tobyhede force-pushed the claude/project-thread-hqgv43 branch from 7aea8ff to 109eca9 Compare October 23, 2026 05:03
@tobyhede
tobyhede merged commit 1519312 into main Oct 23, 2026
9 checks passed
@tobyhede
tobyhede deleted the claude/project-thread-hqgv43 branch October 23, 2026 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants