Repository navigation
[Server] Rotate the requestState signing key with a key ring - #626
BrocksiNet wants to merge 1 commit into
Conversation
Builder::setRequestState() and RequestStateCodec take a list of keys: the first key signs, every key verifies, so an operator can put a new key in front and drop the old one once the states it signed expired, without failing the multi round-trip calls in flight. A state names no key: verification tries each key of the ring, so the wire format is unchanged and instances of an older release still verify the states a ring signs. A single key works as before.
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: 0ba97eede0
ℹ️ 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".
| * To rotate, put the new key first and keep the old one behind it until | ||
| * the states it signed have expired, one TTL later. |
There was a problem hiding this comment.
In a multi-process rolling deployment, following this instruction moves the first updated process straight to [$new, $old] while its peers still accept only $old; any new state minted there and retried on a peer is rejected. The detailed guide already requires first deploying [$old, $new] everywhere (docs/handlers/input-required.md lines 112–117), so this API documentation should include or link that learning phase instead of presenting the unsafe sequence.
Useful? React with 👍 / 👎.
chr-hertel
added
Server
There was a problem hiding this comment.
🟡 Changes recommended
The rotation recipe omits the cluster-wide key-preparation phase needed to prevent MAC failures during rollout and must tie the retirement TTL to when the last instance stops signing with the old key.
1 open issue
What changed in this PR
Adds signing-key rotation for requestState, allowing in-flight multi-round-trip calls to survive key changes while preserving the wire format and single-key compatibility.
Changes:
- Accept a validated key ring in the builder and codec: the first key signs, and every key is checked during verification.
- Add tests for rotation, invalid rings, compatibility, and retries across server configurations.
- Document the rotation procedure and update the changelog.
| File | Description |
|---|---|
tests/Unit/Server/Stateless/StatelessProtocolTest.php |
Tests retries after key rotation and retirement through the builder. |
tests/Unit/Server/Stateless/RequestStateCodecTest.php |
Tests rotation stages, ring validation, and single-key compatibility. |
src/Server/Stateless/RequestStateCodec.php |
Validates key rings, signs with the first key, and verifies against every key. |
src/Server/Builder.php |
Widens request-state configuration to accept key rings. |
docs/run/server-builder.md |
Documents key-ring support in the builder reference. |
docs/handlers/input-required.md |
Adds a three-step key-rotation guide. |
CHANGELOG.md |
Records key rotation and backward compatibility. |
Review effort: Balanced
| * To rotate, put the new key first and keep the old one behind it until | ||
| * the states it signed have expired, one TTL later. |
Summary
Builder::setRequestState()andRequestStateCodec::__construct()takestring|list<string>as$key.A list is a key ring: the first key signs, every key verifies.
hash_equals(), without an early exit.docs/handlers/input-required.mdwith the three rotation steps, and the builder example and table indocs/run/server-builder.md.The 2026-07-28 specification requires integrity protection for a
requestStatethat influences authorization or business logic (MRTR, Server Requirements).With one key, rotating it fails every multi round-trip call in flight, and on several instances each refuses the others' states until all are switched.
Design decisions
A state names no key, and verification tries each key of the ring.
The ring is two keys during a rotation and one otherwise, and an HMAC over a short body costs microseconds, so an O(1) lookup buys nothing measurable.
A key id would change the wire format, and then instances still on an older SDK release could not verify what upgraded ones sign during a rolling deployment: the very situation rotation happens in.
It also keeps "a single key is a ring of one" literally true (same wire value, asserted in a test).
setRequestStateKeys().The ring replaces the key rather than adding to it, and two methods would need a rule for when both are called.
The parameter keeps its name
$keyso named-argument callers do not break.Moving the new key straight to the front works for one process; with several, instances that have not been updated yet would refuse the new states, so step 1 (
[$current, $new]) distributes the new key before anyone signs with it.This matches the Python SDK's documented rotation.
Not strictly needed, since key position is not secret in any useful way, but it keeps the timing independent of which key signed at no real cost.
Alternatives considered
If maintainers want it later, it can be added as a versioned prefix while still accepting the two-part format.
RequestStateKeyProviderInterfacefetching keys from a secret store: more flexible, but a list covers rotation, and a provider can be layered on top later.Backward compatibility
RequestStateCodec's constructor parameter widens fromstringtostring|array; the class isfinal, so no subclass is affected.Precedents
keys[0]seals and every key unseals, with a key id for AES-GCM:src/mcp/server/request_state.py, python-sdk#3032packages/server/src/server/requestStateCodec.tsMotivation: Shopware (https://github.com/shopware/shopware) plans URL-mode elicitation on the 2026-07-28 era and runs multi-instance deployments whose signing key comes from configuration and has to be rotatable.
Fixes #624
Test plan
RequestStateCodecTest, rotation walk-through with several keys at once: a state minted with the old key verifies after the new key is added and after it moves to the front; a state minted with the new key verifies on the ring and on an instance still on step 1; a state minted while learning verifies on a not yet updated single-key instance; after the old key is removed, its state is refused withmacRequestStateCodecTest: a state signed by none of the ring's keys is refused; a single key and a ring of that key mint the same wire value and verify each other's states; an empty ring, a short key behind a good one, a non-list and a non-string key are refused at constructionStatelessProtocolTest(end to end throughBuilder): the retry of a call signed with the old key is served by a server configured with[$new, $old], and refused with-32602/400 once the ring is[$new]make tests(2959 tests, 8 skipped)make phpstanmake cscomposer validate --strictRelation to #625
This PR and #625 (binding
requestStateto its originating request) change the samesetRequestState()signature and codec and compose:setRequestState(string|array $key, int $ttl = 600, ?RequestStateBinderInterface $binder = null), with the binding covered by the MAC under every key of the ring.Whichever merges second needs a mechanical conflict resolution, which I have ready and tested (unit suite, phpstan, cs), including a test that a bound state survives a key rotation.