From fc298744f76ffff588cef44f5eb2ced75a2aed44 Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 20 Aug 2026 00:18:59 +0700 Subject: [PATCH 1/3] fix(dashmate): deliver renewed certificates to the running gateway The gateway certificate and key are bind-mounted into the container as individual files. A file bind mount follows the inode, not the path, so a certificate installed by writing a replacement and renaming it over the old file never reaches a running gateway: the container keeps reading the inode it mounted at startup, and the mount itself holds that inode alive. Envoy would go on serving the previous certificate until it expired, with both files on disk looking current and nothing in the renewal path recreating the container. Install the pair in place again so the inode the container holds is the one that receives the renewal. The replaced install guarded against a torn write during renewal. Concurrent writers - the scheduled renewal and a manual obtain - are already serialized by the configuration lock, which spans the whole obtain on both paths, so what remained was an unclean shutdown landing inside a sub-millisecond write that happens once every few days. Against that, the delivery failure was silent, affected every node, and needed an operator to recreate a container to clear. The private key permission repair is kept, with the owner write bit restored around the write so a key hardened to 0400 can still be replaced. Also reload the gateway after `dashmate ssl obtain`. Only the scheduled renewal ever signalled Envoy, so an operator could run the command dashmate points them to, see it succeed, and find nothing changed on the wire. The reload is skipped when the certificate was already current or the gateway is not running. Tests, red before the change and green after: should install a renewal into the files the gateway already has mounted AssertionError: expected 422747267 to equal 422747265 should reload the gateway after obtaining a certificate AssertionError: expected stub to have been called exactly once with arguments { get: [Function: functionStub] }, 'gateway', 'kill -SIGHUP 1' The inode assertion pins the property the bind mount depends on, so a future atomic-install change cannot silently reintroduce this. Note on scope: the inode behaviour of the write is demonstrated by the test above, but that a container's file bind mount then goes stale on Linux was reasoned from the mount semantics, not measured against a running gateway. The specs covering the removed temp-file rollback and cleanup are dropped with the code they exercised. Co-Authored-By: Claude Opus 5 --- packages/dashmate/src/commands/ssl/obtain.js | 20 +++++ .../listr/tasks/ssl/saveCertificateTask.js | 74 +++++---------- .../test/unit/commands/ssl/obtain.spec.js | 76 ++++++++++++++++ .../test/unit/ssl/saveCertificateTask.spec.js | 90 +++++-------------- 4 files changed, 139 insertions(+), 121 deletions(-) diff --git a/packages/dashmate/src/commands/ssl/obtain.js b/packages/dashmate/src/commands/ssl/obtain.js index 7fa2f37a309..00be466a521 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,25 @@ 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. + title: 'Reload gateway', + skip: async (ctx) => { + if (!ctx.certificateSaved) { + return 'Certificate is already up to date'; + } + + 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..48b4a9a810f 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,69 +29,38 @@ 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); - } - } - throw e; - } finally { - cleanupTempFiles(); - unsubscribe(); + // 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); } + fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: keyMode }); + fs.chmodSync(keyFile, keyMode); + + // A running gateway only picks up what was written here once it is + // told to reload, so let callers see that the pair changed. + ctx.certificateSaved = true; + 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..0ec1518ca13 100644 --- a/packages/dashmate/test/unit/commands/ssl/obtain.spec.js +++ b/packages/dashmate/test/unit/commands/ssl/obtain.spec.js @@ -2,6 +2,82 @@ import { Listr } from 'listr2'; import ObtainCommand from '../../../../src/commands/ssl/obtain.js'; describe('SSL obtain command', () => { + /** + * @param {Object} sinon + * @param {boolean} certificateSaved + * @return {Object} + */ + function obtainDependencies(sinon, certificateSaved) { + return { + config: { + get: sinon.stub().returns('letsencrypt'), + }, + dockerCompose: { + isServiceRunning: sinon.stub().resolves(true), + execCommand: sinon.stub().resolves(), + }, + obtainLetsEncryptCertificateTask: sinon.stub().callsFake(() => new Listr([ + { + task: (ctx) => { + ctx.certificateSaved = certificateSaved; + }, + }, + ])), + }; + } + + /** + * @param {Object} dependencies + * @return {Promise} + */ + function runObtain({ config, dockerCompose, obtainLetsEncryptCertificateTask }) { + return new ObtainCommand().runWithDependencies( + {}, + { + verbose: false, + 'no-retry': true, + 'expiration-days': undefined, + force: false, + provider: 'letsencrypt', + }, + config, + () => new Listr([]), + obtainLetsEncryptCertificateTask, + { 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, true); + + await runObtain(dependencies); + + expect(dependencies.dockerCompose.execCommand) + .to.have.been.calledOnceWith(dependencies.config, 'gateway', 'kill -SIGHUP 1'); + }); + + it('should not reload the gateway when the certificate was already up to date', async function it() { + const dependencies = obtainDependencies(this.sinon, undefined); + + await runObtain(dependencies); + + expect(dependencies.dockerCompose.execCommand).to.have.not.been.called(); + }); + + it('should not reload a gateway that is not running', async function it() { + const dependencies = obtainDependencies(this.sinon, true); + 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..b2491b4861f 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(); @@ -91,71 +112,4 @@ describe('saveCertificateTaskFactory', () => { 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'); - } - - return originalRenameSync(source, destination); - }); - - await expect(savePair()).to.be.rejectedWith('key replace 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(); - }); - - it('should sweep stale certificate temp files before writing', 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'); - - 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(); - }); }); From 3705cbafca36ed3be4dc50dda07015815be35bcf Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 20 Aug 2026 01:16:38 +0700 Subject: [PATCH 2/3] fix(dashmate): reload the gateway after every obtain, not only after a write The reload was gated on a marker set by saveCertificateTask, which two paths never reach. ZeroSSL does not use saveCertificateTask at all - it writes the bind-mounted pair itself - so a successful `dashmate ssl obtain --provider=zerossl` skipped the reload entirely and left Envoy on its previously loaded certificate. The marker is also transient, so it could not recover a failed reload. Both providers skip the write once the pair is on disk, so re-running the command after a reload failure set no marker, sent no signal, and reported success. That is the same "command succeeds, wire unchanged" failure the reload was added to fix, reached from the other side. Nothing on disk reveals which certificate a running Envoy holds, so reload whenever the gateway is up. This costs a hot restart on an obtain that changed nothing, and in exchange the command becomes idempotent: running it again is how an operator retries a reload that failed. The marker is dropped from saveCertificateTask, where it now has no reader. Tests, red before the change and green after: should reload the gateway after obtaining a certificate should reload the gateway after obtaining a ZeroSSL certificate should reload the gateway when the certificate was already installed AssertionError: expected stub to have been called exactly once with arguments { get: [Function: functionStub] }, 'gateway', 'kill -SIGHUP 1' The ZeroSSL case pins the reported defect; the third pins recovery, and replaces an earlier test that asserted the opposite. Co-Authored-By: Claude Opus 5 --- packages/dashmate/src/commands/ssl/obtain.js | 12 ++-- .../listr/tasks/ssl/saveCertificateTask.js | 4 -- .../test/unit/commands/ssl/obtain.spec.js | 61 +++++++++++++------ 3 files changed, 49 insertions(+), 28 deletions(-) diff --git a/packages/dashmate/src/commands/ssl/obtain.js b/packages/dashmate/src/commands/ssl/obtain.js index 00be466a521..8bd6f7e389c 100644 --- a/packages/dashmate/src/commands/ssl/obtain.js +++ b/packages/dashmate/src/commands/ssl/obtain.js @@ -94,12 +94,14 @@ Certificate will be renewed if it is about to expire (see 'expiration-days' flag // 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 (ctx) => { - if (!ctx.certificateSaved) { - return 'Certificate is already up to date'; - } - + skip: async () => { if (!await dockerCompose.isServiceRunning(config, 'gateway')) { return 'Gateway is not running'; } diff --git a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js index 48b4a9a810f..f1a8064eaee 100644 --- a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js +++ b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js @@ -57,10 +57,6 @@ export default function saveCertificateTaskFactory(homeDir) { fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: keyMode }); fs.chmodSync(keyFile, keyMode); - // A running gateway only picks up what was written here once it is - // told to reload, so let callers see that the pair changed. - ctx.certificateSaved = true; - 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 0ec1518ca13..5e80a1967f4 100644 --- a/packages/dashmate/test/unit/commands/ssl/obtain.spec.js +++ b/packages/dashmate/test/unit/commands/ssl/obtain.spec.js @@ -4,25 +4,20 @@ import ObtainCommand from '../../../../src/commands/ssl/obtain.js'; describe('SSL obtain command', () => { /** * @param {Object} sinon - * @param {boolean} certificateSaved + * @param {string} [provider] * @return {Object} */ - function obtainDependencies(sinon, certificateSaved) { + function obtainDependencies(sinon, provider = 'letsencrypt') { return { + provider, config: { - get: sinon.stub().returns('letsencrypt'), + get: sinon.stub().returns(provider), }, dockerCompose: { isServiceRunning: sinon.stub().resolves(true), execCommand: sinon.stub().resolves(), }, - obtainLetsEncryptCertificateTask: sinon.stub().callsFake(() => new Listr([ - { - task: (ctx) => { - ctx.certificateSaved = certificateSaved; - }, - }, - ])), + obtainTask: sinon.stub().callsFake(() => new Listr([{ task: () => {} }])), }; } @@ -30,7 +25,11 @@ describe('SSL obtain command', () => { * @param {Object} dependencies * @return {Promise} */ - function runObtain({ config, dockerCompose, obtainLetsEncryptCertificateTask }) { + function runObtain({ + provider, config, dockerCompose, obtainTask, + }) { + const noop = () => new Listr([]); + return new ObtainCommand().runWithDependencies( {}, { @@ -38,11 +37,11 @@ describe('SSL obtain command', () => { 'no-retry': true, 'expiration-days': undefined, force: false, - provider: 'letsencrypt', + provider, }, config, - () => new Listr([]), - obtainLetsEncryptCertificateTask, + provider === 'zerossl' ? obtainTask : noop, + provider === 'letsencrypt' ? obtainTask : noop, { write: () => {} }, {}, dockerCompose, @@ -53,7 +52,7 @@ describe('SSL obtain command', () => { // 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, true); + const dependencies = obtainDependencies(this.sinon); await runObtain(dependencies); @@ -61,16 +60,40 @@ describe('SSL obtain command', () => { .to.have.been.calledOnceWith(dependencies.config, 'gateway', 'kill -SIGHUP 1'); }); - it('should not reload the gateway when the certificate was already up to date', async function it() { - const dependencies = obtainDependencies(this.sinon, undefined); + // 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.not.been.called(); + 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, true); + const dependencies = obtainDependencies(this.sinon); dependencies.dockerCompose.isServiceRunning.resolves(false); await runObtain(dependencies); From 63c47f86352a04cf373237e25c28ed47ffa806fa Mon Sep 17 00:00:00 2001 From: Ivan Shumkov Date: Thu, 20 Aug 2026 01:23:16 +0700 Subject: [PATCH 3/3] fix(dashmate): keep a hardened private key mode when its write fails Writing the key in place needs the owner write bit, which an owner who hardened the key to 0400 has removed, so the mode is loosened for the write and restored after it. A write that threw skipped the restore and left the key readable and writable by its owner rather than read-only. Restore in a finally block so the chosen mode is put back on both paths. Test, red before the change and green after: should keep a hardened private key mode when the write fails AssertionError: expected 384 to equal 256 Co-Authored-By: Claude Opus 5 --- .../listr/tasks/ssl/saveCertificateTask.js | 11 ++++++++-- .../test/unit/ssl/saveCertificateTask.spec.js | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js index f1a8064eaee..31e93a14a31 100644 --- a/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js +++ b/packages/dashmate/src/listr/tasks/ssl/saveCertificateTask.js @@ -54,8 +54,15 @@ export default function saveCertificateTaskFactory(homeDir) { fs.chmodSync(keyFile, 0o600); } - fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: keyMode }); - fs.chmodSync(keyFile, keyMode); + try { + fs.writeFileSync(keyFile, ctx.privateKeyFile, { encoding: 'utf8', mode: keyMode }); + } finally { + // 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/ssl/saveCertificateTask.spec.js b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js index b2491b4861f..3f17e995cc6 100644 --- a/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js +++ b/packages/dashmate/test/unit/ssl/saveCertificateTask.spec.js @@ -102,6 +102,28 @@ describe('saveCertificateTaskFactory', () => { expect(mode(keyPath)).to.equal(0o600); }); + // 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); + + 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 originalWriteFileSync(filePath, data, options); + }); + + await expect(savePair()).to.be.rejectedWith('key write failed'); + + expect(mode(keyPath)).to.equal(0o400); + }); + it('should keep a private key mode stricter than Dashmate would choose', async () => { fs.mkdirSync(certificatesDir, { recursive: true }); fs.writeFileSync(certificatePath, 'old-certificate');