diff --git a/.changeset/eager-pandas-verify.md b/.changeset/eager-pandas-verify.md new file mode 100644 index 0000000..cd48034 --- /dev/null +++ b/.changeset/eager-pandas-verify.md @@ -0,0 +1,37 @@ +--- +'seamless-auth-api': minor +--- + +Read the JWKS public keys document under one name, and document rotation properly. + +**Fixes a deployment trap.** `validateEnvs.sh` required `JWKS_PUBLIC_KEYS`, and the +configuration reference and `.env.example` named only that, but token verification read +`SEAMLESS_JWKS_PUBLIC_KEYS`. A deployment that set exactly what this API asked for +started cleanly and then threw on every JWT verification, because +`getPublicKeyByKid` could not find the secret. The failure arrives at the first +authenticated request rather than at boot, which is the worst place for it. + +Everything now uses `SEAMLESS_JWKS_PUBLIC_KEYS`: the entrypoint check, the +`/.well-known/jwks.json` handler, the configuration reference, and `.env.example`. + +**Breaking for anyone setting only the unprefixed name**, who is already broken and +does not know it. After this they fail to start, with the variable named, instead of +serving a deployment that cannot verify a token it just issued. + +**Rotation is documented rather than implemented.** The acceptance criteria in the +rotation issue are met by the read path that already exists: the document is a list, +every key in it is published and can verify, and only the active kid signs. What was +missing was a written procedure, which `docs/production-operations.md` now carries as +the three-step overlap (add, flip, retire), including why the steps cannot be +collapsed and why key ids must be environment-variable safe. + +The server deliberately does not rotate its own keys. It has no secret-store write +path, and environment variables are fixed for a process's lifetime, so a server that +rotated could not observe the result without a restart it cannot trigger. The document +belongs to whatever manages the secrets. + +Accordingly the empty `ensureKeys()` production branch is removed, along with +`initKeys`, their specs and the dev-stack invocation. It advertised a runtime rotation +capability that is not going to exist, and its development branch wrote a keypair to +`./keys` that nothing has ever read: `signingKeyStore` keeps dev keys under +`./keys/dev` and generates them lazily. diff --git a/.env.example b/.env.example index c5b7245..1761e18 100644 --- a/.env.example +++ b/.env.example @@ -165,7 +165,7 @@ MESSAGING_TWILIO_AUTH_TOKEN= # Required when NODE_ENV=production. # SEAMLESS_JWKS_ACTIVE_KID=main_2026_04 # SEAMLESS_JWKS_KEY_main_2026_04_PRIVATE="-----BEGIN PRIVATE KEY-----..." -# JWKS_PUBLIC_KEYS={"keys":[{"kid":"main_2026_04","pem":"-----BEGIN PUBLIC KEY-----...","createdAt":"2026-04-22T00:00:00.000Z"}]} +# SEAMLESS_JWKS_PUBLIC_KEYS={"keys":[{"kid":"main_2026_04","pem":"-----BEGIN PUBLIC KEY-----...","createdAt":"2026-04-22T00:00:00.000Z"}]} # FIDO2 CONFORMANCE # Mounts the FIDO2 conformance test interface at /conformance. That surface takes no diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index c46b625..7b9c28c 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -25,13 +25,13 @@ services: - node-modules-dev:/app/node_modules # Dockerfile.dev has no build step, so the compiled entrypoint cannot run from a # fresh clone. Install any dependencies added since this container last booted, - # generate dev signing keys, migrate (creating the database on first boot), then - # run the watcher against the TypeScript sources. + # migrate (creating the database on first boot), then run the watcher against the + # TypeScript sources. Dev signing keys are generated lazily by signingKeyStore on + # first use, so nothing has to create them up front. entrypoint: ['/bin/sh', '-c'] command: - > ./syncDevDeps.sh && - npx tsx src/scripts/initKeys.ts && (npm run migrate:up || (npm run db:create && npm run migrate:up)) && npm run dev:container depends_on: diff --git a/docs/configuration.md b/docs/configuration.md index 2dd8583..868f2bf 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -245,11 +245,11 @@ combinations (for example, Twilio requires SID, auth token, and a from-number). Required when `NODE_ENV=production`. In development, signing keys are generated locally. -| Variable | Required | Default | Notes | -| --------------------------------- | --------- | ------- | ---------------------------------------------------------------------------------- | -| `SEAMLESS_JWKS_ACTIVE_KID` | Prod only | - | Key ID of the active signing key. | -| `SEAMLESS_JWKS_KEY__PRIVATE` | Prod only | - | PKCS8 private key PEM for the active KID. | -| `JWKS_PUBLIC_KEYS` | Prod only | - | JSON `{ "keys": [{ "kid", "pem", "createdAt" }] }` published at the JWKS endpoint. | +| Variable | Required | Default | Notes | +| --------------------------------- | --------- | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `SEAMLESS_JWKS_ACTIVE_KID` | Prod only | - | Key ID of the active signing key. | +| `SEAMLESS_JWKS_KEY__PRIVATE` | Prod only | - | PKCS8 private key PEM for the active KID. | +| `SEAMLESS_JWKS_PUBLIC_KEYS` | Prod only | - | JSON `{ "keys": [{ "kid", "pem", "createdAt" }] }`. Every published key, not just the active one: the endpoint serves them all and token verification resolves by `kid`, which is what makes a rotation overlap rather than cut over. | See [docs/production-operations.md](./production-operations.md) for key rotation. diff --git a/docs/production-operations.md b/docs/production-operations.md index 8f3a3e3..6e234c6 100644 --- a/docs/production-operations.md +++ b/docs/production-operations.md @@ -24,7 +24,7 @@ Production deployments should define: - `OAUTH_STATE_SECRET` - `SEAMLESS_JWKS_ACTIVE_KID` - `SEAMLESS_JWKS_KEY__PRIVATE` -- `JWKS_PUBLIC_KEYS` +- `SEAMLESS_JWKS_PUBLIC_KEYS` - OAuth client-secret environment variables referenced by provider `clientSecretEnv` - Messaging provider credentials when direct delivery is enabled @@ -32,17 +32,52 @@ Do not store raw secrets in `system_config`. ## Signing Keys -Access tokens are signed with configured JWKS signing keys. A typical rotation is: +### What the server does, and does not do -1. Generate a new key pair. -2. Publish the new public key in `JWKS_PUBLIC_KEYS`. -3. Deploy with both old and new public keys available. -4. Switch `SEAMLESS_JWKS_ACTIVE_KID` to the new key id. -5. Keep retired public keys until all tokens signed with them expire. -6. Remove retired public keys after the token TTL window. +`SEAMLESS_JWKS_PUBLIC_KEYS` holds **every** published key, not just the active one. The server +treats it as a list: -Use key ids with letters, numbers, and underscores because the active key id is used to derive the -private-key environment variable name: `SEAMLESS_JWKS_KEY__PRIVATE`. +- `/.well-known/jwks.json` publishes all of them. +- Token verification resolves a key by the `kid` in the token's own header, so any published key + can verify a token it signed. +- Only `SEAMLESS_JWKS_ACTIVE_KID`, and that key's private half, is used to sign. + +That separation is what makes rotation an overlap rather than a cutover. Nothing has to be +invalidated to change signing keys. + +**The server never writes any of this.** It has no secret-store write path and no cloud SDK, and +environment variables are fixed for a process's lifetime, so a server that rotated its own keys +could not observe the result without a restart it cannot trigger. Whatever manages your secrets +owns the document; the server reads it. + +### Rotating without downtime + +Three steps, each one deployed before the next. Do not collapse them: the overlap is the whole +mechanism. + +1. **Add.** Generate a key pair, add its public half to `SEAMLESS_JWKS_PUBLIC_KEYS`, and deploy. + The outgoing key still signs. Verifiers now accept both. +2. **Flip.** Point `SEAMLESS_JWKS_ACTIVE_KID` at the new key id, inject its + `SEAMLESS_JWKS_KEY__PRIVATE`, and deploy. The new key signs. Tokens already issued keep + verifying, because the outgoing public key is still published. +3. **Retire.** Once `refresh_token_ttl` has passed, so no token signed by the outgoing key can + still be live, remove it from the document and drop its private key. Deploy. + +Each step needs a restart to take effect, because the values arrive as environment variables. The +server re-reads the document every five minutes within a process, which covers a secret updated in +place, but a changed environment variable needs a new process. + +Key ids must be letters, digits and underscores, and must not start with a digit, because the +private-key variable name is derived from the id: `SEAMLESS_JWKS_KEY__PRIVATE`. A key id with +a hyphen produces a variable name that cannot be set. + +### Managed deployments + +For instances built with `seamless-iac`, this is already automated and should not be done by hand. +Terraform owns the document, signing keys are a set of labels with `jwks_active_key_label` +selecting the signer, and the three steps above are three applies. See ADR 0011, +`Terraform owns the JWKS document, and rotation overlaps`, and the procedure in that repo's +`portal-auth/README.md`. ## Refresh Tokens diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index 8bd6c88..322cf29 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 99% + + coverage: 99.1% @@ -7,17 +7,17 @@ - + - - + + coverage coverage - 99% - 99% + 99.1% + 99.1% diff --git a/src/controllers/jwks.ts b/src/controllers/jwks.ts index 076a700..aaad194 100644 --- a/src/controllers/jwks.ts +++ b/src/controllers/jwks.ts @@ -29,7 +29,7 @@ export function __resetJwksCache() { async function loadJwksFromSecrets(): Promise { logger.info('Loading JWKS from Secrets Manager'); - const raw = await getSecret('JWKS_PUBLIC_KEYS'); + const raw = await getSecret('SEAMLESS_JWKS_PUBLIC_KEYS'); const parsed = JSON.parse(raw); const jwks: JWK[] = []; diff --git a/src/scripts/initKeys.ts b/src/scripts/initKeys.ts deleted file mode 100644 index 26a7399..0000000 --- a/src/scripts/initKeys.ts +++ /dev/null @@ -1,12 +0,0 @@ -/* - * Copyright © 2026 Fells Code, LLC - * Licensed under the GNU Affero General Public License v3.0 - * See LICENSE file in the project root for full license information - */ - -import { ensureKeys } from './keyManager.js'; - -async function init() { - await ensureKeys(); -} -init(); diff --git a/src/scripts/keyManager.ts b/src/scripts/keyManager.ts deleted file mode 100644 index 0cf82bb..0000000 --- a/src/scripts/keyManager.ts +++ /dev/null @@ -1,74 +0,0 @@ -/* - * Copyright © 2026 Fells Code, LLC - * Licensed under the GNU Affero General Public License v3.0 - * See LICENSE file in the project root for full license information - */ - -import crypto from 'crypto'; -import { access, mkdir, writeFile } from 'fs/promises'; -import path from 'path'; - -import getLogger from '../utils/logger.js'; - -const logger = getLogger('jwks-bootstrap'); - -const isProduction = process.env.NODE_ENV === 'production'; - -const localKeyDir = path.resolve('./keys'); -const localPrivate = path.join(localKeyDir, 'private.pem'); -const localPublic = path.join(localKeyDir, 'public.pem'); - -// A fast path only: the exclusive write below is what actually settles a race, so a -// stale answer here costs a wasted keypair, never a clobbered one. -async function pathExists(target: string) { - try { - await access(target); - return true; - } catch { - return false; - } -} - -// Create local keys if needed -async function ensureLocalDevKeys() { - await mkdir(localKeyDir, { recursive: true }); - - if (await pathExists(localPrivate)) { - logger.info('Dev keys already exist.'); - return; - } - - logger.info('Generating new dev RSA keypair...'); - const { publicKey, privateKey } = crypto.generateKeyPairSync('rsa', { - modulusLength: 2048, - publicKeyEncoding: { type: 'spki', format: 'pem' }, - privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, - }); - - try { - // Exclusive create rather than exists-then-write: the pair must come from one - // generation, and a concurrent run must not replace a private key whose public - // half has already been published. - await writeFile(localPrivate, privateKey, { flag: 'wx' }); - } catch (error) { - if ((error as { code?: string }).code === 'EEXIST') { - logger.info('Dev keys already exist.'); - return; - } - throw error; - } - - await writeFile(localPublic, publicKey); - logger.info('Dev keypair created in ./keys/'); -} - -export async function ensureKeys() { - if (!isProduction) { - // local development mode - logger.info('Running in dev mode → using local keys.'); - return ensureLocalDevKeys(); - } - - // PRODUCTION MODE - // Implement a first time JWKS rotation. See Seamless Auth docs for guides -} diff --git a/tests/integration/jwks/jwks.spec.ts b/tests/integration/jwks/jwks.spec.ts index 14a357a..4beeefe 100644 --- a/tests/integration/jwks/jwks.spec.ts +++ b/tests/integration/jwks/jwks.spec.ts @@ -77,7 +77,7 @@ describe('JWKS - Production Mode', () => { expect(res.status).toBe(200); expect(res.body.keys[0].kid).toBe('key-1'); - expect(getSecret).toHaveBeenCalledWith('JWKS_PUBLIC_KEYS'); + expect(getSecret).toHaveBeenCalledWith('SEAMLESS_JWKS_PUBLIC_KEYS'); expect(res.headers['cache-control']).toContain('max-age=300'); }); }); diff --git a/tests/unit/scripts/initKeys.spec.ts b/tests/unit/scripts/initKeys.spec.ts deleted file mode 100644 index 7797912..0000000 --- a/tests/unit/scripts/initKeys.spec.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { vi } from 'vitest'; - -vi.mock('../../../src/scripts/keyManager', () => ({ - ensureKeys: vi.fn(), -})); - -import { describe, it, expect } from 'vitest'; - -describe('init script', () => { - it('calls ensureKeys on import', async () => { - const { ensureKeys } = await import('../../../src/scripts/keyManager'); - - await import('../../../src/scripts/initKeys'); - - expect(ensureKeys).toHaveBeenCalled(); - }); -}); diff --git a/tests/unit/scripts/keyManager.spec.ts b/tests/unit/scripts/keyManager.spec.ts deleted file mode 100644 index b9deadf..0000000 --- a/tests/unit/scripts/keyManager.spec.ts +++ /dev/null @@ -1,92 +0,0 @@ -import { vi } from 'vitest'; - -vi.mock('fs/promises', () => ({ - access: vi.fn(), - mkdir: vi.fn(), - writeFile: vi.fn(), -})); - -vi.mock('crypto', async () => { - const actual = await vi.importActual('crypto'); - return { - ...actual, - generateKeyPairSync: vi.fn(), - }; -}); - -import { describe, it, expect, beforeEach } from 'vitest'; - -describe('keyManager', () => { - beforeEach(() => { - vi.resetModules(); - vi.clearAllMocks(); - delete process.env.NODE_ENV; - }); - - it('runs dev key setup when not production', async () => { - vi.stubEnv('NODE_ENV', 'development'); - - const crypto = await import('crypto'); - const fsp = await import('fs/promises'); - - (fsp.access as any).mockRejectedValue(new Error('ENOENT')); - - (crypto.generateKeyPairSync as any).mockReturnValue({ - publicKey: 'PUBLIC', - privateKey: 'PRIVATE', - }); - - const { ensureKeys } = await import('../../../src/scripts/keyManager'); - - await ensureKeys(); - - expect(fsp.mkdir).toHaveBeenCalled(); - expect(fsp.writeFile).toHaveBeenCalledTimes(2); - }); - - it('does nothing in production', async () => { - vi.stubEnv('NODE_ENV', 'production'); - - const { ensureKeys } = await import('../../../src/scripts/keyManager'); - - await ensureKeys(); - - // no filesystem interaction - const fsp = await import('fs/promises'); - expect(fsp.mkdir).not.toHaveBeenCalled(); - }); - - it('skips generation if keys already exist', async () => { - vi.stubEnv('NODE_ENV', 'development'); - - const fsp = await import('fs/promises'); - - (fsp.access as any).mockResolvedValue(undefined); - - const { ensureKeys } = await import('../../../src/scripts/keyManager'); - - await ensureKeys(); - - expect(fsp.writeFile).not.toHaveBeenCalled(); - }); - - it('generates keys when missing', async () => { - vi.stubEnv('NODE_ENV', 'development'); - - const crypto = await import('crypto'); - const fsp = await import('fs/promises'); - - (fsp.access as any).mockRejectedValue(new Error('ENOENT')); - - (crypto.generateKeyPairSync as any).mockReturnValue({ - publicKey: 'PUBLIC_KEY', - privateKey: 'PRIVATE_KEY', - }); - - const { ensureKeys } = await import('../../../src/scripts/keyManager'); - - await ensureKeys(); - - expect(fsp.writeFile).toHaveBeenCalledTimes(2); - }); -}); diff --git a/tests/unit/utils/keyRotationOverlap.spec.ts b/tests/unit/utils/keyRotationOverlap.spec.ts new file mode 100644 index 0000000..497cc4a --- /dev/null +++ b/tests/unit/utils/keyRotationOverlap.spec.ts @@ -0,0 +1,147 @@ +import { generateKeyPairSync } from 'crypto'; +import { importPKCS8, importSPKI, jwtVerify, SignJWT } from 'jose'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../../src/utils/secretsStore.js', () => ({ + getSecret: vi.fn(), +})); + +/** + * The rotation contract, exercised end to end with real keys rather than asserted + * about. + * + * A rotation is an overlap: the incoming key is published alongside the outgoing one, + * the active kid flips, and the outgoing key stays published until every token it + * signed has expired. Nothing here writes keys. The server reads the document, and + * whatever manages the secrets owns it (seamless-iac ADR 0011). + * + * The property that makes it work is that verification resolves a key by the `kid` in + * the token's own header, so a token outlives the key's time as the signer. + */ +function keypair() { + return generateKeyPairSync('rsa', { + modulusLength: 2048, + publicKeyEncoding: { type: 'spki', format: 'pem' }, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, + }); +} + +const outgoing = keypair(); +const incoming = keypair(); + +const publicKeysDocument = JSON.stringify({ + keys: [ + { kid: 'key_a', pem: outgoing.publicKey, createdAt: '2026-01-01T00:00:00.000Z' }, + { kid: 'key_b', pem: incoming.publicKey, createdAt: '2026-06-01T00:00:00.000Z' }, + ], +}); + +/** Serves the secrets the way the deployment does, by name rather than call order. */ +function serveSecrets(activeKid: string) { + return async (name: string) => { + if (name === 'SEAMLESS_JWKS_ACTIVE_KID') return activeKid; + if (name === 'SEAMLESS_JWKS_PUBLIC_KEYS') return publicKeysDocument; + if (name === 'SEAMLESS_JWKS_KEY_key_a_PRIVATE') return outgoing.privateKey; + if (name === 'SEAMLESS_JWKS_KEY_key_b_PRIVATE') return incoming.privateKey; + throw new Error(`Secret "${name}" is not defined.`); + }; +} + +async function loadStore(activeKid: string) { + vi.resetModules(); + vi.stubEnv('NODE_ENV', 'production'); + + const { getSecret } = await import('../../../src/utils/secretsStore.js'); + (getSecret as ReturnType).mockImplementation(serveSecrets(activeKid)); + + return import('../../../src/utils/signingKeyStore.js'); +} + +async function signWith(privateKeyPem: string, kid: string) { + return new SignJWT({ sub: 'user-1' }) + .setProtectedHeader({ alg: 'RS256', kid }) + .setIssuedAt() + .setExpirationTime('15m') + .sign(await importPKCS8(privateKeyPem, 'RS256')); +} + +beforeEach(() => { + vi.clearAllMocks(); + vi.unstubAllEnvs(); +}); + +describe('signing key rotation overlap', () => { + it('signs with the active key and no other', async () => { + const before = await loadStore('key_a'); + expect((await before.getSigningKey()).kid).toBe('key_a'); + + const after = await loadStore('key_b'); + expect((await after.getSigningKey()).kid).toBe('key_b'); + }); + + it('keeps resolving the outgoing key after the active kid has moved on', async () => { + const store = await loadStore('key_b'); + + expect(await store.getPublicKeyByKid('key_a')).toBe(outgoing.publicKey); + expect(await store.getPublicKeyByKid('key_b')).toBe(incoming.publicKey); + }); + + /** + * The acceptance criterion of #175: a token issued before a rotation stays valid for + * its lifetime. Signed with the outgoing key, then verified against the key the + * store hands back once the incoming key has taken over signing. + */ + it('verifies a token signed before the rotation', async () => { + const beforeRotation = await loadStore('key_a'); + const { kid, privateKeyPem } = await beforeRotation.getSigningKey(); + const token = await signWith(privateKeyPem, kid); + + const afterRotation = await loadStore('key_b'); + expect((await afterRotation.getSigningKey()).kid).toBe('key_b'); + + const pem = await afterRotation.getPublicKeyByKid(kid); + const { payload } = await jwtVerify(token, await importSPKI(pem!, 'RS256')); + + expect(payload.sub).toBe('user-1'); + }); + + /** + * Step three of the procedure. Once the retired key leaves the document the tokens + * it signed stop verifying, which is why the docs tie that step to refresh_token_ttl + * rather than to the rotation. + */ + it('stops resolving a retired key once it leaves the document', async () => { + vi.resetModules(); + vi.stubEnv('NODE_ENV', 'production'); + + const { getSecret } = await import('../../../src/utils/secretsStore.js'); + (getSecret as ReturnType).mockImplementation(async (name: string) => { + if (name === 'SEAMLESS_JWKS_ACTIVE_KID') return 'key_b'; + if (name === 'SEAMLESS_JWKS_PUBLIC_KEYS') { + return JSON.stringify({ + keys: [{ kid: 'key_b', pem: incoming.publicKey, createdAt: '2026-06-01T00:00:00.000Z' }], + }); + } + throw new Error(`Secret "${name}" is not defined.`); + }); + + const store = await import('../../../src/utils/signingKeyStore.js'); + + expect(await store.getPublicKeyByKid('key_a')).toBeNull(); + expect(await store.getPublicKeyByKid('key_b')).toBe(incoming.publicKey); + }); + + /** + * The split that made this reachable: validateEnvs.sh required the unprefixed name + * while verification read the prefixed one, so a deployment following the documented + * contract booted and then failed every verification. Both names are now the same one. + */ + it('reads the public keys document under the documented name', async () => { + const store = await loadStore('key_b'); + const { getSecret } = await import('../../../src/utils/secretsStore.js'); + + await store.getPublicKeyByKid('key_b'); + + expect(getSecret).toHaveBeenCalledWith('SEAMLESS_JWKS_PUBLIC_KEYS'); + }); +}); diff --git a/validateEnvs.sh b/validateEnvs.sh index ac3459d..a5c8341 100644 --- a/validateEnvs.sh +++ b/validateEnvs.sh @@ -52,7 +52,7 @@ require_var API_SERVICE_TOKEN if [ "${NODE_ENV:-development}" = "production" ]; then require_var SEAMLESS_JWKS_ACTIVE_KID - require_var JWKS_PUBLIC_KEYS + require_var SEAMLESS_JWKS_PUBLIC_KEYS active_kid="${SEAMLESS_JWKS_ACTIVE_KID}" private_key_var="SEAMLESS_JWKS_KEY_${active_kid}_PRIVATE"