diff --git a/CHANGELOG.md b/CHANGELOG.md index 50a641627..1ba7547a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,11 @@ session expires (with some exceptions that will be fixed in the future). * Requests for static content (e.g., `/robots.txt`) and special pages (e.g., the HTTP API, `/stats`) no longer create login session state. + * The secret used to sign the `express_sid` cookie is now automatically + regenerated every day (called *key rotation*) by default. If key rotation is + enabled, the now-deprecated `SESSIONKEY.txt` file can be safely deleted + after Etherpad starts up (its content is read and saved to the database and + used to validate signatures from old cookies until they expire). * The following settings from `settings.json` are now applied as expected (they were unintentionally ignored before): * `padOptions.lang` diff --git a/doc/docker.adoc b/doc/docker.adoc index 75265ce76..d49c46510 100644 --- a/doc/docker.adoc +++ b/doc/docker.adoc @@ -370,6 +370,10 @@ For the editor container, you can also make it full width by adding `full-width- | Description | Default +|`COOKIE_KEY_ROTATION_INTERVAL` +|How often (ms) to rotate in a new secret for signing cookies +|`86400000` (1 day) + | `COOKIE_SAME_SITE` | Value of the SameSite cookie property. | `"Lax"` diff --git a/doc/docker.md b/doc/docker.md new file mode 100644 index 000000000..e69de29bb diff --git a/settings.json.docker b/settings.json.docker index 053b6b618..fbc1ea4f3 100644 --- a/settings.json.docker +++ b/settings.json.docker @@ -363,6 +363,23 @@ * Settings controlling the session cookie issued by Etherpad. */ "cookie": { + /* + * How often (in milliseconds) the key used to sign the express_sid cookie + * should be rotated. Long rotation intervals reduce signature verification + * overhead (because there are fewer historical keys to check) and database + * load (fewer historical keys to store, and less frequent queries to + * get/update the keys). Short rotation intervals are slightly more secure. + * + * Multiple Etherpad processes sharing the same database (table) is + * supported as long as the clock sync error is significantly less than this + * value. + * + * Key rotation can be disabled (not recommended) by setting this to 0 or + * null, or by disabling session expiration (see sessionLifetime). + */ + // 86400000 = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s + "keyRotationInterval": "${COOKIE_KEY_ROTATION_INTERVAL:86400000}", + /* * Value of the SameSite cookie property. "Lax" is recommended unless * Etherpad will be embedded in an iframe from another site, in which case @@ -392,6 +409,8 @@ * indefinitely without consulting authentication or authorization * hooks, so once a user has accessed a pad, the user can continue to * use the pad until the user leaves for longer than sessionLifetime. + * - More historical keys (sessionLifetime / keyRotationInterval) must be + * checked when verifying signatures. * * Session lifetime can be set to infinity (not recommended) by setting this * to null or 0. Note that if the session does not expire, most browsers diff --git a/settings.json.template b/settings.json.template index 375c4db9c..ea8f41812 100644 --- a/settings.json.template +++ b/settings.json.template @@ -364,6 +364,22 @@ * Settings controlling the session cookie issued by Etherpad. */ "cookie": { + /* + * How often (in milliseconds) the key used to sign the express_sid cookie + * should be rotated. Long rotation intervals reduce signature verification + * overhead (because there are fewer historical keys to check) and database + * load (fewer historical keys to store, and less frequent queries to + * get/update the keys). Short rotation intervals are slightly more secure. + * + * Multiple Etherpad processes sharing the same database (table) is + * supported as long as the clock sync error is significantly less than this + * value. + * + * Key rotation can be disabled (not recommended) by setting this to 0 or + * null, or by disabling session expiration (see sessionLifetime). + */ + "keyRotationInterval": 86400000, // = 1d * 24h/d * 60m/h * 60s/m * 1000ms/s + /* * Value of the SameSite cookie property. "Lax" is recommended unless * Etherpad will be embedded in an iframe from another site, in which case @@ -393,6 +409,8 @@ * indefinitely without consulting authentication or authorization * hooks, so once a user has accessed a pad, the user can continue to * use the pad until the user leaves for longer than sessionLifetime. + * - More historical keys (sessionLifetime / keyRotationInterval) must be + * checked when verifying signatures. * * Session lifetime can be set to infinity (not recommended) by setting this * to null or 0. Note that if the session does not expire, most browsers diff --git a/src/node/hooks/express.js b/src/node/hooks/express.js index 9c42fd6d8..98be763c2 100644 --- a/src/node/hooks/express.js +++ b/src/node/hooks/express.js @@ -1,6 +1,7 @@ 'use strict'; const _ = require('underscore'); +const SecretRotator = require('../security/SecretRotator'); const cookieParser = require('cookie-parser'); const events = require('events'); const express = require('express'); @@ -14,6 +15,7 @@ const stats = require('../stats'); const util = require('util'); const webaccess = require('./express/webaccess'); +let secretRotator = null; const logger = log4js.getLogger('http'); let serverName; let sessionStore; @@ -53,6 +55,8 @@ const closeServer = async () => { } if (sessionStore) sessionStore.shutdown(); sessionStore = null; + if (secretRotator) secretRotator.stop(); + secretRotator = null; }; exports.createServer = async () => { @@ -174,13 +178,23 @@ exports.restartServer = async () => { })); } - app.use(cookieParser(settings.sessionKey, {})); + const {keyRotationInterval, sessionLifetime} = settings.cookie; + let secret = settings.sessionKey; + if (keyRotationInterval && sessionLifetime) { + secretRotator = new SecretRotator( + 'expressSessionSecrets', keyRotationInterval, sessionLifetime, settings.sessionKey); + await secretRotator.start(); + secret = secretRotator.secrets; + } + if (!secret) throw new Error('missing cookie signing secret'); + + app.use(cookieParser(secret, {})); sessionStore = new SessionStore(settings.cookie.sessionRefreshInterval); exports.sessionMiddleware = expressSession({ propagateTouch: true, rolling: true, - secret: settings.sessionKey, + secret, store: sessionStore, resave: false, saveUninitialized: false, @@ -188,7 +202,7 @@ exports.restartServer = async () => { // cleaner :) name: 'express_sid', cookie: { - maxAge: settings.cookie.sessionLifetime || null, // Convert 0 to null. + maxAge: sessionLifetime || null, // Convert 0 to null. sameSite: settings.cookie.sameSite, // The automatic express-session mechanism for determining if the application is being served diff --git a/src/node/security/SecretRotator.js b/src/node/security/SecretRotator.js new file mode 100644 index 000000000..b02a195cd --- /dev/null +++ b/src/node/security/SecretRotator.js @@ -0,0 +1,251 @@ +'use strict'; + +const {Buffer} = require('buffer'); +const crypto = require('./crypto'); +const db = require('../db/DB'); +const log4js = require('log4js'); + +class Kdf { + async generateParams() { throw new Error('not implemented'); } + async derive(params, info) { throw new Error('not implemented'); } +} + +class LegacyStaticSecret extends Kdf { + async derive(params, info) { return params; } +} + +class Hkdf extends Kdf { + constructor(digest, keyLen) { + super(); + this._digest = digest; + this._keyLen = keyLen; + } + + async generateParams() { + const [secret, salt] = (await Promise.all([ + crypto.randomBytes(this._keyLen), + crypto.randomBytes(this._keyLen), + ])).map((b) => b.toString('hex')); + return {digest: this._digest, keyLen: this._keyLen, salt, secret}; + } + + async derive(p, info) { + return Buffer.from( + await crypto.hkdf(p.digest, p.secret, p.salt, info, p.keyLen)).toString('hex'); + } +} + +// Key derivation algorithms. Do not modify entries in this array, except: +// * It is OK to replace an unused algorithm with `null` after any entries in the database +// using the algorithm have been deleted. +// * It is OK to append a new algorithm to the end. +// If the entries are modified in any other way then key derivation might fail or produce invalid +// results due to broken compatibility with existing database records. +const algorithms = [ + new LegacyStaticSecret(), + new Hkdf('sha256', 32), +]; +const defaultAlgId = algorithms.length - 1; + +// In JavaScript, the % operator is remainder, not modulus. +const mod = (a, n) => ((a % n) + n) % n; +const intervalStart = (t, interval) => t - mod(t, interval); + +/** + * Maintains an array of secrets across one or more Etherpad instances sharing the same database, + * periodically rotating in a new secret and removing the oldest secret. + * + * The secrets are generated using a key derivation function (KDF) with input keying material coming + * from a long-lived secret stored in the database (generated if missing). + */ +class SecretRotator { + /** + * @param {string} dbPrefix - Database key prefix to use for tracking secret metadata. + * @param {number} interval - How often to rotate in a new secret. + * @param {number} lifetime - How long after the end of an interval before the secret is no longer + * useful. + * @param {string} [legacyStaticSecret] - Optional secret to facilitate migration to secret + * rotation. If the oldest known secret starts after `lifetime` ago, this secret will cover + * the time period starting `lifetime` ago and ending at the start of that secret. + */ + constructor(dbPrefix, interval, lifetime, legacyStaticSecret = null) { + /** + * The secrets. The first secret in this array is the one that should be used to generate new + * MACs. All of the secrets in this array should be used when attempting to authenticate an + * existing MAC. The contents of this array will be updated every `interval` milliseconds, but + * the Array object itself will never be replaced with a new Array object. + * + * @type {string[]} + * @public + */ + this.secrets = []; + Object.defineProperty(this, 'secrets', {writable: false}); // Defend against bugs. + + if (/[*:%]/.test(dbPrefix)) throw new Error(`dbPrefix contains an invalid char: ${dbPrefix}`); + this._dbPrefix = dbPrefix; + this._interval = interval; + this._legacyStaticSecret = legacyStaticSecret; + this._lifetime = lifetime; + this._logger = log4js.getLogger(`secret-rotation ${dbPrefix}`); + this._logger.debug(`new secret rotator (interval ${interval}, lifetime: ${lifetime})`); + this._updateTimeout = null; + + // Indirections to facilitate testing. + this._t = {now: Date.now.bind(Date), setTimeout, clearTimeout, algorithms}; + } + + async _publish(params, id = null) { + // Params are published to the db with a randomly generated key to avoid race conditions with + // other instances. + if (id == null) id = `${this._dbPrefix}:${(await crypto.randomBytes(32)).toString('hex')}`; + await db.set(id, params); + return id; + } + + async start() { + this._logger.debug('starting secret rotation'); + if (this._updateTimeout != null) return; // Already started. + await this._update(); + } + + stop() { + this._logger.debug('stopping secret rotation'); + this._t.clearTimeout(this._updateTimeout); + this._updateTimeout = null; + } + + async _deriveSecrets(p, now) { + this._logger.debug('deriving secrets from', p); + if (!p.interval) return [await algorithms[p.algId].derive(p.algParams, null)]; + const t0 = intervalStart(now, p.interval); + // Start of the first interval covered by these params. To accommodate clock skew, p.interval is + // subtracted. If we did not do this, then the following could happen: + // 1. Instance (A) starts up and publishes params starting at the current interval. + // 2. Instance (B) starts up with a clock that is in the previous interval. + // 3. Instance (B) reads the params published by instance (A) and sees that there's no + // coverage of what it thinks is the current interval. + // 4. Instance (B) generates and publishes new params that covers what it thinks is the + // current interval. + // 5. Instance (B) starts generating MACs from a secret derived from the new params. + // 6. Instance (A) fails to validate the MACs generated by instance (B) until it re-reads + // the published params, which might take as long as interval. + // An alternative approach is to backdate p.start by p.interval when creating new params, but + // this could affect the end time of legacy secrets. + const tA = intervalStart(p.start - p.interval, p.interval); + const tZ = intervalStart(p.end - 1, p.interval); + this._logger.debug('now:', now, 't0:', t0, 'tA:', tA, 'tZ:', tZ); + // Starts of intervals to derive keys for. + const tNs = []; + // Whether the derived secret for the interval starting at tN is still relevant. If there was no + // clock skew, a derived secret is relevant until p.lifetime has elapsed since the end of the + // interval. To accommodate clock skew, this end time is extended by p.interval. + const expired = (tN) => now >= tN + (2 * p.interval) + p.lifetime; + // Walk from t0 back until either the start of coverage or the derived secret is expired. t0 + // must always be the first entry in case p is the current params. (The first derived secret is + // used for generating MACs, so the secret derived for t0 must be before the secrets derived for + // other times.) + for (let tN = Math.min(t0, tZ); tN >= tA && !expired(tN); tN -= p.interval) tNs.push(tN); + // Include a future derived secret to accommodate clock skew. + if (t0 + p.interval <= tZ) tNs.push(t0 + p.interval); + this._logger.debug('deriving secrets for intervals with start times:', tNs); + return await Promise.all( + tNs.map(async (tN) => await algorithms[p.algId].derive(p.algParams, `${tN}`))); + } + + async _update() { + const now = this._t.now(); + const t0 = intervalStart(now, this._interval); + let next = t0 + this._interval; // When this._update() should be called again. + let legacyEnd = now; + // TODO: This is racy. If two instances start up at the same time and there are no existing + // matching publications, each will generate and publish their own paramters. In practice this + // is unlikely to happen, and if it does it can be fixed by restarting both Etherpad instances. + const dbKeys = await db.findKeys(`${this._dbPrefix}:*`, null); + let currentParams = null; + let currentId = null; + const dbWrites = []; + const allParams = []; + const legacyParams = []; + await Promise.all(dbKeys.map(async (dbKey) => { + const p = await db.get(dbKey); + if (p.algId === 0 && p.algParams === this._legacyStaticSecret) legacyParams.push(p); + if (p.start < legacyEnd) legacyEnd = p.start; + // Check if the params have expired. Params are still useful if a MAC generated by a secret + // derived from the params is still valid, which can be true up to p.end + p.lifetime if + // there was no clock skew. The p.interval factor is added to accommodate clock skew. + // p.interval is null for legacy secrets, so fall back to this._interval. + if (now >= p.end + p.lifetime + (p.interval || this._interval)) { + // This initial keying material (or legacy secret) is expired. + dbWrites.push(db.remove(dbKey)); + dbWrites[dbWrites.length - 1].catch(() => {}); // Prevent unhandled Promise rejections. + return; + } + const t1 = p.interval && intervalStart(now, p.interval) + p.interval; // Start of next intrvl. + const tA = intervalStart(p.start, p.interval); // Start of interval containing p.start. + if (p.interval) next = Math.min(next, t1); + // Determine if these params can be used to generate the current (active) secret. Note that + // p.start is allowed to be in the next interval in case there is clock skew. + if (p.interval && p.interval === this._interval && p.lifetime === this._lifetime && + tA <= t1 && p.end > now && (currentParams == null || p.start > currentParams.start)) { + if (currentParams) allParams.push(currentParams); + currentParams = p; + currentId = dbKey; + } else { + allParams.push(p); + } + })); + if (this._legacyStaticSecret && now < legacyEnd + this._lifetime + this._interval && + !legacyParams.find((p) => p.end + p.lifetime >= legacyEnd + this._lifetime)) { + const d = new Date(legacyEnd).toJSON(); + this._logger.debug(`adding legacy static secret for ${d} with lifetime ${this._lifetime}`); + const p = { + algId: 0, + algParams: this._legacyStaticSecret, + // The start time is equal to the end time so that this legacy secret does not affect the + // end times of any legacy secrets published by other instances. + start: legacyEnd, + end: legacyEnd, + interval: null, + lifetime: this._lifetime, + }; + allParams.push(p); + dbWrites.push(this._publish(p)); + dbWrites[dbWrites.length - 1].catch(() => {}); // Prevent unhandled Promise rejections. + } + if (currentParams == null) { + currentParams = { + algId: defaultAlgId, + algParams: await algorithms[defaultAlgId].generateParams(), + start: now, + end: now, // Extended below. + interval: this._interval, + lifetime: this._lifetime, + }; + } + // Advance currentParams's expiration time to the end of the next interval if needed. (The next + // interval is used so that the parameters never expire under normal circumstances.) This must + // be done before deriving any secrets from currentParams so that a secret for the next interval + // can be included (in case there is clock skew). + currentParams.end = Math.max(currentParams.end, t0 + (2 * this._interval)); + dbWrites.push(this._publish(currentParams, currentId)); + dbWrites[dbWrites.length - 1].catch(() => {}); // Prevent unhandled Promise rejections. + // The secrets derived from currentParams MUST be the first secrets. + const secrets = await this._deriveSecrets(currentParams, now); + await Promise.all( + allParams.map(async (p) => secrets.push(...await this._deriveSecrets(p, now)))); + // Update this.secrets all at once to avoid race conditions. + this.secrets.length = 0; + this.secrets.push(...secrets); + this._logger.debug('active secrets:', this.secrets); + // Wait for db writes to finish after updating this.secrets so that the new secrets become + // active as soon as possible. + await Promise.all(dbWrites); + // Use an async function so that test code can tell when it's done publishing the new secrets. + // The standard setTimeout() function ignores the callback's return value, but some of the tests + // await the returned Promise. + this._updateTimeout = + this._t.setTimeout(async () => await this._update(), next - this._t.now()); + } +} + +module.exports = SecretRotator; diff --git a/src/node/security/crypto.js b/src/node/security/crypto.js new file mode 100644 index 000000000..ebe918509 --- /dev/null +++ b/src/node/security/crypto.js @@ -0,0 +1,15 @@ +'use strict'; + +const crypto = require('crypto'); +const util = require('util'); + + +/** + * Promisified version of Node.js's crypto.hkdf. + */ +exports.hkdf = util.promisify(crypto.hkdf); + +/** + * Promisified version of Node.js's crypto.randomBytes + */ +exports.randomBytes = util.promisify(crypto.randomBytes); \ No newline at end of file diff --git a/src/node/utils/Settings.js b/src/node/utils/Settings.js index 9cf723eb6..512bc6f4c 100644 --- a/src/node/utils/Settings.js +++ b/src/node/utils/Settings.js @@ -297,9 +297,9 @@ exports.indentationOnNewLine = true; exports.logconfig = defaultLogConfig(); /* - * Session Key, do not sure this. + * Deprecated cookie signing key. */ -exports.sessionKey = false; +exports.sessionKey = null; /* * Trust Proxy, whether or not trust the x-forwarded-for header. @@ -310,6 +310,7 @@ exports.trustProxy = false; * Settings controlling the session cookie issued by Etherpad. */ exports.cookie = { + keyRotationInterval: 1 * 24 * 60 * 60 * 1000, /* * Value of the SameSite cookie property. "Lax" is recommended unless * Etherpad will be embedded in an iframe from another site, in which case @@ -805,12 +806,14 @@ exports.reloadSettings = () => { }); } + const sessionkeyFilename = absolutePaths.makeAbsolute(argv.sessionkey || './SESSIONKEY.txt'); if (!exports.sessionKey) { - const sessionkeyFilename = absolutePaths.makeAbsolute(argv.sessionkey || './SESSIONKEY.txt'); try { exports.sessionKey = fs.readFileSync(sessionkeyFilename, 'utf8'); logger.info(`Session key loaded from: ${sessionkeyFilename}`); - } catch (e) { + } catch (err) { /* ignored */ } + const keyRotationEnabled = exports.cookie.keyRotationInterval && exports.cookie.sessionLifetime; + if (!exports.sessionKey && !keyRotationEnabled) { logger.info( `Session key file "${sessionkeyFilename}" not found. Creating with random contents.`); exports.sessionKey = randomString(32); @@ -822,6 +825,10 @@ exports.reloadSettings = () => { 'If you are seeing this error after restarting using the Admin User ' + 'Interface then you can ignore this message.'); } + if (exports.sessionKey) { + logger.warn(`The sessionKey setting and ${sessionkeyFilename} file are deprecated; ` + + 'use automatic key rotation instead (see the cookie.keyRotationInterval setting).'); + } if (exports.dbType === 'dirty') { const dirtyWarning = 'DirtyDB is used. This is not recommended for production.'; diff --git a/src/package-lock.json b/src/package-lock.json index f92fa4914..a5bcd3537 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -2230,15 +2230,6 @@ "jake": "^10.8.5" } }, - "elasticsearch8": { - "version": "npm:@elastic/elasticsearch@8.8.1", - "resolved": "https://registry.npmjs.org/@elastic/elasticsearch/-/elasticsearch-8.8.1.tgz", - "integrity": "sha512-ibArPKHEmak3jao7xts2gROATiwPQo9aOrWWdix5mJcX1gnjm/UeJBVO901ROmaxFVPKxVnjC9Op3gJYkqagjg==", - "requires": { - "@elastic/transport": "^8.3.2", - "tslib": "^2.4.0" - } - }, "emoji-regex": { "version": "8.0.0", "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-8.0.0.tgz", @@ -3274,18 +3265,6 @@ "parseurl": "~1.3.3", "safe-buffer": "5.2.1", "uid-safe": "~2.1.5" - }, - "dependencies": { - "depd": { - "version": "2.0.0", - "resolved": "https://registry.npmjs.org/depd/-/depd-2.0.0.tgz", - "integrity": "sha512-g7nH6P6dyDioJogAAGprGpCtVImJhpPk/roCzdb3fIh61/s/nPsfR6onyMwkCAR/OlC3yBC0lESvUoQEAssIrw==" - }, - "safe-buffer": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.2.1.tgz", - "integrity": "sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==" - } } }, "extend": { @@ -10525,6 +10504,15 @@ "esutils": "^2.0.2" } }, + "elasticsearch8": { + "version": "npm:@elastic/elasticsearch@8.8.1", + "resolved": "https://registry.npmjs.org/@elastic/elasticsearch/-/elasticsearch-8.8.1.tgz", + "integrity": "sha512-ibArPKHEmak3jao7xts2gROATiwPQo9aOrWWdix5mJcX1gnjm/UeJBVO901ROmaxFVPKxVnjC9Op3gJYkqagjg==", + "requires": { + "@elastic/transport": "^8.3.2", + "tslib": "^2.4.0" + } + }, "es-abstract": { "version": "1.21.2", "resolved": "https://registry.npmjs.org/es-abstract/-/es-abstract-1.21.2.tgz", diff --git a/src/tests/backend/specs/SecretRotator.js b/src/tests/backend/specs/SecretRotator.js new file mode 100644 index 000000000..1831ef71f --- /dev/null +++ b/src/tests/backend/specs/SecretRotator.js @@ -0,0 +1,555 @@ +'use strict'; + +const SecretRotator = require('../../../node/security/SecretRotator'); +const assert = require('assert').strict; +const common = require('../common'); +const crypto = require('../../../node/security/crypto'); +const db = require('../../../node/db/DB'); + +const logger = common.logger; + +// Greatest common divisor. +const gcd = (...args) => ( + args.length === 1 ? args[0] + : args.length === 2 ? ((args[1]) ? gcd(args[1], args[0] % args[1]) : Math.abs(args[0])) + : gcd(args[0], gcd(...args.slice(1)))); +// Least common multiple. +const lcm = (...args) => ( + args.length === 1 ? args[0] + : args.length === 2 ? Math.abs(args[0] * args[1]) / gcd(...args) + : lcm(args[0], lcm(...args.slice(1)))); + +class FakeClock { + constructor() { + logger.debug('new fake clock'); + this._now = 0; + this._nextId = 1; + this._idle = Promise.resolve(); + this.timeouts = new Map(); + } + + _next() { return Math.min(...[...this.timeouts.values()].map((x) => x.when)); } + async setNow(t) { + logger.debug(`setting fake time to ${t}`); + assert(t >= this._now); + assert(t < Infinity); + let n; + while ((n = this._next()) <= t) { + this._now = Math.max(this._now, Math.min(n, t)); + logger.debug(`fake time set to ${this._now}; firing timeouts...`); + await this._fire(); + } + this._now = t; + logger.debug(`fake time set to ${this._now}`); + } + async advance(t) { await this.setNow(this._now + t); } + async advanceToNext() { + const n = this._next(); + if (n < this._now) await this._fire(); + else if (n < Infinity) await this.setNow(n); + } + async _fire() { + // This method MUST NOT execute any of the setTimeout callbacks synchronously, otherwise + // fc.setTimeout(fn, 0) would execute fn before fc.setTimeout() returns. Fortunately, the + // ECMAScript standard guarantees that a function passed to Promise.prototype.then() will run + // asynchronously. + this._idle = this._idle.then(() => Promise.all( + [...this.timeouts.values()] + .filter(({when}) => when <= this._now) + .sort((a, b) => a.when - b.when) + .map(async ({id, fn}) => { + this.clearTimeout(id); + // With the standard setTimeout(), the callback function's return value is ignored. + // Here we await the return value so that test code can block until timeout work is + // done. + await fn(); + }))); + await this._idle; + } + + get now() { return this._now; } + setTimeout(fn, wait = 0) { + const when = this._now + wait; + const id = this._nextId++; + this.timeouts.set(id, {id, fn, when}); + this._fire(); + return id; + } + clearTimeout(id) { this.timeouts.delete(id); } +} + +// In JavaScript, the % operator is remainder, not modulus. +const mod = (a, n) => ((a % n) + n) % n; + +describe(__filename, function () { + let dbPrefix; + let sr; + let interval = 1e3; + const lifetime = 1e4; + const intervalStart = (t) => t - mod(t, interval); + const hkdf = async (secret, salt, tN) => Buffer.from( + await crypto.hkdf('sha256', secret, salt, `${tN}`, 32)).toString('hex'); + + const newRotator = (s = null) => new SecretRotator(dbPrefix, interval, lifetime, s); + + const setFakeClock = (sr, fc = null) => { + if (fc == null) fc = new FakeClock(); + sr._t = { + now: () => fc.now, + setTimeout: fc.setTimeout.bind(fc), + clearTimeout: fc.clearTimeout.bind(fc), + }; + return fc; + }; + + before(async function () { + await common.init(); + }); + + beforeEach(async function () { + dbPrefix = `test-SecretRotator-${common.randomString()}`; + interval = 1e3; + }); + + afterEach(async function () { + if (sr != null) sr.stop(); + sr = null; + await Promise.all( + (await db.findKeys(`${dbPrefix}:*`, null)).map(async (dbKey) => await db.remove(dbKey))); + }); + + describe('constructor', function () { + it('creates empty secrets array', async function () { + sr = newRotator(); + assert.deepEqual(sr.secrets, []); + }); + + for (const invalidChar of '*:%') { + it(`rejects database prefixes containing ${invalidChar}`, async function () { + dbPrefix += invalidChar; + assert.throws(newRotator, /invalid char/); + }); + } + }); + + describe('start', function () { + it('does not replace secrets array', async function () { + sr = newRotator(); + setFakeClock(sr); + const {secrets} = sr; + await sr.start(); + assert.equal(sr.secrets, secrets); + }); + + it('derives secrets', async function () { + sr = newRotator(); + setFakeClock(sr); + await sr.start(); + assert.equal(sr.secrets.length, 3); // Current (active), previous, and next. + for (const s of sr.secrets) { + assert.equal(typeof s, 'string'); + assert(s); + } + assert.equal(new Set(sr.secrets).size, sr.secrets.length); // The secrets should all differ. + }); + + it('publishes params', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const dbKeys = await db.findKeys(`${dbPrefix}:*`, null); + assert.equal(dbKeys.length, 1); + const [id] = dbKeys; + assert(id.startsWith(`${dbPrefix}:`)); + assert.notEqual(id.slice(dbPrefix.length + 1), ''); + const p = await db.get(id); + const {secret, salt} = p.algParams; + assert.deepEqual(p, { + algId: 1, + algParams: { + digest: 'sha256', + keyLen: 32, + salt, + secret, + }, + start: fc.now, + end: fc.now + (2 * interval), + interval, + lifetime, + }); + assert.equal(typeof salt, 'string'); + assert.match(salt, /^[0-9a-f]{64}$/); + assert.equal(typeof secret, 'string'); + assert.match(secret, /^[0-9a-f]{64}$/); + assert.deepEqual(sr.secrets, await Promise.all( + [0, -interval, interval].map(async (tN) => await hkdf(secret, salt, tN)))); + }); + + it('reuses matching publication if unexpired', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const {secrets} = sr; + const dbKeys = await db.findKeys(`${dbPrefix}:*`, null); + sr.stop(); + sr = newRotator(); + setFakeClock(sr, fc); + await sr.start(); + assert.deepEqual(sr.secrets, secrets); + assert.deepEqual(await db.findKeys(`${dbPrefix}:*`, null), dbKeys); + }); + + it('deletes expired publications', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const [oldId] = await db.findKeys(`${dbPrefix}:*`, null); + assert(oldId != null); + sr.stop(); + const p = await db.get(oldId); + await fc.setNow(p.end + p.lifetime + p.interval); + sr = newRotator(); + setFakeClock(sr, fc); + await sr.start(); + const ids = await db.findKeys(`${dbPrefix}:*`, null); + assert.equal(ids.length, 1); + const [newId] = ids; + assert.notEqual(newId, oldId); + }); + + it('keeps expired publications until interval past expiration', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const [, , future] = sr.secrets; + sr.stop(); + const [origId] = await db.findKeys(`${dbPrefix}:*`, null); + const p = await db.get(origId); + await fc.advance(p.end + p.lifetime + p.interval - 1); + sr = newRotator(); + setFakeClock(sr, fc); + await sr.start(); + assert(sr.secrets.slice(1).includes(future)); + // It should have created a new publication, not extended the life of the old publication. + assert.equal((await db.findKeys(`${dbPrefix}:*`, null)).length, 2); + assert.deepEqual(await db.get(origId), p); + }); + + it('idempotent', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + assert.equal(fc.timeouts.size, 1); + const secrets = [...sr.secrets]; + const dbKeys = await db.findKeys(`${dbPrefix}:*`, null); + await sr.start(); + assert.equal(fc.timeouts.size, 1); + assert.deepEqual(sr.secrets, secrets); + assert.deepEqual(await db.findKeys(`${dbPrefix}:*`, null), dbKeys); + }); + + describe(`schedules update at next interval (= ${interval})`, function () { + const testCases = [ + {now: 0, want: interval}, + {now: 1, want: interval}, + {now: interval - 1, want: interval}, + {now: interval, want: 2 * interval}, + {now: interval + 1, want: 2 * interval}, + ]; + for (const {now, want} of testCases) { + it(`${now} -> ${want}`, async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await fc.setNow(now); + await sr.start(); + assert.equal(fc.timeouts.size, 1); + const [{when}] = fc.timeouts.values(); + assert.equal(when, want); + }); + } + + it('multiple active params with different intervals', async function () { + const intervals = [400, 600, 1000]; + const lcmi = lcm(...intervals); + const wants = new Set(); + for (const i of intervals) for (let t = i; t <= lcmi; t += i) wants.add(t); + const fcs = new FakeClock(); + const srs = intervals.map((i) => { + interval = i; + const sr = newRotator(); + setFakeClock(sr, fcs); + return sr; + }); + try { + for (const sr of srs) await sr.start(); // Don't use Promise.all() otherwise they race. + interval = intervals[intervals.length - 1]; + sr = newRotator(); + const fc = setFakeClock(sr); // Independent clock to test a single instance's behavior. + await sr.start(); + for (const want of [...wants].sort((a, b) => a - b)) { + logger.debug(`next timeout should be at ${want}`); + await fc.advanceToNext(); + await fcs.setNow(fc.now); // Keep all of the publications alive. + assert.equal(fc.now, want); + } + } finally { + for (const sr of srs) sr.stop(); + } + }); + }); + }); + + describe('stop', function () { + it('clears timeout', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + assert.notEqual(fc.timeouts.size, 0); + sr.stop(); + assert.equal(fc.timeouts.size, 0); + }); + + it('safe to call multiple times', async function () { + sr = newRotator(); + setFakeClock(sr); + await sr.start(); + sr.stop(); + sr.stop(); + }); + }); + + describe('legacy secret', function () { + it('ends at now if there are no previously published secrets', async function () { + sr = newRotator('legacy'); + const fc = setFakeClock(sr); + // Use a time that isn't a multiple of interval in case there is a modular arithmetic bug that + // would otherwise go undetected. + await fc.setNow(1); + assert(mod(fc.now, interval) !== 0); + await sr.start(); + assert.equal(sr.secrets.length, 4); // 1 for the legacy secret, 3 for past, current, future + assert(sr.secrets.slice(1).includes('legacy')); // Should not be the current secret. + const ids = await db.findKeys(`${dbPrefix}:*`, null); + const params = (await Promise.all(ids.map(async (id) => await db.get(id)))) + .sort((a, b) => a.algId - b.algId); + assert.deepEqual(params, [ + { + algId: 0, + algParams: 'legacy', + // The start time must equal the end time so that legacy secrets do not affect the end + // times of legacy secrets published by other instances. + start: fc.now, + end: fc.now, + lifetime, + interval: null, + }, + { + algId: 1, + algParams: params[1].algParams, + start: fc.now, + end: intervalStart(fc.now) + (2 * interval), + interval, + lifetime, + }, + ]); + }); + + it('ends at the start of the oldest previously published secret', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await fc.setNow(1); + assert(mod(fc.now, interval) !== 0); + const wantTime = fc.now; + await sr.start(); + assert.equal(sr.secrets.length, 3); + const [s1, s0, s2] = sr.secrets; // s1=current, s0=previous, s2=next + sr.stop(); + // Use a time that is not a multiple of interval off of epoch or wantTime just in case there + // is a modular arithmetic bug that would otherwise go undetected. + await fc.advance(interval + 1); + assert(mod(fc.now, interval) !== 0); + assert(mod(fc.now - wantTime, interval) !== 0); + sr = newRotator('legacy'); + setFakeClock(sr, fc); + await sr.start(); + assert.equal(sr.secrets.length, 5); // s0 through s3 and the legacy secret. + assert.deepEqual(sr.secrets, [s2, s1, s0, sr.secrets[3], 'legacy']); + const ids = await db.findKeys(`${dbPrefix}:*`, null); + const params = (await Promise.all(ids.map(async (id) => await db.get(id)))) + .sort((a, b) => a.algId - b.algId); + assert.deepEqual(params, [ + { + algId: 0, + algParams: 'legacy', + start: wantTime, + end: wantTime, + interval: null, + lifetime, + }, + { + algId: 1, + algParams: params[1].algParams, + start: wantTime, + end: intervalStart(fc.now) + (2 * interval), + interval, + lifetime, + }, + ]); + }); + + it('multiple instances with different legacy secrets', async function () { + sr = newRotator('legacy1'); + const fc = setFakeClock(sr); + await sr.start(); + sr.stop(); + sr = newRotator('legacy2'); + setFakeClock(sr, fc); + await sr.start(); + assert(sr.secrets.slice(1).includes('legacy1')); + assert(sr.secrets.slice(1).includes('legacy2')); + }); + + it('multiple instances with the same legacy secret', async function () { + sr = newRotator('legacy'); + const fc = setFakeClock(sr); + await sr.start(); + sr.stop(); + sr = newRotator('legacy'); + setFakeClock(sr, fc); + await sr.start(); + assert.deepEqual(sr.secrets, [...new Set(sr.secrets)]); + // There shouldn't be multiple publications for the same legacy secret. + assert.equal((await db.findKeys(`${dbPrefix}:*`, null)).length, 2); + }); + + it('legacy secret is included for interval after expiration', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + sr.stop(); + await fc.advance(lifetime + interval - 1); + sr = newRotator('legacy'); + setFakeClock(sr, fc); + await sr.start(); + assert(sr.secrets.slice(1).includes('legacy')); + }); + + it('legacy secret is not included if the oldest secret is old enough', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + sr.stop(); + await fc.advance(lifetime + interval); + sr = newRotator('legacy'); + setFakeClock(sr, fc); + await sr.start(); + assert(!sr.secrets.includes('legacy')); + }); + + it('dead secrets still affect legacy secret end time', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const secrets = new Set(sr.secrets); + sr.stop(); + await fc.advance(lifetime + (3 * interval)); + sr = newRotator('legacy'); + setFakeClock(sr, fc); + await sr.start(); + assert(!sr.secrets.includes('legacy')); + assert(!sr.secrets.some((s) => secrets.has(s))); + }); + }); + + describe('rotation', function () { + it('no rotation before start of interval', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + assert.equal(fc.now, 0); + await sr.start(); + const secrets = [...sr.secrets]; + await fc.advance(interval - 1); + assert.deepEqual(sr.secrets, secrets); + }); + + it('does not replace secrets array', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const [current] = sr.secrets; + const secrets = sr.secrets; + await fc.advance(interval); + assert.notEqual(sr.secrets[0], current); + assert.equal(sr.secrets, secrets); + }); + + it('future secret becomes current, new future is generated', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const secrets = new Set(sr.secrets); + assert.equal(secrets.size, 3); + const [s1, s0, s2] = sr.secrets; + await fc.advance(interval); + assert.deepEqual(sr.secrets, [s2, s1, s0, sr.secrets[3]]); + assert(!secrets.has(sr.secrets[3])); + }); + + it('expired publications are deleted', async function () { + const origInterval = interval; + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + sr.stop(); + ++interval; // Force new params so that the old params can expire. + sr = newRotator(); + setFakeClock(sr, fc); + await sr.start(); + assert.equal((await db.findKeys(`${dbPrefix}:*`, null)).length, 2); + await fc.advance(lifetime + (3 * origInterval)); + assert.equal((await db.findKeys(`${dbPrefix}:*`, null)).length, 1); + }); + + it('old secrets are eventually removed', async function () { + sr = newRotator(); + const fc = setFakeClock(sr); + await sr.start(); + const [, s0] = sr.secrets; + await fc.advance(lifetime + interval - 1); + assert(sr.secrets.slice(1).includes(s0)); + await fc.advance(1); + assert(!sr.secrets.includes(s0)); + }); + }); + + describe('clock skew', function () { + it('out of sync works if in adjacent interval', async function () { + const srs = [newRotator(), newRotator()]; + const fcs = srs.map((sr) => setFakeClock(sr)); + for (const sr of srs) await sr.start(); // Don't use Promise.all() otherwise they race. + assert.deepEqual(srs[0].secrets, srs[1].secrets); + // Advance fcs[0] to the end of the interval after fcs[1]. + await fcs[0].advance((2 * interval) - 1); + assert(srs[0].secrets.includes(srs[1].secrets[0])); + assert(srs[1].secrets.includes(srs[0].secrets[0])); + // Advance both by an interval. + await Promise.all([fcs[1].advance(interval), fcs[0].advance(interval)]); + assert(srs[0].secrets.includes(srs[1].secrets[0])); + assert(srs[1].secrets.includes(srs[0].secrets[0])); + // Advance fcs[1] to the end of the interval after fcs[0]. + await Promise.all([fcs[1].advance((3 * interval) - 1), fcs[0].advance(1)]); + assert(srs[0].secrets.includes(srs[1].secrets[0])); + assert(srs[1].secrets.includes(srs[0].secrets[0])); + }); + + it('start up out of sync', async function () { + const srs = [newRotator(), newRotator()]; + const fcs = srs.map((sr) => setFakeClock(sr)); + await fcs[0].advance((2 * interval) - 1); + await srs[0].start(); // Must start before srs[1] so that srs[1] starts in srs[0]'s past. + await srs[1].start(); + assert(srs[0].secrets.includes(srs[1].secrets[0])); + assert(srs[1].secrets.includes(srs[0].secrets[0])); + }); + }); +}); diff --git a/src/tests/backend/specs/crypto.js b/src/tests/backend/specs/crypto.js new file mode 100644 index 000000000..cde096f01 --- /dev/null +++ b/src/tests/backend/specs/crypto.js @@ -0,0 +1,11 @@ +'use strict'; + +const assert = require('assert').strict; +const {Buffer} = require('buffer'); +const crypto = require('../../../node/security/crypto'); +const nodeCrypto = require('crypto'); +const util = require('util'); + +const nodeHkdf = nodeCrypto.hkdf ? util.promisify(nodeCrypto.hkdf) : null; + +const ab2hex = (ab) => Buffer.from(ab).toString('hex');