Repository navigation
test(proxy): cover settings resets and SET ordering cases (CIP-5021) - #480
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: bb3b0f8d2f
ℹ️ 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".
| mapping_disabled | ||
| ); | ||
| } | ||
| self.apply_set_keyset(&statement)?; |
There was a problem hiding this comment.
When mapping is disabled and no default keyset is configured, this call applies a keyset change while handling Parse, even though PostgreSQL only creates the prepared statement at that point. A client can parse SET CIPHERSTASH.KEYSET_ID = K2 without ever executing it, reset mapping, and then have subsequent writes encrypted under K2 while PostgreSQL and the client still expect the previous keyset. Record the prepared setting here, but apply it only when its portal is actually executed.
Useful? React with 👍 / 👎.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
The fix is correct. Each reset now takes its change counts at its position in the statement loop, and the queue keeps one entry for each CommandComplete. Thus a SET and a reset in one Query apply in the order that the client wrote them, and the fail-open case (= true; ... TO DEFAULT) is closed. I ran cargo test -p cipherstash-proxy --lib settings_reset on bb3b0f8: 32 passed. The new tests cover both orders for the keyset and for mapping, and the three pipelined protocol paths.
I have two non-blocking comments (see inline):
- While mapping is disabled and there is no
default_keyset_id, Proxy now parses a keyset SET. An invalid keyset UUID now gives an error and stops the whole Query. Before this change, Proxy forwarded it. - The type-check passthrough fallback does not apply SETs that come after the failed statement. This problem existed before this PR, but this PR changes the same block.
I think this PR is ready to merge after CI passes.
| /// it is in place if a reset not yet confirmed turns mapping back on. With a default keyset | ||
| /// it is ignored instead, as it would be refused, so that a connection with mapping | ||
| /// disabled can still set it. | ||
| fn apply_set_keyset(&mut self, statement: &ast::Statement) -> Result<(), Error> { |
There was a problem hiding this comment.
Non-blocking. While mapping is disabled and there is no default keyset, this function now calls handle_set_keyset, and thus maybe_set_keyset_id. That function returns KeysetIdCouldNotBeParsed for a value that is not a UUID. Before this change, Proxy ignored the statement and forwarded it, and PostgreSQL accepted it, because a custom GUC accepts any string. Now Proxy rejects the whole simple Query, including the other statements in it.
I think that this is acceptable: the value cannot be used when mapping is enabled again. But it changes the "passthrough" behavior of a connection with mapping disabled, and no test or CHANGELOG line records it. Please add a test, or a sentence in the CHANGELOG, so that this change is intentional.
| if self.context.mapping_errors_enabled() || err.must_fail_closed() { | ||
| return Err(err); | ||
| } else { | ||
| // The whole Query is forwarded, so the resets after this statement |
There was a problem hiding this comment.
Non-blocking, and it existed before this PR. This block forwards the whole Query, so the resets after index are correct. But the loop stops here, so Proxy does not apply the SET CIPHERSTASH.* statements after index. Example: SELECT <fails type check>; SET CIPHERSTASH.KEYSET_ID = '<b>'. PostgreSQL then holds keyset b, but Proxy keeps the old keyset. The same problem occurs with UNSAFE_DISABLE_MAPPING. The reset counts that you take here are consistent with this, because those SETs never change the counters.
CIP-3680 removes this fallback. If CIP-3680 will not merge soon, a small loop over parsed_statements[index + 1..] can apply the SETs and take the resets in order. That loop can replace the filter_map here.
|
@freshtonic - this still needs approval |
Port the tests from the CIP-5021 follow-up onto the settings redesign that main now carries. They cover a prepared SET that is never executed, a SET and a reset of the same setting in one query applied in order, a SET sent before an earlier reset is confirmed, a reset bound before an earlier Query completes, SETs after a statement that fails type checking, and refusal of an invalid or disallowed keyset SET. The redesign already gives each of these the expected behaviour, so this change adds tests only. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
bfc3491 to
4919fef
Compare
tobyhede
changed the title
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Re-reviewed bb3b0f8..bfc3491. Approved. The three new commits address the Codex P1 and my two inline comments, and all CI checks pass.
- Prepared SET applies when it runs (6c7db3f). Parse now only checks the SET (
check_set_keyset), andapply_statement_settingapplies it at Bind.a_prepared_set_that_is_never_executed_changes_nothingand the mapping-disabled variant cover the Codex case.check_set_keysetrefuses the same statements ashandle_set_keyset: a keyset SET withdefault_keyset_id, an invalid UUID, and it skipsTO DEFAULT, so a refusal still comes at Parse, as before. - SETs after a statement that fails type checking (4da797a). The fallback loop now applies the SETs and takes the resets in statement order, the same as the main loop.
sets_after_a_statement_that_fails_type_checking_still_applycovers it. - Invalid keyset SET while mapping is disabled (bfc3491). The behaviour change now has a test (
an_invalid_keyset_id_is_refused_while_mapping_is_disabled) and a CHANGELOG sentence. Thanks.
Non-blocking:
- The setting is applied at Bind, not at Execute. A Bind that PostgreSQL rejects (for example, in an aborted transaction) or a portal that is never executed still changes Proxy's setting. This is a smaller window than before (apply at Parse), and it is the same as the 2.2 backport, so a follow-up is fine. Please mention it in CIP-5058.
- Older code, found by reading, not run: the simple-Query loop (and now the fallback loop) applies each
SET CIPHERSTASH.*as it goes. If a later statement in the same Query returns an error, the Query is refused and PostgreSQL never runs it, but Proxy keeps the earlier SET. Example:SET CIPHERSTASH.UNSAFE_DISABLE_MAPPING = true; SET CIPHERSTASH.KEYSET_ID = 'not-a-uuid'leaves mapping disabled in Proxy only. In the same way, withdefault_keyset_idconfigured,maybe_set_keysetstores the keyset beforehandle_set_keysetreturnsUnexpectedSetKeyset, so a refused keyset SET still changes the keyset thatencrypt/decryptuse. Please check both, and consider applying a Query's settings only after every statement in it is accepted. A separate ticket is fine.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approved at 4919fef. Please ignore my previous review text: it described bb3b0f8..bfc3491, and those commits are no longer on this branch. The two non-blocking notes in it were about code that the settings redesign from #469 replaced.
After the rebase, this PR adds only tests (settings_reset module, +269, no production code). They cover the cases from my #475 review on main's new settings code:
- a SET and a reset of the same setting in one Query, in both orders, for the keyset and for mapping. This includes
UNSAFE_DISABLE_MAPPING = true; … TO DEFAULT, the fail-open order; - a keyset SET sent before a mapping reset is confirmed, through a simple Query, an extended Query and a prepared SET;
- a prepared reset bound before an earlier Query completes, with mapping enabled and disabled (the fail-open case from #475);
- a prepared SET that is never executed, SETs after a statement that fails type checking, and refusal of an invalid or disallowed keyset SET.
I ran cargo test -p cipherstash-proxy --lib settings_reset on 4919fef: 38 passed. I agree with dropping the test that expected a keyset SET to be forwarded while mapping is disabled with default_keyset_id, because #469 refuses it on purpose. CI had not started on this head when I approved.
3 participants
Requested by Toby
This was the main-branch follow-up to freshtonic's review of the 2.2 backport (#475). It is now rebased onto main, which has the settings redesign from #469.
Before. The fixes in this PR changed the CIP-5021 reset code that #469 has since replaced. The cases they handled had no tests on main: a prepared SET that is never executed, a SET and a reset of the same setting in one query, a SET sent before an earlier reset is confirmed, a reset bound before an earlier Query completes, SETs after a statement that fails type checking, and invalid or disallowed keyset SETs.
After. Main already behaves correctly in all of these cases, so this PR now adds only the tests, in the
settings_resetmodule. There is no production code change.How. I ported the tests onto main and ran them. 14 of the 15 tests pass unchanged. The one that failed expected a keyset SET to be forwarded unchanged while mapping is disabled and
default_keyset_idis configured. #469 deliberately refuses that SET instead, so I dropped that test, and the changelog lines it needed are gone too.Generated by Claude Code