From d7021884afff7771892c2af93ca3563d19aea36d Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 7 Feb 2022 19:32:47 -0500 Subject: [PATCH] express-session: Enable key rotation --- CHANGELOG.md | 5 +++++ doc/docker.md | 0 settings.json.docker | 19 +++++++++++++++++++ settings.json.template | 18 ++++++++++++++++++ src/node/hooks/express.js | 20 +++++++++++++++++--- src/node/utils/Settings.js | 15 +++++++++++---- 6 files changed, 70 insertions(+), 7 deletions(-) create mode 100644 doc/docker.md 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.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..b64ab5f3d 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('../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 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.';