Add ES256 signing support for MCP access tokens#191
Conversation
mcp.signingAlgorithm now accepts 'ES256' (EC P-256) alongside the RS256 default. Generated keys (first boot and rotation) follow the configured alg, and changing it on a running deployment rotates in a new key while old keys stay in the JWKS until retired — additive, no migration. Pinned keys (mcp.signingKeyPem) derive their alg from the key material (RSA or EC P-256; anything else throws at mint); an EC pin never claims the legacy rs256-default kid. Verification pins algorithms to the selected key's declared alg, and JWK serialization branches on key type. AS metadata advertises the effective alg (pin-aware). EdDSA remains deferred (jsonwebtoken cannot emit it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185AoLyjuZUuKAwg9NxUDiq
This comment has been minimized.
This comment has been minimized.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the ES256 (EC P-256) signing algorithm for Model Context Protocol (MCP) OAuth access tokens, alongside the existing RS256 algorithm. Key changes include updating configuration schemas, implementing EC keypair generation and rotation in the MCPKeyStore (with a rolling-deployment age floor to prevent churn), updating the token issuer to sign and verify ES256 tokens, and serializing EC public keys to JWK format. Additionally, configuration validation warnings have been added, and comprehensive unit tests have been introduced to cover these new capabilities. There are no review comments to address, and the implementation appears robust and well-tested, so I have no further feedback to provide.
|
Reviewed; no blockers found. |
|
Re the gemini-review suggestion on 🤖 Posted by Claude on Nathan's behalf |
The ternary + spread-encoding form made promisify's generateKeyPair overloads resolve to KeyObject, so the 'as string' casts failed tsc --noEmit (masked in CI by `tsc || true`). Inline literal options per branch restore the string-returning overloads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185AoLyjuZUuKAwg9NxUDiq
The integration guidance still described tokens/JWKS as RS256-only and promised the fixed rs256-default kid (stale since UUID-kid multi-key publication). Reworded to alg-neutral or 'RS256 or ES256', and the issued-token section now tells integrators to resolve the kid against the JWKS instead of assuming a fixed key id. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185AoLyjuZUuKAwg9NxUDiq
heskew
left a comment
heskew
left a comment
There was a problem hiding this comment.
Three actionable issues found in the ES256 transition and pinned-key compatibility paths. The first two should be addressed before merge; the third is a narrower upgrade edge case.
🤖 Posted by Codex on Nathan's behalf.
… legacy EC pin repair P1: AS metadata now advertises the union of the effective alg and the algs of keys still live in the JWKS (builder is async; the handler already awaited), so a damper-deferred RS256 signer is not misdescribed as ES256-only during a switch. P2: interval rotation is skipped while the newest key's alg disagrees with config — the deferred alg switch owns that state, so a short keyRotationInterval can no longer bypass the 300s floor. P3: a matching pinned row now has its alg re-derived from the key material and repaired in place — pre-ES256 code persisted every pin as RS256, leaving an EC pin durably unusable after upgrade. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0185AoLyjuZUuKAwg9NxUDiq
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Closes #127 (the ES256 leg; EdDSA stays deferred by design —
jsonwebtokencannot emit it, and per the issue it should only be implemented when a specific client needs it).Summary
mcp.signingAlgorithmnow acceptsES256(EC P-256) alongside theRS256default, exactly as sketched in #127 — no new dependency:algcolumn already existed per-row, so mixed key sets are schema-compatible and the change is additive (old keys stay in the JWKS until retired, outstanding tokens keep verifying, no migration).mcp.signingKeyPem) derive their algorithm from the key material (RSA → RS256, EC P-256 → ES256; anything else throws at mint). An EC pin never claims the legacyrs256-defaultkid.algorithmsto the selected key's declared alg (still blocksalg: noneand cross-alg substitution);publicKeyToJwkbranches on the exported key type (EC →crv/x/y).signingAlgorithmvalue.Where to look
keyStore.ts(getSigningKey) is the one genuinely new behavior with design latitude — the floor (300s) bounds rolling-rollout churn to one rotation per direction per interval instead of per-mint. Flagged by the cross-model review; alternative was documenting "roll out atomically" only.algFromPrivateKeyPemdecides a pinned key's algorithm from material, silently overridingsigningAlgorithm(with a startup warning on mismatch) — confirm that precedence reads right.Review notes
Cross-model review ran (Codex + Claude domain pass; the Gemini leg was unavailable in this environment, so no second-model perf lens). No blockers; the rolling-rollout thrash concern was addressed with the age floor above, and the unrecognized-value silent-RS256 fallback now warns at startup.
1095 unit tests pass (21 new: ES256 sign/verify/JWKS, key-store generation/switch/damper/pin, metadata advertisement, helper validation).
Generated by an LLM (Claude Fable 5) pairing with @heskew.
🤖 Generated with Claude Code
https://claude.ai/code/session_0185AoLyjuZUuKAwg9NxUDiq