fix(proxy): refuse statements on a legacy eql_v2_encrypted column instead of serving it as plaintext (CIP-3688) - #437
Open
freshtonic wants to merge 3 commits into
Conversation
A column still declared as the EQL v2 composite type has no v3 domain identity, so the schema loader fell through to Column::native — a plaintext passthrough. Proxy then ran no encryption on writes and no decryption on reads, and the only signal was one warn! at startup. That is the shape of a partly-finished migration: 19 of 20 columns moved to v3 domains, and the twentieth quietly accumulates plaintext. Model such a column as its own ColumnKind::UnmappableEncrypted instead, which the type checker refuses wherever schema columns enter the type system. The refusal is exempt from the mapping-error passthrough, so it holds with CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS unset. Checkpoint commit: inherited work-in-progress, not yet verified to compile or pass. Integration fixture and tests still to come.
After the move to EQL v3, a column still declared with EQL v2's `eql_v2_encrypted` composite type has no v3 domain identity, so the schema loader fell through to `Column::native` — a plaintext passthrough. Proxy ran no encryption on writes and no decryption on reads, an application writing to the column saw no error, and the only signal was one warning at schema load. This is the shape of a partly finished migration: 19 of 20 columns move to v3 domains and the twentieth quietly accumulates plaintext in a database its operator believes is encrypted. Model such a column as `ColumnKind::UnmappableEncrypted` and refuse at the single point where schema columns enter the type system, so `SELECT`, `INSERT`, `UPDATE` and `DELETE` all fail with an error naming the column, its type, and the need to migrate it. Three choices were on the table. Refusing at startup is loudest but takes a whole deployment offline for one unmigrated column, including for tables nobody queries. An opt-in flag re-creates the same silent plaintext write for anyone who sets it, and the value of failing closed is that it cannot be switched off by accident. Statement-level rejection keeps the blast radius at the one unmigrated table, which is where the problem actually is. The refusal is scoped to the table rather than the column on purpose. Proving a statement can never route a value into or out of the column — across `*`, defaults, RETURNING, triggers and rules — is a negative that fails *open* whenever it is wrong, and failing open is the bug being fixed. Refusing on the table needs no such proof. It is also exempt from the `mapping_errors_enabled` passthrough. That fallback exists for statements Proxy does not understand, on the reasoning that they probably touch nothing encrypted; here Proxy understands the statement exactly, and forwarding it is the specific harm the error exists to prevent. So the guard holds with CS_DEVELOPMENT__ENABLE_MAPPING_ERRORS unset — the default, and what the proxy container runs with — and does not wait on CIP-3680. The loader now also recognises the DOMAIN spelling of the type, not just the composite one it keyed on before. A missed v2 column is a plaintext column, so the cheap extra check is worth having. Fixes CIP-3688.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`direct_to_postgres` connected to `PG_PORT`, the non-TLS PostgreSQL on 5532. The TLS suite CI runs puts Proxy in front of `postgres-tls` on 5617, so these tests inspected a database that Proxy never writes to. That is worse here than an ordinary wrong-port bug. Every assertion in this file is a negative — that a refused statement stored nothing — and a negative assertion against the wrong, empty database passes for the wrong reason. The suite would have reported success while checking nothing. Now uses `get_database_port()`, the same helper `query_direct_by` and `reset_schema` use. Verified by reintroducing the CIP-3688 defect (`Column::native` for a v2 column) with the ports corrected: 7 of the 8 tests fail and only the control passes, so the assertions do bite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On this EQL v3-only build, a column still declared as the legacy
eql_v2_encryptedcomposite type has no v3 domain identity, soSchemaManagerfell back toColumn::native(ident)— a plaintext passthrough. Proxy performed no encryption on writes and no decryption on reads.The only guard was a
warn!emitted once at schema load. The code's own comment admitted the problem: "This is a data-at-rest exposure on the write path, so the warning must be impossible to miss in ops."This is the shape of a partially-completed migration: move 19 of 20 columns to v3 domains, restart Proxy, and the twentieth starts accumulating plaintext in PostgreSQL with no error visible to the application.
The fix, and why this option
Statement-level rejection, scoped to the table rather than the column.
The alternatives were weighed and rejected. Refusing at startup takes a whole deployment offline for one unmigrated column, including tables nobody queries. An opt-in flag re-creates the identical silent plaintext write for anyone who sets it, and the whole value of failing closed is that it cannot be switched off by accident.
Table scope rather than column scope is deliberate: proving a statement can never route a value into the column — across
*, defaults,RETURNING, triggers, rules — is a negative that fails open when wrong, and failing open is the bug being fixed.Independent of CIP-3680 — verified, not assumed
Error::must_fail_closed()exempts this one error from the mapping-error passthrough fallback, so the guard holds withCS_DEVELOPMENT__ENABLE_MAPPING_ERRORSunset — the default, and what the proxy container runs with. All the integration tests run against a default-configured Proxy; if the exemption regressed they would fail. No dependency on #436 landing first, though the two are related and #436 removes that flag entirely.The fixture
PostgreSQL was probed rather than guessed: a composite type reports
udt_name='eql_v2_encrypted'withdomain_name=NULL; a domain reports the reverse. EQL v2 shipped it as a composite, which is what the loader keys on. The fixture table carries a v3 domain column and the v2 column, reproducing the partial-migration shape rather than something adjacent to it.The tests catch the bug
Reverting
classify_columntoColumn::nativeand re-running: 7 of 8 tests fail (only the control passes), and{"secret": "value"}lands in the encrypted column in cleartext. The exposure is reproduced and then closed. The decisive assertion connects directly to PostgreSQL, bypassing Proxy, and asserts no plaintext row landed — a test that only checks the client saw an error would still pass if the write succeeded and the error came afterwards.Testing
The one integration failure is
migrate::tests::migrate_text, which fails identically before and after: it runs the migrate CLI in-process, and that CLI readsCS_SERVER__PORTto find Proxy, so it needs that variable set when the suite runs on non-default ports. Not a defect in this change.config::tandem::tests::prometheus_configis a pre-existing environment-dependent flake that also fails on the base.cargo fmtandclippy --all --all-targets -D warningsclean.CHANGELOG.mdhas a### Securityentry anddocs/errors.mddocuments the new error, which names the offending column so an operator knows what to migrate.Worth a reviewer's opinion
The startup warning is kept, but it sits under
target: SCHEMAand so is invisible unlessCS_LOG__SCHEMA_LEVEL=debug. That is unchanged from before, and it is precisely why the ticket says the only signal "has long since scrolled away" — pre-fix deployments had effectively no warning, not a quiet one. The statement-level refusal is what actually fixes visibility, so the log target was left alone as a broader logging-config decision.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.