diff --git a/settings.json.template b/settings.json.template index 5f5ae29f2..5c01460a6 100644 --- a/settings.json.template +++ b/settings.json.template @@ -966,6 +966,14 @@ "sso": { "issuer": "${SSO_ISSUER:http://localhost:9001}", + /* + * Signing keys for the embedded OIDC provider's cookies. Leave empty and + * Etherpad derives a secret key from the persisted session secret + * (SESSIONKEY.txt), which is stable across restarts and shared across + * horizontally-scaled pods. Set an explicit ordered array to rotate: the + * first key signs, the rest are still accepted for verify. + */ + "cookieKeys": ["${OIDC_COOKIE_KEY:}"], "clients": [ { "client_id": "${ADMIN_CLIENT:admin_client}", diff --git a/src/node/security/OAuth2Provider.ts b/src/node/security/OAuth2Provider.ts index 8bfb46831..551ca32f9 100644 --- a/src/node/security/OAuth2Provider.ts +++ b/src/node/security/OAuth2Provider.ts @@ -10,6 +10,12 @@ import {format} from 'url' import {ParsedUrlQuery} from "node:querystring"; import {MapArrayType} from "../types/MapType"; import crypto from "node:crypto"; +import {resolveOidcCookieKeys, isOriginAllowedForOidcClient} from "./OidcProviderSecurity"; +import SecretRotator from "./SecretRotator"; + +// Held at module scope so the rotator (and its refresh timer) is not garbage +// collected for the lifetime of the process, mirroring express.ts. +let oidcCookieSecretRotator: SecretRotator | null = null; // Small fixed delay applied to every failed interactive login, mirroring // webaccess.authnFailureDelayMs, so failures take a consistent amount of time. @@ -68,9 +74,13 @@ const configuration: Configuration = { profile: ['name'], admin: ['admin'] }, - cookies: { - keys: ['oidc'], - }, + // NOTE: cookies.keys is deliberately NOT set here. A committed literal key + // (historically ['oidc']) lets anyone with the public source forge valid + // OIDC provider `.sig` cookies. The real key material is resolved at + // provider-construction time in expressCreateServer() via + // resolveOidcCookieKeys(), which prefers an operator-supplied + // settings.sso.cookieKeys and otherwise derives a secret, stable key from + // the persisted session secret. See OidcProviderSecurity.ts. features:{ devInteractions: {enabled: false}, }, @@ -92,8 +102,39 @@ export const expressCreateServer = async (hookName: string, args: ArgsExpressTyp publicKeyExported = publicKey privateKeyExported = privateKey + // Resolve the OIDC provider's cookie-signing keys. Never a committed literal. + // When the operator hasn't pinned settings.sso.cookieKeys and cookie key + // rotation is enabled (the default), reuse the same DB-backed SecretRotator + // mechanism as the Express session cookies so the key is stable across + // restarts and shared across horizontally-scaled pods. `settings.sessionKey` + // is commonly null under default rotation settings, so relying on it alone + // would hand the provider an unstable per-process random key. + const operatorCookieKeys = (settings.sso as {cookieKeys?: unknown}).cookieKeys; + const hasOperatorKeys = Array.isArray(operatorCookieKeys) && + operatorCookieKeys.some((k) => typeof k === 'string' && k.length > 0); + const {keyRotationInterval, sessionLifetime} = settings.cookie; + let rotatedSecrets: string[] | null = null; + if (!hasOperatorKeys && keyRotationInterval && sessionLifetime) { + if (oidcCookieSecretRotator == null) { + oidcCookieSecretRotator = new SecretRotator( + 'oidcCookieSecrets', keyRotationInterval, sessionLifetime, settings.sessionKey); + await oidcCookieSecretRotator.start(); + } + rotatedSecrets = oidcCookieSecretRotator.secrets; + } + const oidc = new Provider(settings.sso.issuer, { - ...configuration, jwks: { + ...configuration, + cookies: { + // Secret, deployment-stable signing keys — never a committed literal. + // See resolveOidcCookieKeys(). + keys: resolveOidcCookieKeys({ + cookieKeys: operatorCookieKeys, + rotatedSecrets, + sessionKey: settings.sessionKey, + }), + }, + jwks: { keys: [ privateKeyJWK ], @@ -127,9 +168,12 @@ export const expressCreateServer = async (hookName: string, args: ArgsExpressTyp }, jwtResponseModes: {enabled: true}, }, - clientBasedCORS: (ctx, origin, client) => { - return true - }, + clientBasedCORS: (ctx, origin, client) => + // Only allow cross-origin reads from an origin registered as one of + // the client's redirect URIs. Returning `true` unconditionally + // reflected any Origin into Access-Control-Allow-Origin. See + // isOriginAllowedForOidcClient(). + isOriginAllowedForOidcClient(origin, client as {redirectUris?: unknown}), extraParams: [], extraTokenClaims: async (ctx, token) => { if(token.kind === 'AccessToken') { diff --git a/src/node/security/OidcProviderSecurity.ts b/src/node/security/OidcProviderSecurity.ts new file mode 100644 index 000000000..a5bfc6c1f --- /dev/null +++ b/src/node/security/OidcProviderSecurity.ts @@ -0,0 +1,96 @@ +import crypto from 'node:crypto'; + +/** + * Security helpers for the embedded OIDC provider (`OAuth2Provider.ts`). + * + * Kept in a dependency-light module (only `node:crypto`) so the pure decisions + * can be unit-tested without constructing an `oidc-provider` instance. + */ + +// Domain-separation label so the derived cookie key is cryptographically +// unrelated to any other use of the session secret. The trailing NUL keeps the +// label from ambiguously running into the appended secret. +export const COOKIE_KEY_DERIVATION_LABEL = 'etherpad-oidc-provider-cookie-signing-key\0'; + +/** + * Resolve the cookie-signing keys for the embedded OIDC provider. + * + * oidc-provider signs its short-lived interaction/session/grant cookies + * (`_interaction`, `_interaction_resume`, `_grant`, `_session`) with an HMAC + * keyed by these values. Shipping a hardcoded key (historically `['oidc']`) let + * anyone with the public source forge valid `.sig` cookies, defeating the + * provider's cookie-integrity guarantee. Reported by `meifukun`. + * + * Resolution order: + * 1. An explicit operator-supplied `settings.sso.cookieKeys` array — use this + * for controlled rotation: `[newKey, ...oldKeys]`. + * 2. The live secrets array of a DB-backed `SecretRotator` (the same mechanism + * the Express session-cookie stack uses). This is the default path: it is + * stable across restarts, shared across horizontally-scaled pods via the + * database, and rotates automatically. The array is returned BY REFERENCE + * so a later in-place rotation propagates to oidc-provider/keygrip without + * reconstructing the provider. + * 3. A value derived from the persisted Etherpad session secret + * (`SESSIONKEY.txt`) via a domain-separated SHA-256 — used when rotation is + * disabled but a static session key exists. Stable across restarts/pods, + * never committed to source. + * 4. As a last resort (no key material at all), an ephemeral per-process + * random key. The integrity boundary holds, but interactions won't survive + * a restart or span multiple pods. + */ +export const resolveOidcCookieKeys = ( + opts: {cookieKeys?: unknown, rotatedSecrets?: string[] | null, sessionKey?: string | null}, +): string[] => { + const {cookieKeys, rotatedSecrets, sessionKey} = opts; + + if (Array.isArray(cookieKeys)) { + const usable = cookieKeys.filter((k): k is string => typeof k === 'string' && k.length > 0); + if (usable.length > 0) return usable; + } + + // Return the rotator's array by reference (do not copy/filter) so in-place + // rotation is observed live by keygrip. + if (Array.isArray(rotatedSecrets) && + rotatedSecrets.some((k) => typeof k === 'string' && k.length > 0)) { + return rotatedSecrets; + } + + if (typeof sessionKey === 'string' && sessionKey.length > 0) { + const derived = crypto.createHash('sha256') + .update(COOKIE_KEY_DERIVATION_LABEL) + .update(sessionKey) + .digest('hex'); + return [derived]; + } + + return [crypto.randomBytes(32).toString('hex')]; +}; + +/** + * Origin allow-list decision for the embedded OIDC provider's CORS-enabled + * endpoints (`/oidc/token`, `/oidc/me`, ...). oidc-provider invokes + * `clientBasedCORS(ctx, origin, client)` for every cross-origin request; + * returning `true` unconditionally (the historical behavior) reflected ANY + * `Origin` into `Access-Control-Allow-Origin`, letting unregistered origins read + * token/userinfo responses that use non-cookie credentials (Authorization + * headers, POST-body client credentials). Reported by `meifukun`. + * + * An origin is allowed only when it exactly matches the origin (scheme + host + + * port) of one of the client's registered redirect URIs. + */ +export const isOriginAllowedForOidcClient = ( + origin: string | undefined | null, + client: {redirectUris?: unknown} | undefined | null, +): boolean => { + if (!origin || !client) return false; + const uris = (client as {redirectUris?: unknown}).redirectUris; + if (!Array.isArray(uris)) return false; + return uris.some((uri: unknown) => { + if (typeof uri !== 'string') return false; + try { + return new URL(uri).origin === origin; + } catch { + return false; + } + }); +}; diff --git a/src/node/utils/Settings.ts b/src/node/utils/Settings.ts index 9d8aa3f1c..b0762b72f 100644 --- a/src/node/utils/Settings.ts +++ b/src/node/utils/Settings.ts @@ -304,6 +304,10 @@ export type SettingsType = { sso: { issuer: string, clients?: {client_id: string}[] + // Optional operator-supplied signing keys for the embedded OIDC provider's + // cookies. When unset, a secret key is derived from the session secret. + // Provide an ordered array `[newKey, ...oldKeys]` to rotate. + cookieKeys?: string[] }, showSettingsInAdminPage: boolean, cleanup: { diff --git a/src/tests/backend/specs/OidcProviderSecurity.ts b/src/tests/backend/specs/OidcProviderSecurity.ts new file mode 100644 index 000000000..792e6013d --- /dev/null +++ b/src/tests/backend/specs/OidcProviderSecurity.ts @@ -0,0 +1,128 @@ +'use strict'; + +/** + * Unit coverage for the embedded OIDC provider's cookie-signing key derivation + * and CORS origin allow-list. Both were reported by `meifukun`: + * - the provider historically signed its cookies with the hardcoded key + * `['oidc']`, so anyone with the public source could forge valid `.sig` + * cookies; + * - `clientBasedCORS` returned `true` for every origin, reflecting arbitrary + * `Origin` values into `Access-Control-Allow-Origin`. + */ + +const assert = require('assert').strict; +import { + resolveOidcCookieKeys, + isOriginAllowedForOidcClient, +} from '../../../node/security/OidcProviderSecurity'; + +describe(__filename, function () { + describe('resolveOidcCookieKeys', function () { + it('never returns the historical hardcoded key', function () { + const keys = resolveOidcCookieKeys({sessionKey: 'a-persisted-session-secret'}); + assert.ok(!keys.includes('oidc')); + }); + + it('uses operator-supplied cookieKeys when provided', function () { + const keys = resolveOidcCookieKeys({cookieKeys: ['k1', 'k2'], sessionKey: 'x'}); + assert.deepEqual(keys, ['k1', 'k2']); + }); + + it('ignores empty/invalid entries in cookieKeys and falls through', function () { + const keys = resolveOidcCookieKeys({cookieKeys: ['', null as any], sessionKey: 'secret'}); + assert.equal(keys.length, 1); + assert.notEqual(keys[0], 'oidc'); + assert.ok(keys[0].length >= 32); + }); + + it('prefers rotated DB-backed secrets over the session-key derivation', function () { + const rotated = ['rot-new', 'rot-old']; + const keys = resolveOidcCookieKeys({rotatedSecrets: rotated, sessionKey: 'secret'}); + assert.deepEqual(keys, ['rot-new', 'rot-old']); + }); + + it('returns the live rotatedSecrets array by reference (so rotation propagates)', function () { + // oidc-provider/keygrip holds the array by reference and reads it live on + // each sign/verify, so returning the same object means a rotation that + // mutates the array in place is picked up without reconstructing the provider. + const rotated = ['rot-new']; + const keys = resolveOidcCookieKeys({rotatedSecrets: rotated, sessionKey: 'secret'}); + assert.strictEqual(keys, rotated); + }); + + it('ignores an empty rotatedSecrets array and falls through', function () { + const keys = resolveOidcCookieKeys({rotatedSecrets: [], sessionKey: 'secret'}); + assert.equal(keys.length, 1); + assert.notEqual(keys[0], 'oidc'); + // fell through to the session-key derivation (stable, deterministic) + assert.deepEqual(keys, resolveOidcCookieKeys({sessionKey: 'secret'})); + }); + + it('lets operator cookieKeys win over rotated secrets', function () { + const keys = resolveOidcCookieKeys({ + cookieKeys: ['operator'], rotatedSecrets: ['rot'], sessionKey: 'secret', + }); + assert.deepEqual(keys, ['operator']); + }); + + it('derives a stable key from the session secret (survives restart/multi-pod)', function () { + const a = resolveOidcCookieKeys({sessionKey: 'secret'}); + const b = resolveOidcCookieKeys({sessionKey: 'secret'}); + assert.deepEqual(a, b); + }); + + it('derives different keys for different session secrets', function () { + const a = resolveOidcCookieKeys({sessionKey: 'secret-a'}); + const b = resolveOidcCookieKeys({sessionKey: 'secret-b'}); + assert.notDeepEqual(a, b); + }); + + it('does not reuse the raw session secret as the cookie key', function () { + const keys = resolveOidcCookieKeys({sessionKey: 'secret'}); + assert.ok(!keys.includes('secret')); + }); + + it('falls back to a fresh random key when no session secret exists', function () { + const a = resolveOidcCookieKeys({sessionKey: null}); + const b = resolveOidcCookieKeys({sessionKey: null}); + assert.equal(a.length, 1); + assert.notEqual(a[0], 'oidc'); + assert.ok(a[0].length >= 32); + assert.notDeepEqual(a, b); // random => different each call + }); + }); + + describe('isOriginAllowedForOidcClient', function () { + const client = { + redirectUris: ['https://app.example.com/admin/', 'https://app.example.com/'], + }; + + it('allows an origin matching a registered redirect URI', function () { + assert.equal(isOriginAllowedForOidcClient('https://app.example.com', client), true); + }); + + it('rejects an unregistered attacker origin', function () { + assert.equal(isOriginAllowedForOidcClient('https://evil.attacker.com', client), false); + }); + + it('rejects a look-alike suffix origin (no substring matching)', function () { + assert.equal(isOriginAllowedForOidcClient('https://app.example.com.evil.com', client), false); + }); + + it('rejects a scheme mismatch (http vs https)', function () { + assert.equal(isOriginAllowedForOidcClient('http://app.example.com', client), false); + }); + + it('rejects when origin is missing', function () { + assert.equal(isOriginAllowedForOidcClient(undefined, client), false); + }); + + it('rejects when client is missing', function () { + assert.equal(isOriginAllowedForOidcClient('https://app.example.com', null), false); + }); + + it('rejects when client has no redirect URIs', function () { + assert.equal(isOriginAllowedForOidcClient('https://app.example.com', {}), false); + }); + }); +}); diff --git a/src/tests/backend/specs/oidcCookieRotator.ts b/src/tests/backend/specs/oidcCookieRotator.ts new file mode 100644 index 000000000..f252f35de --- /dev/null +++ b/src/tests/backend/specs/oidcCookieRotator.ts @@ -0,0 +1,54 @@ +'use strict'; + +/** + * Integration coverage for the OIDC provider cookie-key source under the + * DEFAULT deployment config (cookie key rotation enabled, no SESSIONKEY.txt so + * `settings.sessionKey` is null). Without the DB-backed SecretRotator path the + * provider would fall back to a per-process random key, breaking OIDC cookies on + * restart and across horizontally-scaled pods. This test proves the default + * path yields a stable, non-random, DB-backed key. + */ + +const assert = require('assert').strict; +const common = require('../common'); +const SecretRotator = require('../../../node/security/SecretRotator').SecretRotator; +import {resolveOidcCookieKeys} from '../../../node/security/OidcProviderSecurity'; +import settings from '../../../node/utils/Settings'; + +describe(__filename, function () { + this.timeout(30000); + + before(async function () { + // Boots the server, which initialises the database the rotator writes to. + await common.init(); + }); + + it('default config yields DB-backed rotator secrets, not a random key', async function () { + const {keyRotationInterval, sessionLifetime} = settings.cookie; + assert.ok(keyRotationInterval && sessionLifetime, + 'precondition: cookie key rotation is enabled by default'); + + const rotator = new SecretRotator( + 'oidcCookieSecretsTest', keyRotationInterval, sessionLifetime, settings.sessionKey); + await rotator.start(); + try { + assert.ok(Array.isArray(rotator.secrets) && rotator.secrets.length > 0, + 'rotator produced at least one secret'); + assert.ok(rotator.secrets.every((s: any) => typeof s === 'string' && s.length > 0), + 'all rotator secrets are non-empty strings'); + + // The provider would select these rotated secrets (by reference) over any + // session-key derivation or random fallback. + const keys = resolveOidcCookieKeys({ + cookieKeys: (settings.sso as any).cookieKeys, + rotatedSecrets: rotator.secrets, + sessionKey: settings.sessionKey, + }); + assert.strictEqual(keys, rotator.secrets, 'resolver returns the live rotator array'); + assert.ok(!keys.includes('oidc'), 'never the historical hardcoded key'); + } finally { + // stop the refresh timer so it does not keep the event loop alive + if (typeof rotator.stop === 'function') await rotator.stop(); + } + }); +});