Repository navigation
fix(proxy): reset keyset and mapping settings on DISCARD ALL and RESET (2.2.x, CIP-5021) - #475
Merged
Merged
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd6e39b7e
ℹ️ 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".
tobyhede
force-pushed
the
claude/project-thread-gvoja6-2.2
branch
from
October 19, 2026 05:59
2077279 to
ff6d54a
Compare
…T (CIP-5021) A connection pool that resets a connection between clients with DISCARD ALL or RESET ALL handed the next client the previous client's keyset, and left UNSAFE_DISABLE_MAPPING on so its queries were sent unencrypted. When PostgreSQL confirms DISCARD ALL or RESET ALL, Proxy now clears the session keyset (falling back to default_keyset_id) and turns UNSAFE_DISABLE_MAPPING off. RESET CIPHERSTASH.<setting> and SET CIPHERSTASH.<setting> TO DEFAULT reset just that setting, and SET CIPHERSTASH.KEYSET_NAME TO DEFAULT no longer selects a keyset named "DEFAULT". Resets use the same batch mechanism as SQL deallocations: each one is queued with the generation of the batch it was sent in, applied on the matching RESET, SET or DISCARD ALL CommandComplete, and discarded by the batch's ReadyForQuery if PostgreSQL rejected it. A simple Query queues one entry per SET or DISCARD ALL when any of them resets a Proxy setting; a prepared SET, RESET or DISCARD ALL carries its reset onto the portal and queues it on each Execute, including SETs that reset nothing, so each tag in a pipelined batch lines up with its statement. Per-setting change counts keep a late reset from undoing a SET the client sent after it. The bundled SQL parser has no RESET statement, so Proxy rejected every RESET as invalid SQL. SqlParser::parse_resets recognises a query made only of RESET statements and is tried only when the normal parse fails. UNSAFE_DISABLE_MAPPING is now shared by the frontend and backend Context clones, so the backend can reset it.
… on (CIP-5021) Cover DISCARD ALL, RESET ALL, RESET CIPHERSTASH.<setting> and SET CIPHERSTASH.<setting> TO DEFAULT through both the simple and the extended protocol, and add the changelog entry.
…s (CIP-5021) Proxy applied SET CIPHERSTASH.KEYSET_ID, KEYSET_NAME and UNSAFE_DISABLE_MAPPING once, when the statement was parsed. A named prepared SET that ran again after RESET ALL left Proxy reset while PostgreSQL had the setting again, so encrypted queries could use no keyset or the default keyset instead of the one the client selected. Record each prepared SET CIPHERSTASH statement and apply it again on every Bind that Proxy accepts, with mapping enabled or disabled. The change is counted under the settings lock like any other SET, so a reset still pending at the time does not undo it. The record is removed with the statement by Close, DEALLOCATE or DISCARD ALL.
tobyhede
force-pushed
the
claude/project-thread-gvoja6-2.2
branch
from
October 19, 2026 06:36
ff6d54a to
956c9b3
Compare
freshtonic
reviewed
Oct 20, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
The design is right. Resets apply only after PostgreSQL confirms them, they are scoped by generation, and the per-setting change counts stop a late reset from undoing a later SET. I mutated each guarantee, and in each case a test failed. But I found one fail-open path that I think must be fixed before merge (finding 1).
I also checked the blocking findings from my #473 review on the current release/2.2. B1 (same-batch DEALLOCATE), B2 (startup ReadyForQuery), N3 (COPY Syncs) and N5 (ErrorResponse pops the next batch's Execute) are all fixed. When I broke each fix, an existing test failed. Thanks.
Please fix before merge
- Fail-open: a reset is lost when mapping is disabled and the previous batch's ReadyForQuery arrives between Bind and Execute. (
frontend.rs:1088-1100,context/mod.rs:914-928)- When mapping is disabled, the Bind early-return path records
portal_resetsbut does not calladd_portal. - At ReadyForQuery
I,discard_portals_beforeremoves everyportal_resetsentry whose name is not inportals. This includes the portal just bound for the next batch, so the Execute queues nothing. - Scenario (I confirmed it with a throwaway unit test):
- Prepare
r=RESET ALL. Set a keyset and disable mapping. - The client sends
Query("SELECT 1"), thenBind("", "r"). - The server sends
CommandCompleteand ReadyForQueryI. - The client sends
Execute("")andSync. The server sendsBindComplete,CommandComplete("RESET")and ReadyForQuery.
- Prepare
- Result:
unsafe_disable_mapping()is stilltrueand the keyset is still set, but PostgreSQL has reset both. The next client's queries go to PostgreSQL unencrypted, which is the leak that this PR closes. - Suggested fix: on the mapping-disabled Bind path, also call
add_portal(bind.portal, Portal::passthrough(session_id)). Better, pruneportal_resetsandportal_deallocationsby the generation of each entry, not by membership ofportals. Please add the scenario as a regression test. The same pruning also dropsportal_deallocations, but that only leaks a statement.
- When mapping is disabled, the Bind early-return path records
Non-blocking
-
In a simple Query, a
SET … TO DEFAULTundoes a SET that comes after it in the same Query. (frontend.rs:206)add_settings_resetsruns afterquery_handler, so the counts are read after the loop at:463-479has applied everySET CIPHERSTASH.*.- With keyset k1 set,
SET CIPHERSTASH.KEYSET_ID TO DEFAULT; SET CIPHERSTASH.KEYSET_ID = 'k2'gives k2 in PostgreSQL, butNonein Proxy. Proxy then usesdefault_keyset_idor fails. I confirmed this with a throwaway test. - Main (#474) reads the counts before the loop (
middleware/frontend.rs:425). That makes this order correct on main but the reverse order wrong:SET …UNSAFE_DISABLE_MAPPING = true; SET …UNSAFE_DISABLE_MAPPING TO DEFAULTleaves mapping disabled after PostgreSQL resets it, which fails open. I found this by reading the code and did not run it. - Suggested fix for both branches: read the counts for each reset at its position in the loop, queue the entries after the loop, and test both orders.
-
A keyset SET sent before a mapping reset is confirmed is ignored. (
frontend.rs:472-479,:841,:961)- While mapping is disabled, Proxy skips
handle_set_keyset. A RESET orTO DEFAULTnow turns mapping on only at confirmation, so a keyset SET pipelined before then is dropped. - With keyset k1 set and mapping disabled, pipeline
Query("RESET CIPHERSTASH.UNSAFE_DISABLE_MAPPING")andQuery("SET CIPHERSTASH.KEYSET_ID = 'k2'"). Mapping turns on, but Proxy still has k1 while PostgreSQL has k2. I confirmed this with a throwaway test. - Suggested fix: apply keyset SETs while mapping is disabled, because they only record a setting. At a minimum, record them while a mapping reset is pending.
- While mapping is disabled, Proxy skips
-
Nit:
add_settings_resets(context/mod.rs:1027-1033) returns early if the mutex is poisoned. Then it drops the entry and the batch's tags go out of step. This needs a panic first, so it is theoretical. Please queue the entry anyway.
Codex comments
- "Reapply prepared SET effects on every execution": 956c9b3 fixes this, and I verified it.
- "Restore proxy settings when reset transactions roll back": I agree that it can be a follow-up (CIP-5058). Please record in CIP-5058 that a rolled-back RESET now clears the keyset, so Proxy uses
default_keyset_idwhile PostgreSQL still has the tenant keyset.
Out of scope (older code, found by reading, not run): in query_handler, when any statement in a Query is rewritten, the forwarded SQL is rebuilt only from transformed_statements. So a SET or DISCARD that took a continue (:472-483) is not sent to PostgreSQL. For this PR, a reset in a Query that also has an encrypted INSERT never reaches PostgreSQL, but Proxy discards its entry, so the two stay consistent. Please consider a separate ticket for this.
Address review of the 2.2 backport: - A reset bound for the next batch while PostgreSQL was still answering the previous one was lost. Completing the earlier execution closed the unnamed portal and removed the newer Bind's reset, and with mapping disabled the ReadyForQuery cleanup also dropped it because the Bind adds no portal. Portal effects now carry the generation they were bound in, are pruned by generation, and an execution only removes the effects bound at or before it. - In a simple Query, each reset now records the settings change counts at its own position, so it undoes the SETs before it but not a SET after it in the same Query. - A keyset SET sent while mapping is disabled is recorded when no default keyset is configured, so a SET pipelined before PostgreSQL confirms a mapping reset is not lost. - Resets are queued even if the change-count lock is poisoned, so the batch's command tags stay in step.
…s main does (CIP-5021) A SET CIPHERSTASH.KEYSET_ID that is not a UUID, sent while mapping is disabled and no default keyset is configured, now returns the same error as with mapping enabled instead of being logged and forwarded.
…s (CIP-5021) Proxy applied SET CIPHERSTASH.KEYSET_ID, KEYSET_NAME and UNSAFE_DISABLE_MAPPING when the statement was parsed, as well as each time it ran. A client that prepared such a SET without executing it changed Proxy's setting while PostgreSQL's stayed the same, so writes could be encrypted under a keyset the client never selected. Apply the setting only when a portal is bound from the statement.
…P-5021) When a statement fails type checking and Proxy forwards the whole Query, the SET CIPHERSTASH statements after it are now applied, in order with their resets, as PostgreSQL runs them. Also record in a test and the CHANGELOG that an invalid KEYSET_ID is now refused while mapping is disabled, and that a prepared SET takes effect only when executed.
Applying a prepared SET CIPHERSTASH setting only when it runs moved the refusal of a keyset SET, with a default keyset configured or an invalid KEYSET_ID, from Parse to Bind. The set_keyset_error integration tests expect it at Parse. Check the SET at Parse without applying it, so it is refused as before.
Contributor
Author
Review feedback audit at
|
tobyhede
added a commit
that referenced
this pull request
Oct 22, 2026
…tions (CIP-5073) Backport the transactional settings behaviour of #469 (CIP-4167) to release/2.2, for CIP-5058. Proxy kept CIPHERSTASH.KEYSET_ID, KEYSET_NAME and UNSAFE_DISABLE_MAPPING outside PostgreSQL's transactions, so a rolled-back reset left Proxy with no keyset (and later writes under default_keyset_id), a rolled-back SET of UNSAFE_DISABLE_MAPPING = true left mapping disabled, and SET LOCAL was treated as a plain SET. The settings now follow PostgreSQL's rules: - context/settings.rs is ported from main. ConnectionSettings keeps session and local values with transaction and savepoint snapshots: COMMIT keeps changes, ROLLBACK and ROLLBACK TO SAVEPOINT undo them, RELEASE keeps them, SET LOCAL ends with the transaction, and a failed transaction, explicit or implicit, is restored when it ends. - Every settings command, including BEGIN, COMMIT, ROLLBACK and SAVEPOINT, is applied only when PostgreSQL answers it. Each statement of a Query that contains one, and every Execute, is queued by batch generation and matched to its own CommandComplete, EmptyQueryResponse or PortalSuspended, in order. A COMMIT answered with ROLLBACK rolls back, and an ErrorResponse skips the rest of the batch and fails the transaction. This replaces the per-setting change counters and reset queue added in #475, and applying a prepared SET at Bind. - The frontend maps each statement with the settings projected over the commands PostgreSQL has not answered yet, including the statements before it in the same Query, and encrypts with that keyset. The backend decrypts with the confirmed keyset. - Setting names are matched as PostgreSQL matches them, UNSAFE_DISABLE_MAPPING accepts every PostgreSQL boolean spelling, and an unknown CIPHERSTASH setting or a value Proxy cannot interpret is refused, as on main. RESET of either keyset setting clears the keyset, as on main. - A simple query that Proxy rewrites now forwards every statement. Statements Proxy did not map, such as a SET, were dropped from a rewritten query, which would also have put the responses out of step with the queued settings commands. The pooler test matrix, the settings protocol journal and the pipelining barriers of #469 are specific to main's architecture and are not ported. Tests: the settings.rs unit tests from #469, transaction tests for the frontend and backend together, and #478's rolled_back_mapping_changes_ are_undone and multitenant rolled_back_keyset_changes_are_undone integration tests. The #475 tests still pass; the two that expected a reset to survive a failed implicit transaction now expect it to roll back, as on main. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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
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.
Backport of CIP-5021 to the 2.2 line. Stacked on #473 (the 2.2 connection-state leak fix); the main-branch fix is #474.
Before: a connection pool that reset a connection between clients with
DISCARD ALLorRESET ALLhanded the next client the previous client's keyset, and leftCIPHERSTASH.UNSAFE_DISABLE_MAPPINGon, so the next client's queries were sent unencrypted. Proxy rejected everyRESETstatement as invalid SQL.SET CIPHERSTASH.<setting> TO DEFAULTleft the setting as it was, or forKEYSET_NAMEselected a keyset named "DEFAULT".After: when PostgreSQL confirms
DISCARD ALLorRESET ALL, Proxy clears the session keyset and turnsUNSAFE_DISABLE_MAPPINGoff. Proxy then usesdefault_keyset_id, or requires a keyset again when there is none.RESET CIPHERSTASH.KEYSET_ID/KEYSET_NAME/UNSAFE_DISABLE_MAPPINGandSET CIPHERSTASH.<setting> TO DEFAULTreset only that setting. Names match case-insensitively, including a quoted name with the period. A reset that PostgreSQL rejects, for example in an aborted transaction, changes nothing. Both the simple and the extended protocol are covered.How
RESET/SET/DISCARD ALLCommandComplete. If PostgreSQL never confirms it, the batch'sReadyForQuerydiscards it.SET/DISCARD ALLstatement, so each tag lines up with its own statement. It queues nothing when none of them resets a Proxy setting.SET/RESET/DISCARD ALLrecords its reset on the statement. Bind carries it onto the portal, and every Execute queues it.SETs that reset nothing. Main tracks each operation separately, but 2.2 has one queue per connection, so this keeps tags in step when a batch has severalSETs.RESETstatement.SqlParser::parse_resetsrecognises a query made only ofRESETstatements, and Proxy calls it only after the normal parse fails.unsafe_disable_mappingis now anArc<AtomicBool>shared by the frontend and backendContextclones.SET CIPHERSTASH.*the client sent after the reset.postgresql/tests.rs, plus parser and command-tag tests. They drive a realFrontendandBackendover in-memory streams with wire messages.pool_reset_turns_mapping_back_onandpool_reset_clears_keyset_id.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