express-session: Implement and enable key rotation (#5362) by @rhansen

* SecretRotator: New class to coordinate key rotation

* express-session: Enable key rotation

* Added new entry in docker.adoc

* Move to own package.Removed fallback as Node 16 is now lowest node version.

* Updated package-lock.json

---------

Co-authored-by: SamTV12345 <40429738+samtv12345@users.noreply.github.com>

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

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;

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);

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.';