diff --git a/.changeset/scope-jwks-cache-per-instance.md b/.changeset/scope-jwks-cache-per-instance.md new file mode 100644 index 00000000000..956e60370bb --- /dev/null +++ b/.changeset/scope-jwks-cache-per-instance.md @@ -0,0 +1,9 @@ +--- +'@clerk/backend': patch +--- + +Scope the JWKS cache per Clerk instance. The cache was keyed on the JWT `kid` alone and shared across the whole process, so an application verifying tokens for more than one Clerk instance (for example the Dynamic Keys / multi-tenant pattern) could resolve a signing key that was fetched for a different instance. Keys are now cached separately per secret key and API URL, so a token can only be verified against the instance whose credentials fetched its signing key. + +Networkless verification with `jwtKey` had the same flaw: the JWK derived from the PEM was cached by `kid` alone, so a process verifying tokens with different `jwtKey` values could resolve a key derived from another instance's PEM. The JWK is now always derived from the `jwtKey` that was passed in. + +The `jwk-kid-mismatch` error message no longer lists the key IDs currently held in the cache. diff --git a/packages/backend/src/tokens/__tests__/keys.test.ts b/packages/backend/src/tokens/__tests__/keys.test.ts index f4b301c9d53..110273e0613 100644 --- a/packages/backend/src/tokens/__tests__/keys.test.ts +++ b/packages/backend/src/tokens/__tests__/keys.test.ts @@ -37,7 +37,7 @@ describe('tokens.loadClerkJWKFromLocal(localKey)', () => { expect(jwk).toMatchObject(mockPEMJwk); }); - it('caches PEM keys separately for different kids', () => { + it('derives a separate JWK per kid', () => { const jwk1 = loadClerkJwkFromPem({ kid: 'ins_1', pem: mockPEMKey }) as JsonWebKey & { kid: string }; expect(jwk1.kid).toBe('local-ins_1'); expect(jwk1.n).toBe(mockPEMJwk.n); @@ -45,37 +45,32 @@ describe('tokens.loadClerkJWKFromLocal(localKey)', () => { const jwk2 = loadClerkJwkFromPem({ kid: 'ins_2', pem: mockPEMJwtKey }) as JsonWebKey & { kid: string }; expect(jwk2.kid).toBe('local-ins_2'); expect(jwk2.n).toBe(mockPEMJwk.n); + }); - // Verify both are cached independently - const jwk1Cached = loadClerkJwkFromPem({ kid: 'ins_1', pem: mockPEMKey }); - const jwk2Cached = loadClerkJwkFromPem({ kid: 'ins_2', pem: mockPEMJwtKey }); + // Regression test for SDK-148. A cache keyed on `kid` alone (an untrusted token-header + // value) served the first caller's key to every later caller presenting the same kid, + // regardless of the pem they supplied. + it('always derives the JWK from the provided pem, even for a previously seen kid', () => { + const otherModulus = 'x'.repeat(342); + const otherPem = `MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA${otherModulus}IDAQAB`; - expect(jwk1Cached).toBe(jwk1); - expect(jwk2Cached).toBe(jwk2); // Same object reference means its cached - }); + const jwkA = loadClerkJwkFromPem({ kid: 'ins_same_kid', pem: mockPEMKey }) as JsonWebKey & { kid: string }; + expect(jwkA.n).toBe(mockPEMJwk.n); - it('returns cached JWK on subsequent calls with same kid', () => { - const jwk1 = loadClerkJwkFromPem({ kid: 'cache-test', pem: mockPEMKey }); - const jwk2 = loadClerkJwkFromPem({ kid: 'cache-test', pem: mockPEMKey }); - // Should return the exact same reference - expect(jwk1).toBe(jwk2); + const jwkB = loadClerkJwkFromPem({ kid: 'ins_same_kid', pem: otherPem }) as JsonWebKey & { kid: string }; + expect(jwkB.n).toBe(otherModulus); }); - it('uses "local-" prefix to avoid cache collision with remote keys', () => { + it('uses "local-" prefix to distinguish the JWK from remote keys', () => { const localJwk = loadClerkJwkFromPem({ kid: 'test-kid', pem: mockPEMKey }) as JsonWebKey & { kid: string }; expect(localJwk.kid).toBe('local-test-kid'); }); - it('creates separate cache entries for different kids even with same PEM', () => { - // Two JWT keys might theoretically use the same PEM (unlikely but possible) + it('derives separate JWKs for different kids even with same PEM', () => { const jwkA = loadClerkJwkFromPem({ kid: 'ins_key_a', pem: mockPEMKey }) as JsonWebKey & { kid: string }; const jwkB = loadClerkJwkFromPem({ kid: 'ins_key_b', pem: mockPEMKey }) as JsonWebKey & { kid: string }; - // They should be different objects - expect(jwkA).not.toBe(jwkB); - // But have the same modulus expect(jwkA.n).toBe(jwkB.n); - // And different prefixed kids expect(jwkA.kid).toBe('local-ins_key_a'); expect(jwkB.kid).toBe('local-ins_key_b'); }); @@ -200,8 +195,156 @@ describe('tokens.loadClerkJWKFromRemote(options)', () => { kid, }), ).rejects.toThrowError( - "Unable to find a signing key in JWKS that matches the kid='ins_whatever' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT. The following kid is available: ins_2GIoQhbUpy0hX7B2cVkuTMinXoD", + "Unable to find a signing key in JWKS that matches the kid='ins_whatever' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT.", + ); + }); + + // The cached kids are instance ids; enumerating them discloses which co-tenants + // are warm in a shared process. + it('does not enumerate cached kids in the error message', async () => { + server.use( + http.get( + 'https://api.clerk.com/v1/jwks', + validateHeaders(() => { + return HttpResponse.json(mockJwks); + }), + ), + ); + + const error = await loadClerkJWKFromRemote({ secretKey: 'deadbeef', kid: 'ins_whatever' }).catch(e => e); + + expect(error).toBeInstanceOf(TokenVerificationError); + expect(error.message).not.toContain(mockRsaJwkKid); + }); + + // Regression test for SDK-148. The cache was keyed on `kid` alone. Since a Clerk `kid` + // is the instance id and the lookup short-circuits before `secretKey` is consulted, a + // key fetched for one instance was served to another instance's verifier, and + // `verifyJwt` never asserts `iss`. + it('does not serve a cached key to a different secretKey', async () => { + const instanceAKid = 'ins_tenant_a'; + let secretKeysUsed: string[] = []; + + server.use( + http.get( + 'https://api.clerk.com/v1/jwks', + validateHeaders(({ request }) => { + secretKeysUsed.push((request.headers.get('Authorization') ?? '').replace('Bearer ', '')); + // Each instance's JWKS contains only its own signing key. + return HttpResponse.json({ keys: [{ ...mockRsaJwk, kid: instanceAKid }] }); + }), + ), ); + + // Instance A warms the cache with its own key. + const jwk = await loadClerkJWKFromRemote({ secretKey: 'sk_test_a', kid: instanceAKid }); + expect(jwk).toMatchObject({ kid: instanceAKid }); + expect(secretKeysUsed).toEqual(['sk_test_a']); + + // Instance B asking for instance A's kid must miss the cache and fetch under its + // own secretKey. + secretKeysUsed = []; + server.use( + http.get( + 'https://api.clerk.com/v1/jwks', + validateHeaders(({ request }) => { + secretKeysUsed.push((request.headers.get('Authorization') ?? '').replace('Bearer ', '')); + return HttpResponse.json({ keys: [{ ...mockRsaJwk, kid: 'ins_tenant_b' }] }); + }), + ), + ); + + await expect(() => loadClerkJWKFromRemote({ secretKey: 'sk_test_b', kid: instanceAKid })).rejects.toThrowError( + TokenVerificationError, + ); + expect(secretKeysUsed).toEqual(['sk_test_b']); + }); + + it('keeps a separate cache TTL per instance', async () => { + let fetchCount = 0; + server.use( + http.get( + 'https://api.clerk.com/v1/jwks', + validateHeaders(() => { + fetchCount++; + return HttpResponse.json(mockJwks); + }), + ), + ); + + await loadClerkJWKFromRemote({ secretKey: 'sk_ttl_a', kid: mockRsaJwkKid }); + expect(fetchCount).toBe(1); + + // A second instance must not ride on the first instance's fresh TTL. + await loadClerkJWKFromRemote({ secretKey: 'sk_ttl_b', kid: mockRsaJwkKid }); + expect(fetchCount).toBe(2); + + // Each instance now serves from its own cache. + await loadClerkJWKFromRemote({ secretKey: 'sk_ttl_a', kid: mockRsaJwkKid }); + await loadClerkJWKFromRemote({ secretKey: 'sk_ttl_b', kid: mockRsaJwkKid }); + expect(fetchCount).toBe(2); + }); + + it('keeps a separate cache per apiUrl', async () => { + const fetches = { com: 0, test: 0 }; + server.use( + http.get( + 'https://api.clerk.com/v1/jwks', + validateHeaders(() => { + fetches.com++; + return HttpResponse.json(mockJwks); + }), + ), + http.get( + 'https://api.clerk.test/v1/jwks', + validateHeaders(() => { + fetches.test++; + return HttpResponse.json(mockJwks); + }), + ), + ); + + await loadClerkJWKFromRemote({ secretKey: 'sk_api_url', kid: mockRsaJwkKid }); + expect(fetches).toEqual({ com: 1, test: 0 }); + + // The same kid under another apiUrl must not be served from the first scope's cache. + await loadClerkJWKFromRemote({ secretKey: 'sk_api_url', apiUrl: 'https://api.clerk.test', kid: mockRsaJwkKid }); + expect(fetches).toEqual({ com: 1, test: 1 }); + + await loadClerkJWKFromRemote({ secretKey: 'sk_api_url', kid: mockRsaJwkKid }); + await loadClerkJWKFromRemote({ secretKey: 'sk_api_url', apiUrl: 'https://api.clerk.test', kid: mockRsaJwkKid }); + expect(fetches).toEqual({ com: 1, test: 1 }); + }); + + it('keeps a separate cache per apiVersion', async () => { + const fetches = { v1: 0, v2: 0 }; + server.use( + http.get( + 'https://api.clerk.com/v1/jwks', + validateHeaders(() => { + fetches.v1++; + return HttpResponse.json(mockJwks); + }), + ), + http.get( + 'https://api.clerk.com/v2/jwks', + validateHeaders(() => { + fetches.v2++; + return HttpResponse.json(mockJwks); + }), + ), + ); + + await loadClerkJWKFromRemote({ secretKey: 'sk_api_version', kid: mockRsaJwkKid }); + expect(fetches).toEqual({ v1: 1, v2: 0 }); + + // The same kid under another apiVersion must not be served from the first scope's cache. + await loadClerkJWKFromRemote({ secretKey: 'sk_api_version', apiVersion: 'v2', kid: mockRsaJwkKid }); + expect(fetches).toEqual({ v1: 1, v2: 1 }); + + await loadClerkJWKFromRemote({ secretKey: 'sk_api_version', kid: mockRsaJwkKid }); + await loadClerkJWKFromRemote({ secretKey: 'sk_api_version', apiVersion: 'v2', kid: mockRsaJwkKid }); + expect(fetches).toEqual({ v1: 1, v2: 1 }); }); it('cache TTLs do not conflict', async () => { diff --git a/packages/backend/src/tokens/keys.ts b/packages/backend/src/tokens/keys.ts index 64d487a8760..25043b920bc 100644 --- a/packages/backend/src/tokens/keys.ts +++ b/packages/backend/src/tokens/keys.ts @@ -19,20 +19,36 @@ type JsonWebKeyWithKid = JsonWebKey & { kid: string }; type JsonWebKeyCache = Record; -let cache: JsonWebKeyCache = {}; -let lastUpdatedAt = 0; - -function getFromCache(kid: string) { - return cache[kid]; -} +type RemoteJwksCache = { + keys: JsonWebKeyCache; + lastUpdatedAt: number; +}; -function getCacheValues() { - return Object.values(cache); -} +/** + * Remote JWKS caches, one per Clerk instance. A single process-wide cache keyed by `kid` + * alone hands one instance's signing key to another instance's verification: a Clerk `kid` + * is the instance id, the lookup short-circuits before `secretKey` is consulted, and + * `verifyJwt` does not assert `iss`. That let a session token minted by instance B + * authenticate against instance A in any process serving both. + */ +const remoteCaches = new Map(); -function setInCache(cacheKey: string, jwk: JsonWebKeyWithKid, shouldExpire = true) { - cache[cacheKey] = jwk; - lastUpdatedAt = shouldExpire ? Date.now() : -1; +/** + * The scope is held in memory only as a Map key. It is never logged or surfaced in errors. + */ +function getRemoteCache(scope: string): RemoteJwksCache { + let cache = remoteCaches.get(scope); + if (!cache) { + // Evict expired scopes on new-scope creation so one-off scopes cannot grow the Map forever. + for (const [key, entry] of remoteCaches) { + if (cacheHasExpired(entry)) { + remoteCaches.delete(key); + } + } + cache = { keys: {}, lastUpdatedAt: 0 }; + remoteCaches.set(scope, cache); + } + return cache; } const PEM_HEADER = '-----BEGIN PUBLIC KEY-----'; @@ -47,21 +63,12 @@ type LoadClerkJwkFromPemOptions = { /** * Loads a local PEM key usually from process.env and transform it to JsonWebKey format. - * The result is cached on the module level to avoid unnecessary computations in subsequent invocations. + * Derived fresh on every call: a cache keyed on `kid` (which comes from the untrusted token + * header) served one instance's key to another instance's verifier, and derivation is cheap. */ export function loadClerkJwkFromPem(params: LoadClerkJwkFromPemOptions): JsonWebKey { const { kid, pem } = params; - // We use a cache key that includes the local prefix in order to avoid - // cache conflicts when loadClerkJwkFromPem and loadClerkJWKFromRemote - // are called with the same kid - const prefixedKid = `local-${kid}`; - const cachedJwk = getFromCache(prefixedKid); - - if (cachedJwk) { - return cachedJwk; - } - if (!pem) { throw new TokenVerificationError({ action: TokenVerificationErrorAction.SetClerkJWTKey, @@ -80,8 +87,8 @@ export function loadClerkJwkFromPem(params: LoadClerkJwkFromPemOptions): JsonWeb .replace(/\//g, '_'); // https://datatracker.ietf.org/doc/html/rfc7517 - const jwk = { kid: prefixedKid, kty: 'RSA', alg: 'RS256', n: modulus, e: 'AQAB' }; - setInCache(prefixedKid, jwk, false); // local key never expires in cache + // The 'local-' kid prefix distinguishes locally derived JWKs from remote ones. + const jwk: JsonWebKeyWithKid = { kid: `local-${kid}`, kty: 'RSA', alg: 'RS256', n: modulus, e: 'AQAB' }; return jwk; } @@ -131,7 +138,9 @@ export type LoadClerkJWKFromRemoteOptions = { export async function loadClerkJWKFromRemote(params: LoadClerkJWKFromRemoteOptions): Promise { const { secretKey, apiUrl = API_URL, apiVersion = API_VERSION, kid, skipJwksCache } = params; - if (skipJwksCache || cacheHasExpired() || !getFromCache(kid)) { + const cache = getRemoteCache(`${apiUrl}|${apiVersion}|${secretKey ?? ''}`); + + if (skipJwksCache || cacheHasExpired(cache) || !cache.keys[kid]) { if (!secretKey) { throw new TokenVerificationError({ action: TokenVerificationErrorAction.ContactSupport, @@ -150,21 +159,20 @@ export async function loadClerkJWKFromRemote(params: LoadClerkJWKFromRemoteOptio }); } - keys.forEach(key => setInCache(key.kid, key)); + keys.forEach(key => { + cache.keys[key.kid] = key; + }); + cache.lastUpdatedAt = Date.now(); } - const jwk = getFromCache(kid); + const jwk = cache.keys[kid]; if (!jwk) { - const cacheValues = getCacheValues(); - const jwkKeys = cacheValues - .map(jwk => jwk.kid) - .sort() - .join(', '); - + // The available kids are deliberately omitted: they are instance ids, and enumerating + // them would disclose which co-tenants are warm in a shared process. throw new TokenVerificationError({ action: `Go to your Dashboard and validate your secret and public keys are correct. ${TokenVerificationErrorAction.ContactSupport} if the issue persists.`, - message: `Unable to find a signing key in JWKS that matches the kid='${kid}' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT. The following kid is available: ${jwkKeys}`, + message: `Unable to find a signing key in JWKS that matches the kid='${kid}' of the provided session token. Please make sure that the __session cookie or the HTTP authorization header contain a Clerk-generated session JWT.`, reason: TokenVerificationErrorReason.JWKKidMismatch, }); } @@ -218,17 +226,12 @@ async function fetchJWKSFromBAPI(apiUrl: string, key: string, apiVersion: string return response.json(); } -function cacheHasExpired() { - // If lastUpdatedAt is -1, it means that we're using a local JWKS and it never expires - if (lastUpdatedAt === -1) { - return false; - } - +function cacheHasExpired(cache: RemoteJwksCache) { // If the cache has expired, clear the value so we don't attempt to make decisions based on stale data - const isExpired = Date.now() - lastUpdatedAt >= MAX_CACHE_LAST_UPDATED_AT_SECONDS * 1000; + const isExpired = Date.now() - cache.lastUpdatedAt >= MAX_CACHE_LAST_UPDATED_AT_SECONDS * 1000; if (isExpired) { - cache = {}; + cache.keys = {}; } return isExpired;