fix(ssl-migration): make the checks for migrating ssl more robust

closes #552
- skip ssl migration in a couple more cases
- fix tests

extensions/nginx/migrations.js

@@ -14,11 +14,26 @@ function migrateSSL(ctx, migrateTask) {
     const confFile = path.join(ctx.instance.dir, 'system', 'files', `${parsedUrl.hostname}-ssl.conf`);
     const rootPath = path.resolve(ctx.instance.dir, 'system', 'nginx-root');
 
+    // Case to skip 1: SSL config for nginx does not exist
     if (!fs.existsSync(confFile)) {
         return migrateTask.skip('SSL config has not been set up for this domain');
     }
 
     const originalAcmePath = path.join(os.homedir(), '.acme.sh');
+    const originalCertFolder = path.join(originalAcmePath, parsedUrl.hostname);
+
+    // Case to skip 2: SSL cert doesn't exist in the original location for this domain
+    if (!fs.existsSync(originalCertFolder)) {
+        return migrateTask.skip('SSL cert does not exist for this domain');
+    }
+
+    const confFileContents = fs.readFileSync(confFile, {encoding: 'utf8'});
+    const certCheck = new RegExp(`ssl_certificate ${originalCertFolder}/fullchain.cer;`)
+
+    // Case to skip 3: SSL conf does not contain a cert config using the old LE cert
+    if (!certCheck.test(confFileContents)) {
+        return migrateTask.skip('LetsEncrypt SSL cert is not being used for this domain');
+    }
 
     // 1. parse ~/.acme.sh/account.conf to get the email
     const accountConf = fs.readFileSync(path.join(originalAcmePath, 'account.conf'), {encoding: 'utf8'});
@@ -45,8 +60,10 @@ function migrateSSL(ctx, migrateTask) {
             return replace({
                 files: confFile,
                 from: [
-                    /ssl_certificate .*/,
-                    /ssl_certificate_key .*/
+                    // Ensure here that we ONLY replace instances of the LetsEncrypt cert in the file,
+                    // that way we don't overwrite the cert config of other certs.
+                    certCheck,
+                    new RegExp(`ssl_certificate_key ${originalCertFolder}/${parsedUrl.hostname}.key;`)
                 ],
                 to: [
                     `ssl_certificate ${path.join(acmeFolder, 'fullchain.cer')};`,

extensions/nginx/test/fixtures/old-ssl-with-le.txt

@@ -0,0 +1,25 @@
+server {
+    listen 443 ssl http2;
+    listen [::]:443 ssl http2;
+
+    server_name ghost.org;
+    root /var/www/ghost/system/nginx-root;
+
+    ssl_certificate /home/ghost/.acme.sh/ghost.org/fullchain.cer;
+    ssl_certificate_key /home/ghost/.acme.sh/ghost.org/ghost.org.key;
+    include /var/www/ghost/system/files/ssl-params.conf;
+
+    location / {
+        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+        proxy_set_header X-Forwarded-Proto $scheme;
+        proxy_set_header X-Real-IP $remote_addr;
+        proxy_set_header Host $http_host;
+        proxy_pass http://127.0.0.1:2368;
+    }
+
+    location ~ /.well-known {
+        allow all;
+    }
+
+    client_max_body_size 50m;
+}

extensions/nginx/test/fixtures/ssl-without-le.txt

@@ -0,0 +1,25 @@
+server {
+    listen 443 ssl http2;
+    listen [::]:443 ssl http2;
+
+    server_name ghost.org;
+    root /var/www/ghost/system/nginx-root;
+
+    ssl_certificate /etc/ssl/comodo/ghost.org.cer;
+    ssl_certificate_key /etc/ssl/comodo/ghost.org.key;
+    include /var/www/ghost/system/files/ssl-params.conf;
+
+    location / {
+        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+        proxy_set_header X-Forwarded-Proto $scheme;
+        proxy_set_header X-Real-IP $remote_addr;
+        proxy_set_header Host $http_host;
+        proxy_pass http://127.0.0.1:2368;
+    }
+
+    location ~ /.well-known {
+        allow all;
+    }
+
+    client_max_body_size 50m;
+}

extensions/nginx/test/migrations-spec.js

@@ -1,5 +1,6 @@
 'use strict';
-
+const fs = require('fs');
+const path = require('path');
 const expect = require('chai').expect;
 const sinon = require('sinon');
 const proxyquire = require('proxyquire').noCallThru();
@@ -17,6 +18,9 @@ const context =  {
     }
 };
 
+const sslWithoutLe = fs.readFileSync(path.join(__dirname, './fixtures/ssl-without-le.txt'), {encoding: 'utf8'});
+const oldSslWithLe = fs.readFileSync(path.join(__dirname, './fixtures/old-ssl-with-le.txt'), {encoding: 'utf8'});
+
 describe('Unit: Extensions > Nginx > Migrations', function () {
     describe('migrateSSL', function () {
         it('skips if ssl is not set up', function () {
@@ -34,12 +38,61 @@ describe('Unit: Extensions > Nginx > Migrations', function () {
             expect(skipStub.calledOnce).to.be.true;
         });
 
+        it('skips if cert has not been generated using the old method', function () {
+            const skip = sinon.stub();
+            const existsSync = sinon.stub();
+
+            existsSync.withArgs('/var/www/ghost/system/files/ghost.org-ssl.conf').returns(true);
+            existsSync.withArgs('/home/ghost/.acme.sh/ghost.org').returns(false);
+
+            const migrate = proxyquire(modulePath, {
+                'fs-extra': {existsSync: existsSync},
+                os: {homedir: () => '/home/ghost'}
+            });
+
+            migrate.migrateSSL(context, {skip: skip});
+
+            expect(existsSync.calledTwice).to.be.true;
+            expect(existsSync.calledWithExactly('/var/www/ghost/system/files/ghost.org-ssl.conf')).to.be.true;
+            expect(existsSync.calledWithExactly('/home/ghost/.acme.sh/ghost.org')).to.be.true;
+            expect(skip.calledOnce).to.be.true;
+        });
+
+        it('skips if ssl conf isn\'t using an LE cert', function () {
+            const skip = sinon.stub();
+            const existsSync = sinon.stub();
+            const readFileSync = sinon.stub();
+
+            const confFile = '/var/www/ghost/system/files/ghost.org-ssl.conf';
+
+            existsSync.withArgs(confFile).returns(true);
+            existsSync.withArgs('/home/ghost/.acme.sh/ghost.org').returns(true);
+            readFileSync.withArgs(confFile).returns(sslWithoutLe);
+
+            const migrate = proxyquire(modulePath, {
+                'fs-extra': {existsSync: existsSync, readFileSync: readFileSync},
+                os: {homedir: () => '/home/ghost'}
+            });
+
+            migrate.migrateSSL(context, {skip: skip});
+
+            expect(existsSync.calledTwice).to.be.true;
+            expect(existsSync.calledWithExactly('/var/www/ghost/system/files/ghost.org-ssl.conf')).to.be.true;
+            expect(existsSync.calledWithExactly('/home/ghost/.acme.sh/ghost.org')).to.be.true;
+            expect(readFileSync.calledOnce).to.be.true;
+            expect(readFileSync.calledWithExactly(confFile, {encoding: 'utf8'})).to.be.true;
+            expect(skip.calledOnce).to.be.true;
+        });
+
         it('throws an error if it can\'t parse the letsencrypt account email', function () {
-            const existsStub = sinon.stub().returns(true);
-            const rfsStub = sinon.stub().returns('');
+            const existsSync = sinon.stub().returns(true);
+            const readFileSync = sinon.stub();
+
+            readFileSync.onFirstCall().returns(oldSslWithLe);
+            readFileSync.onSecondCall().returns('');
 
             const migrate = proxyquire(modulePath, {
-                'fs-extra': {existsSync: existsStub, readFileSync: rfsStub},
+                'fs-extra': {existsSync: existsSync, readFileSync: readFileSync},
                 os: {homedir: () => '/home/ghost'}
             });
 
@@ -50,13 +103,18 @@ describe('Unit: Extensions > Nginx > Migrations', function () {
                 expect(e).to.be.an.instanceof(cli.errors.SystemError);
                 expect(e.message).to.equal('Unable to parse letsencrypt account email');
 
-                expect(rfsStub.calledWithExactly('/home/ghost/.acme.sh/account.conf'));
+                expect(readFileSync.calledTwice).to.be.true;
+                expect(readFileSync.calledWithExactly('/home/ghost/.acme.sh/account.conf', {encoding: 'utf8'})).to.be.true;
             }
         });
 
         it('runs tasks correctly', function () {
-            const existsStub = sinon.stub().returns(true);
-            const rfsStub = sinon.stub().returns('ACCOUNT_EMAIL=\'[email protected]\'\n');
+            const existsSync = sinon.stub().returns(true);
+            const readFileSync = sinon.stub();
+
+            readFileSync.onFirstCall().returns(oldSslWithLe);
+            readFileSync.onSecondCall().returns('ACCOUNT_EMAIL=\'[email protected]\'\n');
+
             const restartStub = sinon.stub().resolves();
             const replaceStub = sinon.stub().resolves();
 
@@ -70,7 +128,7 @@ describe('Unit: Extensions > Nginx > Migrations', function () {
             };
 
             const migrate = proxyquire(modulePath, {
-                'fs-extra': {existsSync: existsStub, readFileSync: rfsStub},
+                'fs-extra': {existsSync: existsSync, readFileSync: readFileSync},
                 'replace-in-file': replaceStub,
                 './acme': acme,
                 os: {homedir: () => '/home/ghost'}
@@ -80,8 +138,8 @@ describe('Unit: Extensions > Nginx > Migrations', function () {
 
             fn(context);
 
-            expect(existsStub.calledOnce).to.be.true;
-            expect(rfsStub.calledOnce).to.be.true;
+            expect(existsSync.calledTwice).to.be.true;
+            expect(readFileSync.calledTwice).to.be.true;
             expect(ui.listr.calledOnce).to.be.true;
 
             const tasks = ui.listr.getCall(0).args[0];
@@ -112,7 +170,7 @@ describe('Unit: Extensions > Nginx > Migrations', function () {
                 return tasks[4].task();
             }).then(() => {
                 expect(acme.remove.calledOnce).to.be.true;
-                expect(acme.remove.calledWithExactly('ghost.org', ui, '/home/ghost/.acme.sh'));
+                expect(acme.remove.calledWithExactly('ghost.org', ui, '/home/ghost/.acme.sh')).to.be.true;
             });
         });
     });