diff --git a/packages/dashmate/src/commands/ssl/obtain.js b/packages/dashmate/src/commands/ssl/obtain.js index 7fa2f37a309..8bd6f7e389c 100644 --- a/packages/dashmate/src/commands/ssl/obtain.js +++ b/packages/dashmate/src/commands/ssl/obtain.js @@ -56,6 +56,7 @@ Certificate will be renewed if it is about to expire (see 'expiration-days' flag obtainLetsEncryptCertificateTask, configFileRepository, configFile, + dockerCompose, ) { const provider = providerFlag || config.get('platform.gateway.ssl.provider'); @@ -88,6 +89,27 @@ Certificate will be renewed if it is about to expire (see 'expiration-days' flag title: taskTitle, task: () => task(config, taskOptions), }, + { + // Envoy reads the certificate files once at startup, so a gateway that + // is already up keeps serving the previous certificate until it is + // told to reload. Without this the command reports success while + // nothing changes on the wire. + // + // This runs whenever the gateway is up, including when the obtain + // wrote no new files: providers install the pair by different routes, + // and nothing on disk reveals which certificate Envoy currently + // holds, so an obtain that skipped the write is also how an operator + // retries a reload that failed earlier. + title: 'Reload gateway', + skip: async () => { + if (!await dockerCompose.isServiceRunning(config, 'gateway')) { + return 'Gateway is not running'; + } + + return false; + }, + task: () => dockerCompose.execCommand(config, 'gateway', 'kill -SIGHUP 1'), + }, ], { renderer: isVerbose ? 'verbose' : 'default', diff --git a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js index 9d6b2cbbfd5..31e93a14a31 100644 --- a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js +++ b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js @@ -1,7 +1,6 @@ import { Listr } from 'listr2'; import path from 'path'; import fs from 'fs'; -import graceful from 'node-graceful'; /** * @param {HomeDir} homeDir @@ -30,67 +29,39 @@ export default function saveCertificateTaskFactory(homeDir) { const crtFile = path.join(certificatesDir, 'bundle.crt'); const keyFile = path.join(certificatesDir, 'private.key'); - fs.readdirSync(certificatesDir) - .filter((fileName) => ( - fileName.startsWith('bundle.crt.tmp-') - || fileName.startsWith('private.key.tmp-') - )) - .forEach((fileName) => { - fs.rmSync(path.join(certificatesDir, fileName), { force: true }); - }); + // Docker bind-mounts both files into the gateway container individually, + // and a file bind mount follows the inode rather than the path. Writing + // in place is what lets a renewal reach the running gateway: replacing + // either file leaves the container reading the one it mounted at + // startup, and Envoy would serve the previous certificate until it + // expires. + fs.writeFileSync(crtFile, ctx.certificateFile, 'utf8'); - const crtTempFile = `${crtFile}.tmp-${process.pid}`; - const keyTempFile = `${keyFile}.tmp-${process.pid}`; - const previousCertificate = fs.existsSync(crtFile) - ? fs.readFileSync(crtFile) - : null; - const certificateMode = fs.existsSync(crtFile) - // eslint-disable-next-line no-bitwise - ? fs.statSync(crtFile).mode & 0o777 - : 0o644; // Dashmate used to create this file at the process umask, so an // upgraded node carries a group- and world-readable private key. // Dropping those bits repairs it on the next renewal, while an owner // that hardened it further - 0400 - keeps what it chose. - const keyMode = fs.existsSync(keyFile) + const keyExists = fs.existsSync(keyFile); + const keyMode = keyExists // eslint-disable-next-line no-bitwise ? fs.statSync(keyFile).mode & 0o700 : 0o600; - let certificateReplaced = false; - const cleanupTempFiles = () => { - fs.rmSync(crtTempFile, { force: true }); - fs.rmSync(keyTempFile, { force: true }); - }; - const unsubscribe = graceful.on('exit', cleanupTempFiles); - try { - fs.writeFileSync(crtTempFile, ctx.certificateFile, { - encoding: 'utf8', - mode: certificateMode, - }); - fs.chmodSync(crtTempFile, certificateMode); - fs.writeFileSync(keyTempFile, ctx.privateKeyFile, { - encoding: 'utf8', - mode: keyMode, - }); - fs.chmodSync(keyTempFile, keyMode); - fs.renameSync(crtTempFile, crtFile); - certificateReplaced = true; - fs.renameSync(keyTempFile, keyFile); - } catch (e) { - if (certificateReplaced) { - if (previousCertificate === null) { - fs.rmSync(crtFile, { force: true }); - } else { - fs.writeFileSync(crtFile, previousCertificate, { mode: certificateMode }); - fs.chmodSync(crtFile, certificateMode); - } - } + // An owner who hardened the key to 0400 has removed the write bit that + // writing in place needs, so restore it for the write and put the + // chosen mode back straight after. + if (keyExists) { + fs.chmodSync(keyFile, 0o600); + } - throw e; + try { + fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: keyMode }); } finally { - cleanupTempFiles(); - unsubscribe(); + // Also runs when the write throws, so a failure cannot leave the key + // at the looser mode it was given to make the write possible. + if (fs.existsSync(keyFile)) { + fs.chmodSync(keyFile, keyMode); + } } config.set('platform.gateway.ssl.enabled', true); diff --git a/packages/dashmate/test/unit/commands/ssl/obtain.spec.js b/packages/dashmate/test/unit/commands/ssl/obtain.spec.js index c1a64b0edf1..5e80a1967f4 100644 --- a/packages/dashmate/test/unit/commands/ssl/obtain.spec.js +++ b/packages/dashmate/test/unit/commands/ssl/obtain.spec.js @@ -2,6 +2,105 @@ import { Listr } from 'listr2'; import ObtainCommand from '../../../../src/commands/ssl/obtain.js'; describe('SSL obtain command', () => { + /** + * @param {Object} sinon + * @param {string} [provider] + * @return {Object} + */ + function obtainDependencies(sinon, provider = 'letsencrypt') { + return { + provider, + config: { + get: sinon.stub().returns(provider), + }, + dockerCompose: { + isServiceRunning: sinon.stub().resolves(true), + execCommand: sinon.stub().resolves(), + }, + obtainTask: sinon.stub().callsFake(() => new Listr([{ task: () => {} }])), + }; + } + + /** + * @param {Object} dependencies + * @return {Promise} + */ + function runObtain({ + provider, config, dockerCompose, obtainTask, + }) { + const noop = () => new Listr([]); + + return new ObtainCommand().runWithDependencies( + {}, + { + verbose: false, + 'no-retry': true, + 'expiration-days': undefined, + force: false, + provider, + }, + config, + provider === 'zerossl' ? obtainTask : noop, + provider === 'letsencrypt' ? obtainTask : noop, + { write: () => {} }, + {}, + dockerCompose, + ); + } + + // Envoy loads the certificate once at startup. Obtaining a certificate + // without telling the gateway to reload leaves the operator with a command + // that reports success while the node keeps serving the old certificate. + it('should reload the gateway after obtaining a certificate', async function it() { + const dependencies = obtainDependencies(this.sinon); + + await runObtain(dependencies); + + expect(dependencies.dockerCompose.execCommand) + .to.have.been.calledOnceWith(dependencies.config, 'gateway', 'kill -SIGHUP 1'); + }); + + // ZeroSSL writes the certificate pair itself rather than going through + // saveCertificateTask, so the reload cannot depend on anything that task + // records. + it('should reload the gateway after obtaining a ZeroSSL certificate', async function it() { + const dependencies = obtainDependencies(this.sinon, 'zerossl'); + + await runObtain(dependencies); + + expect(dependencies.dockerCompose.execCommand) + .to.have.been.calledOnceWith(dependencies.config, 'gateway', 'kill -SIGHUP 1'); + }); + + // Nothing on disk reveals which certificate a running Envoy actually holds, + // so an obtain that writes no new files still has to reload. Otherwise an + // operator whose earlier reload failed can never recover by running the + // command again. + it('should reload the gateway when the certificate was already installed', async function it() { + const dependencies = obtainDependencies(this.sinon); + dependencies.obtainTask.callsFake(() => new Listr([ + { + title: 'Certificate is up to date', + skip: () => true, + task: () => {}, + }, + ])); + + await runObtain(dependencies); + + expect(dependencies.dockerCompose.execCommand) + .to.have.been.calledOnceWith(dependencies.config, 'gateway', 'kill -SIGHUP 1'); + }); + + it('should not reload a gateway that is not running', async function it() { + const dependencies = obtainDependencies(this.sinon); + dependencies.dockerCompose.isServiceRunning.resolves(false); + + await runObtain(dependencies); + + expect(dependencies.dockerCompose.execCommand).to.have.not.been.called(); + }); + it('should checkpoint a newly created ZeroSSL certificate before a later failure', async function it() { const config = { get: this.sinon.stub().returns('zerossl'), diff --git a/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js index a6c85ac61ab..3f17e995cc6 100644 --- a/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js +++ b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js @@ -1,6 +1,5 @@ import fs from 'fs'; import path from 'path'; -import graceful from 'node-graceful'; import HomeDir from '../../../src/config/HomeDir.js'; import getBaseConfigFactory from '../../../configs/defaults/getBaseConfigFactory.js'; import saveCertificateTaskFactory from '../../../src/listr/tasks/ssl/saveCertificateTask.js'; @@ -47,6 +46,28 @@ describe('saveCertificateTaskFactory', () => { return fs.statSync(filePath).mode & 0o777; } + // Docker bind-mounts bundle.crt and private.key into the gateway container + // as individual files, and a file bind mount follows the inode rather than + // the path. Installing a renewal by writing a replacement file and renaming + // it over the old one leaves the running container reading the file it + // mounted at startup, so Envoy keeps serving the previous certificate until + // it expires and the node goes dark with nothing on disk looking wrong. + it('should install a renewal into the files the gateway already has mounted', async () => { + fs.mkdirSync(certificatesDir, { recursive: true }); + fs.writeFileSync(certificatePath, 'old-certificate'); + fs.writeFileSync(keyPath, 'old-key'); + + const certificateInode = fs.statSync(certificatePath).ino; + const keyInode = fs.statSync(keyPath).ino; + + await savePair(); + + expect(fs.statSync(certificatePath).ino).to.equal(certificateInode); + expect(fs.statSync(keyPath).ino).to.equal(keyInode); + expect(fs.readFileSync(certificatePath, 'utf8')).to.equal('new-certificate'); + expect(fs.readFileSync(keyPath, 'utf8')).to.equal('new-key'); + }); + it('should create a private key with mode 0600', async () => { await savePair(); @@ -81,81 +102,36 @@ describe('saveCertificateTaskFactory', () => { expect(mode(keyPath)).to.equal(0o600); }); - it('should keep a private key mode stricter than Dashmate would choose', async () => { + // Writing in place needs the owner write bit, so a key hardened to 0400 is + // loosened for the write. A write that then fails must not leave it that way. + it('should keep a hardened private key mode when the write fails', async function it() { fs.mkdirSync(certificatesDir, { recursive: true }); fs.writeFileSync(certificatePath, 'old-certificate'); fs.writeFileSync(keyPath, 'old-key'); fs.chmodSync(keyPath, 0o400); - await savePair(); - - expect(mode(keyPath)).to.equal(0o400); - }); - - it('should restore the previous certificate pair and modes when saving the key fails', async function it() { - fs.mkdirSync(certificatesDir, { recursive: true }); - fs.writeFileSync(certificatePath, 'old-certificate'); - fs.writeFileSync(keyPath, 'old-key'); - fs.chmodSync(certificatePath, 0o640); - fs.chmodSync(keyPath, 0o600); - - const originalRenameSync = fs.renameSync.bind(fs); - this.sinon.stub(fs, 'renameSync').callsFake((source, destination) => { - if (destination === keyPath) { - throw new Error('key replace failed'); + const originalWriteFileSync = fs.writeFileSync.bind(fs); + this.sinon.stub(fs, 'writeFileSync').callsFake((filePath, data, options) => { + if (filePath === keyPath) { + throw new Error('key write failed'); } - return originalRenameSync(source, destination); + return originalWriteFileSync(filePath, data, options); }); - await expect(savePair()).to.be.rejectedWith('key replace failed'); + await expect(savePair()).to.be.rejectedWith('key write failed'); - expect(fs.readFileSync(certificatePath, 'utf8')).to.equal('old-certificate'); - expect(fs.readFileSync(keyPath, 'utf8')).to.equal('old-key'); - expect(mode(certificatePath)).to.equal(0o640); - expect(mode(keyPath)).to.equal(0o600); - expect(fs.readdirSync(certificatesDir).filter((name) => name.includes('.tmp-'))) - .to.be.empty(); - expect(config.get('platform.gateway.ssl.enabled')).to.be.true(); + expect(mode(keyPath)).to.equal(0o400); }); - it('should sweep stale certificate temp files before writing', async () => { + it('should keep a private key mode stricter than Dashmate would choose', async () => { fs.mkdirSync(certificatesDir, { recursive: true }); - fs.writeFileSync(path.join(certificatesDir, 'bundle.crt.tmp-stale'), 'old-certificate'); - fs.writeFileSync(path.join(certificatesDir, 'private.key.tmp-stale'), 'old-key'); + fs.writeFileSync(certificatePath, 'old-certificate'); + fs.writeFileSync(keyPath, 'old-key'); + fs.chmodSync(keyPath, 0o400); await savePair(); - expect(fs.readdirSync(certificatesDir).filter((name) => name.includes('.tmp-'))) - .to.be.empty(); - }); - - it('should remove active certificate temp files from the graceful exit handler', async function it() { - let exitHandler; - const unsubscribe = this.sinon.stub(); - this.sinon.stub(graceful, 'on').callsFake((event, handler) => { - expect(event).to.equal('exit'); - exitHandler = handler; - return unsubscribe; - }); - - const originalRenameSync = fs.renameSync.bind(fs); - this.sinon.stub(fs, 'renameSync').callsFake((source, destination) => { - if (destination === certificatePath) { - expect(exitHandler).to.be.a('function'); - expect(fs.existsSync(source)).to.be.true(); - exitHandler(); - expect(fs.existsSync(source)).to.be.false(); - throw new Error('exit cleanup observed'); - } - - return originalRenameSync(source, destination); - }); - - await expect(savePair()).to.be.rejectedWith('exit cleanup observed'); - - expect(unsubscribe).to.have.been.calledOnce(); - expect(fs.readdirSync(certificatesDir).filter((name) => name.includes('.tmp-'))) - .to.be.empty(); + expect(mode(keyPath)).to.equal(0o400); }); });