feat: signing-key rotation + multi-key JWKS publication (#128) by heskew · Pull Request #158 · HarperFast/oauth

heskew · 2026-07-06T17:32:29Z

Summary

Implements #128 — signing-key rotation + multi-key JWKS publication.

harper_oauth_mcp_keys now holds a set of keys. Signer = newest created_at (kid-desc tie-break). The legacy rs256-default row remains a valid member — no migration; existing tokens keep verifying.
JWKS publishes all persisted public keys — this also fixes the clustered first-boot race (every node's key is published, so cross-node tokens verify regardless of which node signed). mcp.signingKeyPem is now recommended for clusters, no longer required.
Opt-in rotation: new mcp.keyRotationInterval (seconds; unset/0 = no rotation). Checked lazily at token mint — no timers. signingKeyPem pins the key: rotation is skipped with a startup warning when both are set.
GC: a retired key is deleted once its immediate successor is older than 2 × accessTokenTtl (the last token it signed has expired, with a full TTL of margin for replication lag). Runs detached from minting; errors logged, never blocking.
First-boot keys get a UUID kid (pinned PEM keeps the fixed rs256-default kid — identical content on every node makes the shared primary key safe there, and only there).
getSigningKey propagates table-read errors (mint fails loudly) instead of treating them as an empty table and generating spurious keys; the JWKS read path stays best-effort ([]).

buildJWKS and withMCPAuth/verifyAccessTokenWithKeySet needed no changes — both were already key-set-shaped (selection by kid).

Where to focus review

GC rule (keyStore.ts garbageCollect): successor-based windows. Round-1 review (Gemini/Claude/Codex all) caught that a signer-age rule never fires when keyRotationInterval < 2×accessTokenTtl (accumulation); fixed + regression-tested. Known caveat: the 2× margin assumes accessTokenTtl wasn't drastically shortened between rotations — a token minted under a much longer previous TTL could theoretically outlive the window of a later, shorter TTL. Documented tradeoff; operators changing TTL sharply downward should expect old-key tokens to be cut off at rotation + 2×newTTL.
First-boot kid collision (Codex round-1 P1): two unpinned nodes racing an empty table previously both wrote rs256-default, one overwriting the other — stranding the loser's tokens with an unverifiable kid. Generated keys now use UUID kids so both rows survive and publish.
Rotation/pin interplay: pin wins; rotation + GC only run unpinned.

Tests

828 total / 826 pass / 2 pre-existing skips. Covers: multi-key publication + newest-wins + tie-break, rotation on/off/fresh, old-key token verifying against the post-rotation key set (overlap window), pin-wins, GC successor rule + no-accumulation regression + signer never deleted + GC-error tolerance, enumerate-error propagation (mint) vs best-effort (JWKS), legacy rs256-default compat.

Docs: docs/mcp-oauth.md + configuration reference updated (keyRotationInterval, multi-key JWKS, revised cluster guidance).

Closes #128

🤖 Generated with Claude Code (Opus 4.8)

Replaces the v1 singleton-key approach with a full key-set model: - `getAllPublicKeys()` now enumerates all rows via `table.search({})` and publishes every key at the JWKS endpoint. Cross-node tokens verify immediately — the clustered first-boot race is resolved without requiring `mcp.signingKeyPem` on every node (though pinning still works and is recommended for zero-race deployments). - `getSigningKey()` selects the newest key by `created_at` (tie-break: `kid` descending). The legacy `rs256-default` row is a valid set member; no migration. - New `mcp.keyRotationInterval` config (seconds): when set and > 0, a fresh UUID-kid RS256 keypair is lazily generated at token mint time once the newest key's age exceeds the interval. Old keys are GC'd once no token they signed can be valid (`2 × accessTokenTtl` after a newer key's creation). - `mcp.signingKeyPem` + `mcp.keyRotationInterval` are mutually exclusive: pin wins; both-set logs a startup warning. - Extended `Table` interface with `search(query)` returning `AsyncIterable`. - Updated token.test.js and tokenAuditHook.test.js mock tables with `search`. - 17 new keyStore tests covering multi-key, rotation, no-rotation default, pin-wins, GC thresholds, GC error resilience, and legacy compat. - `withMCPAuth` and `buildJWKS` required no changes; verified they handle multi-key sets correctly via existing `verifyAccessTokenWithKeySet` kid selection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions · 2026-07-06T17:34:47Z

Reviewed; no blockers found.

The PR successfully implements robust signing-key rotation and multi-key JWKS publication for the MCP OAuth flow. The successor-age based retirement logic correctly handles token overlap windows, and the pinning support (signingKeyPem) provides a reliable migration path for clustered deployments. The implementation follows Harper v5 conventions, specifically avoiding Proxy-spread pitfalls and ensuring data-integrity through single-flight write guards and convergent re-enumeration.

Suggestions (non-blocking)

src/lib/mcp/keyStore.ts:192 — Consider adding a read-side single-flight guard to enumerateKeys. While the 5s TTL limits frequency, a cold-cache spike could trigger N concurrent table scans before the first one refills the cache.

gemini-code-assist

Code Review

This pull request implements automatic signing-key rotation and multi-key JWKS support for MCP OAuth, allowing nodes to lazily rotate keys and garbage-collect expired ones. Feedback on these changes highlights that getAllPublicKeys passes a raw error object to the logger, violating codebase conventions, and that safeEnumerate silently swallows errors, which could lead to silent failures or infinite key generation loops.

claude · 2026-07-06T17:39:20Z

Reviewed; no blockers found.

…umerate errors (review) Three review findings on #158: - GC now keys each retired key off its IMMEDIATE SUCCESSOR's created_at (Gemini/Claude/Codex blocker): the signer-age rule never fired when keyRotationInterval < 2*accessTokenTtl (signer always young), so retired keys accumulated unbounded. Successor rule ages keys out independently; conservative if a successor was itself GCd. - Generated first-boot keys get a UUID kid (Codex P1): two nodes racing an empty table both wrote rs256-default, one overwriting the other and stranding the loser's tokens with an unverifiable kid. Only the pinned signingKeyPem path keeps the fixed kid (identical content everywhere). - getSigningKey PROPAGATES enumeration errors instead of treating them as an empty table (Gemini high): a transient read error no longer mints spurious keys; JWKS reads stay best-effort ([]). Removed the safeEnumerate indirection that made getAllPublicKeys' catch dead code (Claude nit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

heskew

I found two issues worth fixing or clarifying before merge:

src/lib/mcp/keyStore.ts:182-190 only consults mcp.signingKeyPem when the keys table is empty. If an unpinned node has already minted a UUID key, adding the pinned PEM later never persists or selects rs256-default; it just disables rotation while the existing generated key continues signing indefinitely. That contradicts the new "pin wins" / "one canonical key" guidance. Either make the configured PEM become the signer when present, or explicitly document/diagnose that signingKeyPem must be set before the first token/key is minted.
docs/configuration.md still looks stale: the MCP option table does not list mcp.keyRotationInterval, the mcp.signingKeyPem row still warns that cluster nodes can sign with keys the JWKS has not published, and the checklist still says clusters should pin signingKeyPem. That reference should match the new multi-key JWKS and rotation behavior.

🤖 Posted by Codex (gpt-5.5) on Nathan's behalf

…docs Round 3 of PR #158 (issue #128, signing-key rotation + multi-key JWKS). Pin-wins-always: `mcp.signingKeyPem` now wins as signer regardless of what is already in the table — not just on first boot. `getOrPersistPinnedKey` searches all keys by `public_key_pem` string equality; if the pinned key is absent it persists it under `rs256-default` (when that row is free) or under a deterministic fingerprint kid `pinned-<sha256 first 16 hex>` (when `rs256-default` holds different material). Concurrent clustered puts against the same fingerprint kid are idempotent. Old keys remain in the table for JWKS overlap. setImmediate GC: all `garbageCollect` dispatches are now wrapped in `setImmediate(() => { ... })` so GC never holds the request's transaction context. Tests continue to use a single `await new Promise(r => setImmediate(r))` which is sufficient because Node.js drains the microtask queue between each setImmediate callback. Tests (keyStore.test.js): five new pin-semantics tests covering post-boot pinning, legacy-material match (no new row), fingerprint-kid with conflicting rs256-default, determinism (same PEM → same kid across store instances), and idempotency. Updated the old "pin wins over rotation" test to reflect the new behavior (pinned key persisted alongside old key, table size 2). Docs (configuration.md): added `mcp.keyRotationInterval` option row; updated `mcp.signingKeyPem` description to reflect pin-wins-always semantics and that pinning is recommended (not required) in clusters thanks to multi-key JWKS; updated the withMCPAuth security callout to reflect the same. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

heskew · 2026-07-06T18:21:01Z

Both findings addressed in 18dbed7 (plus the Gemini GC-detach suggestion):

Pin now wins always, not just at first boot. getSigningKey takes a dedicated pin path when mcp.signingKeyPem is set: it finds the pinned key by public-key material match (so legacy pinned rs256-default rows are recognized without a rewrite) and uses it as signer regardless of created_at. If absent, it persists the pinned key — under rs256-default when that row is free, otherwise under a deterministic fingerprint kid (pinned-<sha256[:16]>) so a legacy row holding different material is never overwritten (its tokens keep verifying) and concurrent writes from clustered nodes are idempotent. Existing generated keys stay in the table/JWKS for overlap. 5 new tests cover post-boot pinning, legacy-row match, fingerprint-kid conflict, cross-instance determinism, and idempotency.
docs/configuration.md updated: mcp.keyRotationInterval added to the MCP option table; the signingKeyPem row no longer claims nodes can sign with unpublished keys (multi-key JWKS made that obsolete — pinning is now recommended, not required); the cluster checklist and the withMCPAuth callout updated to match.
GC dispatch is wrapped in setImmediate so it can't hold the request's transaction context.

830 tests, 0 failures.

🤖 Posted by Claude on Nathan's behalf

Gemini suggestion on #158 applied to all three post-write re-enumeration fallbacks (pinned, first-boot, rotate) so transient table read failures are diagnosable in clustered environments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

kriszyp · 2026-07-07T21:41:29Z

Reviewed via Claude review-queue (full review: oauth-158-8f2ffee.md). Core rotation/overlap semantics are correct — old keys stay published until every token they signed expires, kid matching is untouched and sound, private key material is never exposed or logged. Three issues worth fixing before merge, all confirmed independently (also flagged by Gemini in an earlier pass):

Thundering-herd key generation on the mint path (keyStore.ts:202) — getSigningKey now enumerates the whole table and, on an empty table or a stale newest key, generates+persists a new keypair with no mutex, no debounce, no in-flight guard. A burst of concurrent mint requests at first boot (empty table) or exactly at the rotation-interval boundary will each independently kick off RSA keygen, saturating libuv's threadpool and leaving N redundant keys in the JWKS. This re-introduces at the single-node level exactly the race this PR claims to fix at the cluster level.

GC leaks a superseded, non-signer key permanently (keyStore.ts:451) — the retirement loop starts at i = 1, treating sorted[0] as always-safe on the assumption it's the signer. But the pin path selects the signer by material match against the configured PEM, not by created_at — so if a pin adopts an older created_at than an existing generated key, and any later key lands newer than the pinned signer, that key sits at sorted[0], is never the signer, and is never visited by the loop. Confirmed in isolation: the loop never reaches index 0 regardless of threshold. Fix: iterate i = 0..length and special-case the signer via the existing key.kid === signerKey.kid continue, not via loop bounds.

getAllPublicKeys() full-table-scan on every authenticated request, no caching (withMCPAuth.ts:247) — v1 was a single get() by primary key; verification now scans the whole table on every MCP-authenticated call, not just mint, and gets worse as the first two issues let more keys accumulate. A short-TTL in-memory cache (5-10s) or invalidate-on-write would remove this without reintroducing the stale-cache race, since the risk there was signing with a stale key, not reading one.

Minor: the pinned-PEM public-key derivation (createPublicKey().export()) reruns on every mint instead of being memoized once at startup — the configured PEM is static for the process lifetime.

Happy to talk through a single-flight lock pattern for the mint path if there's an existing Harper-wide convention you'd rather use.

heskew

I found one additional issue beyond the existing review comments:

src/lib/mcp/keyStore.ts:248 returns the full persisted key set, but retirement cleanup is only scheduled from the mint path (getSigningKey / getOrPersistPinnedKey). At rotation time the old key is intentionally retained because its successor is brand new; however, if no later token mint happens after 2 * accessTokenTtl, nothing re-runs GC. withMCPAuth and buildJWKS keep trusting/publishing that retired key via getAllPublicKeys(), so anyone holding the old private key can continue producing JWTs with valid exp/iss/aud and have them accepted by kid selection indefinitely. Please run the same retirement check before returning keys for JWKS/auth verification, or schedule a delayed cleanup when a successor is created, so retired keys actually fall out of the trusted set after the overlap window.

🤖 Posted by Codex (gpt-5.5) on Nathan's behalf

…ache, pin-bump GC fix Round 5 of PR #158 (issue #128). Addresses Kris's review + Nathan's Codex pass. Single-flight guard (item 1): module-level `pendingWrite` promise serializes concurrent first-boot generation, rotation writes, and pin-persist/bump writes per process. `runSingleFlight(condition, fn)` awaits any in-flight write, re- evaluates `condition` against the result (the concurrent write likely satisfied it), and only runs `fn` with a double-checked-locking fresh enumerate when still needed. Cross-node correctness remains convergence-based (multi-key JWKS + re-enumerate after write); the guard is per-process only. Pin created_at bump — GC leak fix (item 2): when the pinned key exists but is not the newest key, `getOrPersistPinnedKey` re-persists it with `created_at = now` (same kid, same material). Without the bump the previously-newer generated key sits permanently at sorted[0] with no entry below it in the GC loop, so `garbageCollect` never reaches it. The bump makes the pin sorted[0]; the generated key now has a successor whose created_at ≈ the moment it stopped signing, and the existing successor-age GC rule cleans it up on schedule. Read-time retirement (item 3 — security-critical): `partitionRetired` helper implements the successor-age rule used by both `getAllPublicKeys` (read path, never writes) and `garbageCollect` (mint path, physical deletion). `getAllPublicKeys` gains an optional `mcpConfig` param and returns only the live set; retired keys are excluded from the JWKS and from `withMCPAuth` token verification so trust expires by time, not by traffic — a rotated-away private key can no longer forge accepted tokens once the window closes even without mint traffic. `wellKnown.ts` passes `mcpConfig` through to `getAllPublicKeys`; `withMCPAuth` passes `cfg`; `KeySource` interface updated to `getAllPublicKeys(mcpConfig?: MCPConfig)`. 5s enumeration cache (item 4): `enumerateKeys()` caches decoded records in a module-level `{ records, fetchedAt }` entry for 5 s. Every `table.put` and `table.delete` inside `MCPKeyStore` calls `invalidateEnumCache()` immediately so a just-minted key verifies on this node without waiting. The retirement filter runs per-call on the cached records (cheap CPU, time-accurate). `resetMCPKeysTableCache` clears the cache. `_setCacheNowMs(fn|null)` injectable for TTL tests (internal). Pinned public-key memoization (item 5): single-entry memo keyed by PEM string avoids re-deriving the public key on every mint when `signingKeyPem` is stable. Tests: single-flight (5 concurrent first-boot, 3 concurrent rotation → exactly 1 key each), pin-bump (seeded older pin + newer generated → bump, signer, GC regression), read-time retirement (old key excluded from JWKS, token signed by it rejected), cache (hit-count, invalidate-on-write, TTL expiry via clock injection), plus existing test updated to use recent timestamps now that retirement filter is active. 839 pass, 0 fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

heskew · 2026-07-08T22:02:43Z

All five findings from the two reviews addressed in 0927053:

@kriszyp's three + minor:

Thundering herd — key generation/rotation/pin-persist now run under an in-process single-flight guard (runSingleFlight): concurrent mints await the in-flight write and re-evaluate; the guarded section re-checks the trigger from a fresh table read (double-checked locking) before generating. Cross-node stays convergence-based (multi-key JWKS covers correctness) — the guard is per-process by design.
GC leak under a pinned signer — fixed, but via a different mechanic than the suggested i = 0 loop bound: iterating from 0 leaves sorted[0] with no successor to derive a retirement window from, and falling back to the pinned signer's created_at (potentially years old while the pin was configured minutes ago) could GC sorted[0] while its tokens are still live. Instead, when the pin path adopts a signer that is not the newest key, it re-persists the pinned record with created_at = now (same kid + material — nothing strands). The pin becomes sorted[0], the previously-newest key gains a successor stamped at ≈ the moment it stopped signing, and the successor-window rule stays exact. One-time write, idempotent across nodes. Regression test covers the exact leak scenario.
Per-request table scan — enumeration is now cached in-process (5s TTL, invalidated by any local put/delete so a just-minted key verifies immediately on this node; cross-node staleness ≤ TTL ≈ replication lag).
(minor) Pinned public-key derivation is memoized per PEM string.

@heskew's Codex finding (trust never expires without mint traffic): retirement is now evaluated at read time — a shared partitionRetired helper (same successor-window rule) filters getAllPublicKeys(), so withMCPAuth verification and JWKS stop trusting/publishing a retired key on schedule even if no mint ever runs GC. A token signed by a retired key now fails verification once the window passes (test included). Physical deletion remains mint-path-only (unauthenticated JWKS fetches never write); the same helper drives GC, so trust and deletion can't diverge.

841 tests, 839 pass, 0 fail (10 tests added, 1 replaced — net +9 this round).

🤖 Posted by Claude on Nathan's behalf

…ken verify Root cause of the e2e CI failure: after generateAndPersistFirstKey/rotateTo persisted a key and invalidated the cache, the post-write re-enumeration could return [] if Harper's table.search() did not yet reflect the put (timing). The function returned the local fallback but left enumCache = {records: [], ...} live. The next getAllPublicKeys call (token verification in withMCPAuth) hit the stale-empty cache and returned [], triggering "no signing keys available → 401". Fix: when the post-write re-enumerate returns [], explicitly seed enumCache with the local record instead of leaving the stale-empty entry. Applied to all four write paths: generateAndPersistFirstKey, rotateTo, pin-persist, pin-bump. Additional fixes in the same pass (Gemini review blockers): - runSingleFlight: `if` → `while` so a waiter that loses the condition check after N concurrent writes loops back to await the next pendingWrite rather than racing to start a redundant write. - getAllPublicKeys: strip private_key_pem from results (returns MCPPublicKeyRecord[]). Add MCPPublicKeyRecord = Omit<MCPSigningKeyRecord, 'private_key_pem'> type. Update KeySource interface and verifyAccessTokenWithKeySet signature to match. Non-blocking improvements: - enumerateKeys: pre-sort records in cache so partitionRetired/selectNewestKey skip redundant sorts on the hot path. - selectNewestKey: throw on empty input (invariant guard). Unit tests: 840 pass, 0 fail. Added regression test for the stale-empty path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Input is pre-sorted at cache population (enumerateKeys) and by the sorted local fallbacks; document the precondition instead of re-sorting on every verification/JWKS request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions · 2026-07-08T22:34:12Z

+function pinnedKidFromPem(publicKeyPem: string): string {
+	const fingerprint = createHash('sha256').update(publicKeyPem).digest('hex').slice(0, 16);
+	return `pinned-${fingerprint}`;
+}


💡 Suggestion (non-blocking) — Read-side single-flighting

Consider serializing concurrent reads to enumerateKeys when the cache is cold. This would prevent a stampede of table scans on the JWKS or verification paths during a traffic spike.

heskew requested a review from kriszyp July 6, 2026 17:32

github-actions Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts

gemini-code-assist Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts Outdated

claude Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts Outdated

claude Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts

heskew and others added 2 commits July 6, 2026 10:46

style: satisfy require-yield in error-simulating test generators

b3647cb

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

github-actions Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts Outdated

heskew marked this pull request as ready for review July 6, 2026 18:03

heskew commented Jul 6, 2026

View reviewed changes

github-actions Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts Outdated

heskew commented Jul 8, 2026

View reviewed changes

github-actions Bot reviewed Jul 8, 2026

View reviewed changes

Comment thread src/lib/mcp/keyStore.ts

github-actions Bot reviewed Jul 8, 2026

View reviewed changes

kriszyp approved these changes Jul 8, 2026

View reviewed changes

heskew merged commit 6f75443 into main Jul 8, 2026
12 checks passed

heskew deleted the feat/128-key-rotation-multikey-jwks branch July 8, 2026 23:50

heskew mentioned this pull request Jul 11, 2026

Release 2.2.0 #172

Merged

Uh oh!

Conversation

heskew commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Where to focus review

Tests

Uh oh!

github-actions Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suggestions (non-blocking)

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

heskew left a comment

Choose a reason for hiding this comment

Uh oh!

heskew commented Jul 6, 2026

Uh oh!

Uh oh!

kriszyp commented Jul 7, 2026

Uh oh!

heskew left a comment

Choose a reason for hiding this comment

Uh oh!

heskew commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

heskew commented Jul 6, 2026 •

edited

Loading

github-actions Bot commented Jul 6, 2026 •

edited

Loading

claude Bot commented Jul 6, 2026 •

edited

Loading

heskew commented Jul 8, 2026 •

edited

Loading