From bbb2f444aa8397c8ec654e19f09186cf5b04e0fd Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Mon, 29 Jun 2026 21:06:52 +0200 Subject: [PATCH] refactor(mcp-oauth): stateless signed client_id (multi-replica safe) Replace the in-memory dynamic-client registry with a self-describing, HMAC-signed client_id that encodes the registered redirect_uris. The Container App is stateless with scale-to-zero and may run multiple replicas, so the old Map was wiped on cold start and never shared across replicas -- a client_id minted by /oauth/register read as unknown at /oauth/authorize, surfacing as invalid_client. The signed token is verifiable on any replica holding the same root secret (OAuthStateSigningKey, already deployed), so: - /oauth/authorize now verifies the signature statelessly and FAILS CLOSED on a forged/unknown client_id (back to 400 invalid_client), while keeping the exact per-client redirect_uri match. This removes the earlier cold-start fail-open without reintroducing the bug. - /oauth/callback verifies the same way (the redirect_uri is already pinned in the HMAC-signed state). - /oauth/register returns the signed token; no storage, no memory cap. The client_id signing key is domain-separated from the proxy-state key via a labelled HMAC, and the payload carries a `typ` discriminator, so a state token can never be replayed as a client_id or vice versa. This is the prerequisite for raising maxReplicas above 1; remaining per-instance state (accessCache, rate-limit buckets) degrades gracefully, not incorrectly. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/oauth-register.test.ts | 79 +++++--- .../src/__tests__/security-guards.test.ts | 11 +- .../autopilot-monitor-mcp/src/oauth.ts | 189 ++++++++++-------- 3 files changed, 165 insertions(+), 114 deletions(-) diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts index bd73f10a..ffc8abd4 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts @@ -11,14 +11,14 @@ * would call Entra ID (the /oauth/authorize success case yields a 302 we read * with redirect:'manual'; the token error case returns before fetch). */ -import { describe, it, expect, beforeAll, afterAll, afterEach } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import express from 'express'; import type { Server } from 'node:http'; import type { AddressInfo } from 'node:net'; // createOAuthRouter() throws at import unless the Entra client id is present. process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; -const { createOAuthRouter, _registryForTest, MAX_REGISTERED_CLIENTS, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH } = +const { createOAuthRouter, signClientId, verifyClientId, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH } = await import('../oauth.js'); let server: Server; @@ -40,11 +40,6 @@ afterAll(async () => { await new Promise((resolve) => server.close(() => resolve())); }); -afterEach(() => { - // Reset the registry between tests (the cap test fills it to capacity). - _registryForTest.clear(); -}); - async function register(body: unknown) { const res = await fetch(`${baseUrl}/oauth/register`, { method: 'POST', @@ -117,15 +112,39 @@ describe('/oauth/register — dynamic client registration (RFC 7591)', () => { expect(json.error).toBe('invalid_client_metadata'); }); - it('returns 503 once the registry is at its hard cap', async () => { - _registryForTest.fill(MAX_REGISTERED_CLIENTS); - expect(_registryForTest.size()).toBeGreaterThanOrEqual(MAX_REGISTERED_CLIENTS); - const { status, json } = await register({ - client_name: 'overflow', - redirect_uris: ['http://localhost:1/cb'], - }); - expect(status).toBe(503); - expect(json.error).toBe('temporarily_unavailable'); + it('returns a stateless signed client_id that verifies back to its redirect_uris', async () => { + const uris = ['http://127.0.0.1:33418/', 'https://vscode.dev/redirect']; + const { json } = await register({ client_name: 'Visual Studio Code', redirect_uris: uris }); + const clientId = json.client_id as string; + // Format is `base64url(payload).hmac` — not an opaque UUID. + expect(clientId).toContain('.'); + const verified = verifyClientId(clientId); + expect(verified).not.toBeNull(); + expect(verified?.redirectUris).toEqual(uris); + expect(verified?.typ).toBe('client'); + }); +}); + +describe('signClientId / verifyClientId — stateless client registry', () => { + it('round-trips redirect_uris through a signed token', () => { + const token = signClientId(['http://127.0.0.1:1/cb'], 'x'); + expect(verifyClientId(token)?.redirectUris).toEqual(['http://127.0.0.1:1/cb']); + }); + + it('rejects a tampered payload (signature no longer matches)', () => { + const token = signClientId(['http://127.0.0.1:1/cb'], 'x'); + const [, sig] = token.split('.'); + const forgedBody = Buffer.from( + JSON.stringify({ typ: 'client', redirectUris: ['http://127.0.0.1:6666/'], name: 'evil', iat: 0 }), + 'utf-8', + ).toString('base64url'); + expect(verifyClientId(`${forgedBody}.${sig}`)).toBeNull(); + }); + + it('rejects a random / opaque client_id (no valid signature)', () => { + expect(verifyClientId('00000000-dead-beef-0000-000000000000')).toBeNull(); + expect(verifyClientId('not.a.token')).toBeNull(); + expect(verifyClientId('')).toBeNull(); }); }); @@ -159,23 +178,35 @@ describe('/oauth/authorize — PKCE S256 enforcement', () => { expect(res.status).toBe(400); }); - it('tolerates an unknown client_id (registry cold-start) and still redirects to Entra', async () => { - // Stateless scale-to-zero / multi-replica wipes the in-memory registry, so - // a client_id minted by /oauth/register can read as unknown at /authorize. - // The host allowlist + PKCE are the real gate; the flow must proceed, not - // dead-end with invalid_client. + it('rejects a forged / unsigned client_id with invalid_client (fail closed)', async () => { + // A signed client_id is verifiable on any replica, so an unverifiable one + // is genuinely bogus — fail closed rather than fall back to a weaker gate. const res = await authorize({ client_id: '00000000-dead-beef-0000-000000000000', redirect_uri: 'http://127.0.0.1:33418/', code_challenge: 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', code_challenge_method: 'S256', }); - expect(res.status).toBe(302); - expect(res.headers.get('location') ?? '').toContain('login.microsoftonline.com'); + expect(res.status).toBe(400); + expect(((await res.json()) as Record).error).toBe('invalid_client'); }); - it('redirects a valid S256 request to the Entra authorize endpoint', async () => { + it('rejects a valid client_id whose registered list omits the redirect_uri', async () => { + const clientId = signClientId(['http://127.0.0.1:33418/'], 'vscode'); const res = await authorize({ + client_id: clientId, + redirect_uri: 'http://127.0.0.1:9999/', // loopback (allowlisted) but not registered to this client + code_challenge: 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', + code_challenge_method: 'S256', + }); + expect(res.status).toBe(400); + expect(((await res.json()) as Record).error).toBe('invalid_request'); + }); + + it('redirects a valid S256 request (signed client_id, matching redirect_uri) to Entra', async () => { + const clientId = signClientId(['http://localhost:54321/cb'], 'cc'); + const res = await authorize({ + client_id: clientId, redirect_uri: 'http://localhost:54321/cb', code_challenge: 'E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM', code_challenge_method: 'S256', diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts index 6cb730a9..b6c1b2e0 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts @@ -29,7 +29,6 @@ const { parseAllowedHosts, signState, verifyState, - MAX_REGISTERED_CLIENTS, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH, @@ -325,13 +324,9 @@ describe('F1 — access-guard cache key includes token hash', () => { }); describe('F2 — /oauth/register defense constants', () => { - it('exposes a hard cap that bounds memory at ~3 MB worst case', () => { - // 0.5 GiB container budget × ~300 bytes/entry → 10k is two orders of - // magnitude below the OOM cliff and well above realistic peak. - expect(MAX_REGISTERED_CLIENTS).toBeGreaterThanOrEqual(1_000); - expect(MAX_REGISTERED_CLIENTS).toBeLessThanOrEqual(100_000); - }); - + // The client registry is now stateless (signed client_id, no server-side + // Map), so there is no MAX_REGISTERED_CLIENTS memory cap to assert — the + // field-level bounds below are what backstop the signed-token size. it('caps redirect_uris per client (RFC 7591 typically lists 1-3)', () => { expect(MAX_REDIRECT_URIS_PER_CLIENT).toBeGreaterThanOrEqual(2); expect(MAX_REDIRECT_URIS_PER_CLIENT).toBeLessThanOrEqual(50); diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index 6187df70..0d61b170 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -33,18 +33,18 @@ const CLIENT_SECRET = process.env.AUTOPILOT_ENTRA_CLIENT_SECRET ?? ''; const AUTHORITY = process.env.AUTOPILOT_ENTRA_AUTHORITY ?? 'https://login.microsoftonline.com/organizations'; const SCOPES = `api://${CLIENT_ID}/access_as_user openid profile offline_access`; -/** - * In-memory store for dynamically registered OAuth clients (RFC 7591). - * Clients re-register after a server restart — this is fine because the MCP - * server acts as an OAuth proxy and the real credentials live in Entra ID. - * - * TODO(oauth-map-ttl): TTL/LRU eviction is intentionally NOT enabled. With a - * stateless scale-to-zero Container App, an evicted client_id forces the user - * back through Claude Code's manual `/mcp` reconnect flow (no auto-recovery). - * The hard size cap (MAX_REGISTERED_CLIENTS) bounds memory; revisit TTL once - * the reconnect UX is debugged or the registry moves to persistent storage. - */ -const registeredClients = new Map(); +// Dynamic client registration (RFC 7591) is STATELESS: instead of a +// server-side Map, the issued client_id is itself an HMAC-signed token that +// encodes the client's registered redirect_uris (see signClientId below). +// +// Why: the Container App is stateless with scale-to-zero and may run multiple +// replicas. An in-memory registry was wiped on every cold start and never +// shared across replicas, so a client_id minted by /oauth/register read as +// "unknown" at /oauth/authorize once the container slept or a sibling replica +// served the request — surfacing as "invalid_client". A self-describing signed +// client_id needs no shared storage: any replica holding the same signing key +// can verify it and recover the registered redirect_uris. This also removes +// the unbounded-memory concern that the old hard size cap guarded against. // ---- State signing (HMAC) ------------------------------------------------- // @@ -122,34 +122,76 @@ export function verifyState(state: string, nowSeconds: number = Math.floor(Date. return parsed; } -/** - * Hard cap on the dynamic-client registry. At ~300 bytes per entry this caps - * memory at ~3 MB worst case — well below the 0.5 GiB container budget. Real - * usage adds one entry per Claude-Code-session × server-URL, lifetime; even a - * busy deployment stays in the low thousands. - */ -export const MAX_REGISTERED_CLIENTS = 10_000; export const MAX_REDIRECT_URIS_PER_CLIENT = 10; export const MAX_REDIRECT_URI_LENGTH = 1024; export const MAX_CLIENT_NAME_LENGTH = 256; +// ---- Client-id signing (HMAC) --------------------------------------------- +// +// The dynamic-registration client_id is a self-describing signed token rather +// than a lookup key into server-side storage. It carries the client's +// registered redirect_uris so /oauth/authorize and /oauth/callback can enforce +// the exact-match check without any shared registry — making the whole OAuth +// proxy stateless and multi-replica safe. +// +// The signing key is DERIVED from OAuthStateSigningKey via a labelled HMAC so +// it is domain-separated from the proxy-state key: a `state` token can never be +// replayed as a `client_id` (or vice versa) even though both use the same root +// secret. Deriving means one secret to manage; the `typ` discriminator inside +// each payload is a second, independent guard against cross-use. +const OAUTH_CLIENT_ID_KEY: Buffer = crypto + .createHmac('sha256', OAUTH_STATE_SIGNING_KEY) + .update('autopilot-mcp/oauth-client-id/v1') + .digest(); + +interface ClientIdPayload { + typ: 'client'; + redirectUris: string[]; + name: string; + iat: number; // unix seconds — issued-at, for observability; not expired +} + /** - * Test-only handle on the registry. Lets unit tests verify cap behavior - * without spinning up the full Express stack. Production code never imports - * this — it only ever touches the closure inside createOAuthRouter(). + * Mints a stateless client_id: `base64url(json).hmac`. Survives restarts and is + * verifiable on any replica holding the same root secret. A public-client + * client_id is not a credential (token_endpoint_auth_method=none) — it only + * asserts which already-host-allowlisted redirect_uris were registered — so it + * carries no expiry, matching RFC 7591's client_secret_expires_at=0 semantics. */ -export const _registryForTest = { - size: () => registeredClients.size, - fill: (count: number) => { - for (let i = 0; i < count; i++) { - registeredClients.set(`__test_${i}_${crypto.randomUUID()}`, { - name: '__test', - redirectUris: ['http://localhost:1/cb'], - }); - } - }, - clear: () => registeredClients.clear(), -}; +export function signClientId(redirectUris: string[], name: string): string { + const payload: ClientIdPayload = { + typ: 'client', + redirectUris, + name, + iat: Math.floor(Date.now() / 1000), + }; + const body = Buffer.from(JSON.stringify(payload), 'utf-8').toString('base64url'); + const sig = crypto.createHmac('sha256', OAUTH_CLIENT_ID_KEY).update(body).digest('base64url'); + return `${body}.${sig}`; +} + +/** Verifies a signed client_id and returns its payload, or null if forged/malformed. */ +export function verifyClientId(clientId: string | undefined | null): ClientIdPayload | null { + if (!clientId) return null; + const parts = clientId.split('.'); + if (parts.length !== 2) return null; + const [body, sig] = parts; + + const expected = crypto.createHmac('sha256', OAUTH_CLIENT_ID_KEY).update(body).digest('base64url'); + const sigBuf = Buffer.from(sig, 'utf-8'); + const expBuf = Buffer.from(expected, 'utf-8'); + if (sigBuf.length !== expBuf.length) return null; + if (!crypto.timingSafeEqual(sigBuf, expBuf)) return null; + + let parsed: ClientIdPayload; + try { + parsed = JSON.parse(Buffer.from(body, 'base64url').toString('utf-8')) as ClientIdPayload; + } catch { + return null; + } + if (parsed.typ !== 'client' || !Array.isArray(parsed.redirectUris)) return null; + return parsed; +} /** * Strict allowlist of host patterns acceptable for OAuth redirect_uri. @@ -304,26 +346,14 @@ export function createOAuthRouter(): Router { // --- Dynamic Client Registration (RFC 7591) --- router.post('/oauth/register', (req, res) => { - // Hard memory ceiling — refuse new registrations once the registry is at - // capacity. The endpoint is unauthenticated per RFC 7591, so without this - // a flood would push the 0.5 GiB container into OOM. - if (registeredClients.size >= MAX_REGISTERED_CLIENTS) { - console.error( - `[oauth/register] Registry at cap (${MAX_REGISTERED_CLIENTS}) — rejecting new registration`, - ); - res.status(503).json({ - error: 'temporarily_unavailable', - error_description: 'Registration capacity reached, please retry later', - }); - return; - } - const { client_name, redirect_uris, grant_types, response_types, token_endpoint_auth_method } = req.body ?? {}; // Field-level bounds — backstop the route-level body-size limit. The // parser caps total body bytes; these caps stop a single registration // from carrying e.g. a thousand redirect_uris that all individually - // pass the URI allowlist. + // pass the URI allowlist. They also bound the size of the signed client_id + // token (which embeds redirect_uris) so a crafted registration cannot mint + // an arbitrarily long client_id. if (typeof client_name === 'string' && client_name.length > MAX_CLIENT_NAME_LENGTH) { res.status(400).json({ error: 'invalid_client_metadata', @@ -369,13 +399,12 @@ export function createOAuthRouter(): Router { return; } - const clientId = crypto.randomUUID(); - registeredClients.set(clientId, { - name: client_name ?? 'unknown', - redirectUris: uris, - }); + // Stateless: the client_id is a signed token embedding the redirect_uris, + // not a key into a server-side map. No storage, no memory cap, verifiable + // on any replica (see signClientId). + const clientId = signClientId(uris, client_name ?? 'unknown'); - console.error(`[oauth/register] Registered dynamic client ${clientId} (${client_name ?? 'unknown'})`); + console.error(`[oauth/register] Registered dynamic client (${client_name ?? 'unknown'}), ${uris.length} redirect_uri(s)`); res.status(201).json({ client_id: clientId, @@ -441,24 +470,22 @@ export function createOAuthRouter(): Router { return; } if (client_id) { - const registered = registeredClients.get(client_id); + // The client_id is a self-describing signed token (see signClientId), so + // this verifies statelessly — no registry lookup, correct across cold + // starts and replicas. A forged/malformed token fails closed with + // invalid_client; a valid one must still carry the exact redirect_uri. + const registered = verifyClientId(client_id); if (!registered) { - // Stateless scale-to-zero / multi-replica: the in-memory registry may - // have been wiped (cold start) or live on a different replica since - // /oauth/register ran, so a client_id that was just issued can read as - // "unknown" here. The redirect_uri host allowlist (checked above) plus - // mandatory PKCE are the real gate, so fall back to them rather than - // dead-ending the user's flow — same cold-start tolerance the - // /oauth/callback handler already applies. Hard-failing here is what - // produced "invalid_client — register via /oauth/register first" for - // browser-opened authorize URLs after the container had slept. - console.warn( - `[oauth/authorize] Unknown client_id ${client_id} (registry cold-start / other replica?) — ` + - 'proceeding on host-allowlist gate only', - ); - } else if (!registered.redirectUris.includes(redirect_uri)) { + console.error('[oauth/authorize] client_id failed signature verification'); + res.status(400).json({ + error: 'invalid_client', + error_description: 'client_id is invalid — register via /oauth/register first', + }); + return; + } + if (!registered.redirectUris.includes(redirect_uri)) { console.error( - `[oauth/authorize] redirect_uri not registered for client ${client_id}: ${redirect_uri}`, + `[oauth/authorize] redirect_uri not registered for this client_id: ${redirect_uri}`, ); res.status(400).json({ error: 'invalid_request', @@ -522,10 +549,8 @@ export function createOAuthRouter(): Router { const clientId = decoded.clientId; // Re-validate the redirectUri at callback time too. Belt-and-suspenders: - // /oauth/authorize already gated the URI, but the state value transits - // the user-agent and could in principle be replayed against a server - // whose registeredClients map was wiped (cold start) — re-running the - // host allowlist + registry check here closes that gap. + // /oauth/authorize already gated the URI, but the state value transits the + // user-agent — re-running the host allowlist here closes any replay gap. if (!isAllowedRedirectUri(redirectUri)) { console.error(`[oauth/callback] Rejected redirectUri from state (host not in allowlist): ${redirectUri}`); res.status(400).json({ @@ -535,13 +560,13 @@ export function createOAuthRouter(): Router { return; } if (clientId) { - const registered = registeredClients.get(clientId); - // Cold-start may have wiped the registry — fall back to host allowlist - // alone (already validated above) rather than fail the user's flow. - if (registered && !registered.redirectUris.includes(redirectUri)) { - console.error( - `[oauth/callback] redirectUri not registered for client ${clientId}: ${redirectUri}`, - ); + // Stateless verify (no registry) — correct across cold starts/replicas. + // The redirectUri is already pinned inside the HMAC-verified state, so + // this only adds defense-in-depth: a valid client_id must still list the + // redirectUri. A forged client_id fails closed. + const registered = verifyClientId(clientId); + if (!registered || !registered.redirectUris.includes(redirectUri)) { + console.error(`[oauth/callback] client_id invalid or redirectUri not registered: ${redirectUri}`); res.status(400).json({ error: 'invalid_request', error_description: 'redirect_uri does not match any URI registered for this client_id',