fix(proxy): make schema changes transaction aware (BUG-308)

[freshtonic]

Summary

• publish PostgreSQL schema and encryption metadata as one atomic, versioned snapshot
• add transaction-pinned overlays with successful-execution activation, savepoint semantics, and extended-protocol pipeline deferral
• fail closed for unmodelled DDL, publication failures, and dependent simple-query batches
• add state-machine and TLS/database regressions, including direct ciphertext-at-rest verification through an already-open second connection

Issue

BUG-308

Validation

• mise run test:format
• mise run test:clippy
• mise run test:check
• 382/382 non-multitenant integration tests
• 6/6 final-image BUG-308 TLS regressions
• release cross-build and Docker image build
• unit suite: 371/372 passed; the existing macOS SystemConfiguration test panics in the full suite but passes in isolation

[tobyhede]

Review — PR #460, fix(proxy): make schema changes transaction aware (BUG-308)

Scope of this review: origin/main...HEAD. This is 3 commits and 18 files (+2005 / -229). Local main is 11 commits behind origin/main.

The design is correct in its core. The generation algebra in coalesced_reload is sound. The ExceptionInjected path correctly suppresses the duplicate Sync boundary. discard_skipped_executions correctly stops at the first ReadyBoundary. The problems below are in the edges around that core.

Two findings let plaintext go to the server. Three findings stop normal client work. I recommend that you correct findings 1 to 5 before you merge.

---

1. Blocking — the DDL predictor reads domain names with case sensitivity

packages/cipherstash-proxy/src/proxy/schema/middleware.rs:311-351
packages/cipherstash-proxy/src/proxy/schema/eql_domains.rs:76-80

column_domain keeps the case of the type name as the client wrote it. eql_domains::resolve then does a case-sensitive lookup in a map. The map keys come from pg_catalog, so they are always lower case.

PostgreSQL folds an unquoted type name to lower case. Therefore EQL_V3_TEXT_SEARCH is an encrypted domain on the server, but ddl_may_change_encryption classifies it as native.

Send this as one simple-query message:

CREATE TABLE secrets (id bigint, value EQL_V3_TEXT_SEARCH);
INSERT INTO secrets (id, value) VALUES (1, 'classified');

The batch passes simple_query_requires_fail_closed. The INSERT then fails the type check, because secrets is not in the pre-batch schema. That error is not in must_fail_closed. With the default enable_mapping_errors = false, frontend.rs:497-501 returns Ok(None) and forwards the original batch. The literal 'classified' goes to PostgreSQL in plaintext.

ALTER TABLE t ADD COLUMN c EQL_V3_TEXT_SEARCH has the same result.

Commit 5956d84d did not have this hole, because its blanket rule rejected the batch. Commit cb0f9b76 opened it.

Correct resolve and column_domain to apply PostgreSQL folding rules: fold an unquoted name to lower case, and keep a quoted name as written.

---

2. Blocking — a savepoint name with different case or quotes fails open

packages/cipherstash-proxy/src/proxy/schema/middleware.rs:416-439

rposition compares Ident values. Ident::PartialEq compares both value and quote_style. PostgreSQL folds unquoted identifiers, and it treats "foo" and foo as the same savepoint.

If the two spellings do not agree, rposition returns None. The arm then does nothing. The overlay keeps DDL that PostgreSQL has rolled back. The code does not set unmodelled or dirty, so nothing stops the next statement.

BEGIN;
CREATE TABLE users (id bigint, secret eql_v3_text_search);
SAVEPOINT Foo;
ALTER TABLE users DROP COLUMN secret;
ROLLBACK TO SAVEPOINT foo;
INSERT INTO users (secret) VALUES ('classified');

PostgreSQL restores the encrypted column. The middleware does not. The INSERT maps against an overlay that has no users.secret, fails the type check, and goes to passthrough. Plaintext goes into a column that is still eql_v3_text_search.

The ReleaseSavepoint arm at :407-415 has the same comparison fault. It leaves a stale stack entry.

Most ORMs generate self-consistent lower-case names, so mainstream drivers are safe. Hand-written SQL and Connection.setSavepoint("Name") in JDBC are not.

Two changes are necessary. Compare identifiers with PostgreSQL folding rules. Also make the None branch fail closed, so that an unexpected desync can never fail open.

Note: a savepoint name that the server does not know is not the trigger. In that case PostgreSQL returns 3B001, and execution_failed discards the intent correctly.

---

3. Blocking — pipelined DDL and a dependent statement cause a deadlock

packages/cipherstash-proxy/src/postgresql/frontend.rs:851 (also :452)

parse_handler calls wait_for_schema_execution() before it forwards the message. wait_for_ddl at middleware.rs:355-363 has no timeout.

PostgreSQL holds extended-protocol output until it receives Sync or Flush. Frontend owns client_reader and is the only reader of the client socket. If the frontend waits, it cannot read the client's Sync.

Parse("CREATE TABLE t (...)")
Bind
Execute                      -> in_flight_ddl = 1
Parse("INSERT INTO t ...")   -> wait_for_ddl() blocks here
Sync                         -> the proxy never reads this

CommandComplete for the DDL never arrives. in_flight_ddl stays at 1. Both sides wait.

The connection stalls for connection_timeout, which is 120 seconds by default. If an operator sets connection_timeout = 0, the connection waits forever.

This affects every driver that sends one Sync for a batch: libpq pipeline mode, pgx SendBatch, pgjdbc executeBatch, and Npgsql NpgsqlBatch. This is the exact shape that the module doc says the design supports.

A client that sends Flush after Execute is safe, because the code forwards Flush before it reads the next Parse.

tokio_postgres sends one Sync for each statement. This is why pipelined_statement_waits_for_extended_ddl_activation does not catch the fault.

---

4. Blocking — ALTER TABLE operations that are safe stop the transaction

packages/cipherstash-proxy/src/proxy/schema/middleware.rs:639-649

is_modelled_ddl matches only AddColumn, RenameColumn, RenameTable, and non-cascade DropColumn. The _ => false arm covers about 39 other variants.

These common operations are therefore unmodelled:

ALTER TABLE t ADD CONSTRAINT ...
ALTER TABLE t ALTER COLUMN c SET NOT NULL
ALTER TABLE t ALTER COLUMN c DROP NOT NULL
ALTER TABLE t ALTER COLUMN c SET DEFAULT ...
ALTER TABLE t OWNER TO ...
ALTER TABLE t ENABLE ROW LEVEL SECURITY

unmodelled does more than start a reload. ensure_schema_modelled() is a ? at frontend.rs:453 and frontend.rs:852, before every mapping decision. UnmodelledDdl is in must_fail_closed, so the proxy never downgrades it to passthrough.

Inside a transaction, only ROLLBACK clears the flag. Therefore:

BEGIN;
ALTER TABLE users ADD COLUMN email text;
ALTER TABLE users ADD CONSTRAINT users_email_uq UNIQUE (email);
UPDATE users SET email = lower(email);   -- ERROR: UnmodelledDdl
COMMIT;

This is the shape of a typical Rails, Django, Flyway, or Alembic migration.

None of these operations can change encryption metadata. Add the encryption-neutral variants to is_modelled_ddl.

The integration suite has no ALTER TABLE statement at all, so no test covers this.

---

5. Blocking — CREATE TEMP TABLE stops the rest of the transaction

packages/cipherstash-proxy/src/proxy/schema/middleware.rs:629-638

is_modelled_ddl treats create.temporary and create.on_commit as unmodelled, but is_schema_ddl matches CreateTable(_) always. A successful CREATE TEMPORARY TABLE therefore sets unmodelled = true through execution_succeeded at :452-458.

BEGIN;
CREATE TEMPORARY TABLE t (name text);
SELECT id FROM anything;   -- ERROR: UnmodelledDdl

The refused statement does not have to touch t.

Outside a transaction the flag clears, but only through a full authoritative catalog reload. mark_publication_pending() increments a process-wide counter, so every other idle connection also takes the publication branch before its next statement. One temporary table for each request gives one catalog reload for each request.

The module doc at :44-46 says that native temporary-table batches can pass through. That is true of the pre-execution guard only. It is not true of the post-execution flag.

native_temporary_table_batch_does_not_fail_closed at :947 asserts only simple_query_requires_fail_closed(...) == false. It never calls simple_query() or execution_succeeded(), so it cannot see the flag.

The pgx batch in tests/integration/golang/pgx_test.go:57 does not reach this path, for an unrelated reason. Its INSERT fails the type check first, and frontend.rs:493-501 returns from query_handler before it records anything. The claimed verification of the Go path does not exercise the flag.

---

6. Simple-query execution intents do not agree with the forwarded batch

packages/cipherstash-proxy/src/postgresql/frontend.rs:500 and :591

Two faults share one cause. The proxy derives the intent queue from parsed_statements, not from the bytes that it sends to the server.

6a. The type-check fallback forwards the batch with no tracking.

The Ok(None) return at :500 occurs 91 lines before execute_simple_schema_statements at :591. The batch goes to the server with no Execute intents and no ReadyBoundary. The Err path calls mark_schema_protocol_boundary() correctly; this path does not.

This is a regression. On origin/main, check_for_schema_change(statement) ran inside the loop, before the type check. DDL in such a batch set schema_changed and started a reload. On this branch the proxy records it nowhere, and no publication occurs until the periodic reload.

mapping_errors_enabled() returns false by default, so this is the production path for any mapper failure that is not must_fail_closed. MERGE is one example, because the mapper always rejects it.

parse_handler records its intent at :854-855, before its own fallback at :886. Only the simple path has the regression.

6b. The rewritten batch can hold fewer statements than the queue.

When encrypted == true, the code joins transformed_statements. That vector omits every statement that hit continue at :484. simple_query() still queues one intent for each parsed statement.

CREATE TABLE staging (id bigint);
INSERT INTO encrypted (id, encrypted_text) VALUES (1, 'x');

The proxy sends only the INSERT. The INSERT's CommandComplete pops the CREATE intent and applies it, which puts a phantom table in the overlay.

The queue re-syncs at each ReadyBoundary, and an authoritative reload erases the phantom when the status is I. Inside an open transaction there is no reload, so the phantom stays for the rest of the transaction. The result is a false relation does not exist error and one unnecessary reload for each such batch.

The statement-dropping itself is pre-existing and identical on origin/main. The 1:1 accounting at :591 is new, and it turns the old behaviour into a tracking fault.

I could not construct a plaintext write from 6b alone. The fail-closed guard catches every batch where the DDL touches a table that is in encrypt_config.

Derive the intents from the statements that the proxy forwards, and record them before the early returns. One change corrects both parts.

---

7. The startup warning for unprotected data is gone

packages/cipherstash-proxy/src/proxy/encrypt_config/manager.rs:100

mod.rs no longer exports EncryptConfigManager, and proxy/mod.rs deleted every call site. manager is a private module, so the type is now unnameable. init, load, is_empty, reload, init_reloader, the periodic reload task, and load_encrypt_config_with_retry are all unreachable.

lib.rs:1 has #![allow(dead_code)], which beats the -D warnings flag in CI. The dead code therefore ships with a green build.

These three warnings exist on this branch only inside the dead function:

ENCRYPT CONFIGURATION NOT LOADED
No active Encrypt configuration found in database.
Data is not protected with encryption

SchemaManager::init_reloader logs Loaded committed schema snapshot without a condition. The only emptiness check in load_snapshot is on tables, at manager.rs:388-390, not on the encrypt config.

An operator who starts Proxy against a schema that has tables but no encrypted columns now sees one info line. Every statement runs in passthrough, and nothing says so. This is a loss of security observability.

Delete the module, and move the warning into SchemaManager::init_reloader.

---

8. The ADR no longer describes the implementation

packages/cipherstash-proxy/docs/adr/0001-transaction-aware-schema-middleware.md:52

The ADR is unconditional:

A simple-query message containing DDL followed by a schema-dependent statement fails closed for the initial implementation.

Commit 5956d84d implemented that rule exactly. Commit cb0f9b76 narrowed it to DDL that the proxy predicts can change encryption metadata. That commit amended the middleware.rs module doc only.

The ADR still has status: accepted at line 2. Three documents are now incorrect:

• ADR line 52, and the coverage claim at line 87.
• docs/errors.md — "it refuses the complete batch before PostgreSQL executes any part of it".
• CHANGELOG.md — "dependent simple-query batches ... fail closed".

The narrowed rule now permits these batches:

1. CREATE TABLE with native columns only, and with no AS, LIKE, CLONE, or INHERITS.
2. ALTER TABLE t where t is not in encrypt_config, and where every operation is a native ADD COLUMN, DROP COLUMN, RENAME COLUMN, or RENAME TABLE.
3. DROP TABLE where no named table is in encrypt_config.

The narrowing is defensible, and it restores pgx compatibility. Please make it explicit. Amend the ADR, docs/errors.md, and the CHANGELOG to state the actual boundary. The ADR is accepted, so amend it deliberately rather than as a cleanup.

The ADR verification claim at line 87 is also not exercised. packages/cipherstash-proxy-integration/src/schema_change.rs:150-167 uses create_encrypted_table, so it passes under both the blanket rule and the narrow rule. No test covers the path that the narrow rule newly permits.

---

9. A temporary table can shadow an encrypted table

packages/cipherstash-proxy/src/proxy/schema/middleware.rs:313-322

The CreateTable arm never checks the name against encrypt_config. The AlterTable and Drop arms both call contains_table.

With public.users (id bigint, email eql_v3_text_search) in the config:

CREATE TEMP TABLE users (id bigint, email text);
INSERT INTO users (id, email) VALUES (1, 'alice@example.com');

The proxy maps the INSERT against the pinned snapshot and encrypts. PostgreSQL resolves users to pg_temp.users, so ciphertext JSON goes into a text column.

No plaintext escapes. The direction is over-encryption and data corruption, not disclosure. A leak would need proxy-native and PostgreSQL-encrypted, and the publication connection cannot see temporary objects, so the proxy can only over-classify.

The divergence lasts for the session. adopt_latest() clears unmodelled after publication, so the UnmodelledDdl guard does not catch it.

Make the CreateTable arm consult contains_table, as the other two arms do.

---

10. Duplicated ReadyForQuery publication logic

packages/cipherstash-proxy/src/postgresql/backend.rs:187-194 and :301-312

These six lines are identical text in both places:

if bytes.last() == Some(&b'I') {
    self.context.publish_schema_if_changed().await?;
}
if let Some(status) = bytes.last().copied() {
    self.context.schema_ready_for_query(status);
}

They handle the same case, reached through two exits of the same rewrite() function. They are not two different cases.

This is not a fault today. The drift risk is real, because these lines gate a fail-closed boundary: publication must complete before ReadyForQuery(I) reaches the client. An edit that changes one copy silently reopens the stale-metadata window on the other path.

Extract one helper. That helper is also the correct place for the point below.

middleware.rs:224 takes the transaction status as a bare u8. The three real values are I, T, and E, and an invalid byte fails safe, so this is not a fault. An Idle / InTransaction / FailedTransaction enum would make status != b'I' readable, and would let the middleware distinguish E from T later. There is no existing enum in the codebase to reuse.

---

11. CONTEXT.md overstates middleware ownership

packages/cipherstash-proxy/CONTEXT.md:133-138

The text says that the middleware owns "reload coordination, and schema publication". The ADR at line 27 says "sole owner".

The work is in three places. SchemaMiddleware owns the state and the predicates. Context owns reload_sender and the ReloadCommand round-trip, at context/mod.rs:643 and :875. SchemaManager owns coalescing and generation order, at schema/manager.rs:190-222.

The runtime design is sound. Only the wording misleads. A reader will look for a reload sender inside SchemaMiddleware and not find one, which is the failure that CONTEXT.md exists to prevent.

One sentence corrects it. For example: "The middleware owns the publication decision. Context drives the reload round-trip, and SchemaManager coalesces and orders reloads."

Note that context/mod.rs:639 (adopt_latest_schema) is a one-line delegation and is correct. Cite :643 and :875 instead.

---

Checked and cleared

These points were raised and did not survive verification. I record them so that nobody spends time on them again.

• wait_for_ddl lost wakeup (middleware.rs:364-372). Not a fault. In tokio 1.48, Notify::notified() records the notify_waiters counter when it creates the future, and inner_notify_waiters increments that counter even with no registered waiter. poll_notified compares the two and returns Ready. Registration happens at the notified() call, so create-then-check is the correct idiom, and enable() is not necessary. A test that closes the claimed window did not hang.
• ROLLBACK TO SAVEPOINT with an unknown name. PostgreSQL returns 3B001, and execution_failed discards the intent. See finding 2 for the real trigger.
• A plaintext write from the intent misalignment in 6b alone. The fail-closed guard catches every batch where the DDL touches a table in encrypt_config.
• Temporary-table shadowing as a disclosure path. It over-encrypts. See finding 9.
• Schema-qualified evasion, such as DROP TABLE public.users. object_name takes the last identifier, and contains_table compares bare names.
• CREATE TABLE AS, LIKE, and ALTER COLUMN ... TYPE. All are covered by ddl_may_change_encryption.
• DO-block, ALTER DOMAIN, and ALTER SCHEMA RENAME statements. The parser rejects these before this code runs. This is a compatibility limit, not a bypass.
• SELECT ... INTO, CALL, SET search_path, CREATE EXTENSION. These fall through, but is_schema_ddl also returned false for them on origin/main. Pre-existing, not introduced.

Test gaps

• No test drives a batch API that sends one Sync (finding 3).
• The integration suite contains no ALTER TABLE statement (finding 4).
• No test observes unmodelled after execution (finding 5).
• No test drives query_handler through the Ok(None) fallback with DDL in the batch (finding 6a).
• No test covers the path that commit cb0f9b76 newly permits (finding 8).
• Every savepoint test uses the same unquoted lower-case name on both sides, so none can catch finding 2.

Conventions

Commits follow Conventional Commits and carry DCO sign-offs. The ADR follows the eql-mapper precedent for numbering, frontmatter, and structure. docs/errors.md gains anchored sections with "How to fix" for both new refusals. The CHANGELOG has a ### Security entry under ## [Unreleased].