express-session: Enable key rotation

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`

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

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

src/node/hooks/express.js

@@ -1,6 +1,7 @@
 'use strict';
 
 const _ = require('underscore');
+const SecretRotator = require('../utils/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/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 = () => {
     });
   }
 
-  if (!exports.sessionKey) {
   const sessionkeyFilename = absolutePaths.makeAbsolute(argv.sessionkey || './SESSIONKEY.txt');
+  if (!exports.sessionKey) {
     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.';