fix(dashmate): deliver renewed certificates to the running gateway

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',

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

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'),

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