From a7f00013280416ce889d841e675526b8cb96a0ee Mon Sep 17 00:00:00 2001
From: Lucas Savva <lucas@m1cr0man.com>
Date: Sat, 27 Nov 2021 00:03:35 +0000
Subject: [PATCH] nixos/acme: Check for revoked certificates

Closes #129838

It is possible for the CA to revoke a cert that has not yet
expired. We must run lego to validate this before expiration,
but we must still ignore failures on unexpired certs to retain
compatibility with #85794

Also changed domainHash logic such that a renewal will only
be attempted at all if domains are unchanged, and do a full
run otherwises. Resolves #147540 but will be partially
reverted when go-acme/lego#1532 is resolved + available.
---
 nixos/modules/security/acme.nix | 25 +++++++++++--------------
 nixos/tests/acme.nix            | 22 +++++++++++++++++++++-
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/nixos/modules/security/acme.nix b/nixos/modules/security/acme.nix
index be4762da8d13..9242a12fd35f 100644
--- a/nixos/modules/security/acme.nix
+++ b/nixos/modules/security/acme.nix
@@ -156,6 +156,7 @@ let
       ${toString data.ocspMustStaple} ${data.keyType}
     '';
     certDir = mkHash hashData;
+    # TODO remove domainHash usage entirely. Waiting on go-acme/lego#1532
     domainHash = mkHash "${concatStringsSep " " extraDomains} ${data.domain}";
     accountHash = (mkAccountHash acmeServer data);
     accountDir = accountDirRoot + accountHash;
@@ -372,22 +373,19 @@ let
 
         echo '${domainHash}' > domainhash.txt
 
-        # Check if we can renew
-        if [ -e 'certificates/${keyName}.key' -a -e 'certificates/${keyName}.crt' -a -n "$(ls -1 accounts)" ]; then
+        # Check if we can renew.
+        # We can only renew if the list of domains has not changed.
+        if cmp -s domainhash.txt certificates/domainhash.txt && [ -e 'certificates/${keyName}.key' -a -e 'certificates/${keyName}.crt' -a -n "$(ls -1 accounts)" ]; then
 
-          # When domains are updated, there's no need to do a full
-          # Lego run, but it's likely renew won't work if days is too low.
-          if [ -e certificates/domainhash.txt ] && cmp -s domainhash.txt certificates/domainhash.txt; then
+          # Even if a cert is not expired, it may be revoked by the CA.
+          # Try to renew, and silently fail if the cert is not expired.
+          # Avoids #85794 and resolves #129838
+          if ! lego ${renewOpts} --days ${toString cfg.validMinDays}; then
             if is_expiration_skippable out/full.pem; then
-              echo 1>&2 "nixos-acme: skipping renewal because expiration isn't within the coming ${toString cfg.validMinDays} days"
+              echo 1>&2 "nixos-acme: Ignoring failed renewal because expiration isn't within the coming ${toString cfg.validMinDays} days"
             else
-              echo 1>&2 "nixos-acme: renewing now, because certificate expires within the configured ${toString cfg.validMinDays} days"
-              lego ${renewOpts} --days ${toString cfg.validMinDays}
+              exit 3
             fi
-          else
-            echo 1>&2 "certificate domain(s) have changed; will renew now"
-            # Any number > 90 works, but this one is over 9000 ;-)
-            lego ${renewOpts} --days 9001
           fi
 
         # Otherwise do a full run
@@ -406,8 +404,7 @@ let
         chown 'acme:${data.group}' certificates/*
 
         # Copy all certs to the "real" certs directory
-        CERT='certificates/${keyName}.crt'
-        if [ -e "$CERT" ] && ! cmp -s "$CERT" out/fullchain.pem; then
+        if ! cmp -s 'certificates/${keyName}.crt' out/fullchain.pem; then
           touch out/renewed
           echo Installing new certificate
           cp -vp 'certificates/${keyName}.crt' out/fullchain.pem
diff --git a/nixos/tests/acme.nix b/nixos/tests/acme.nix
index 72b7bb8a396a..80b85502d4e8 100644
--- a/nixos/tests/acme.nix
+++ b/nixos/tests/acme.nix
@@ -113,11 +113,19 @@ in import ./make-test-python.nix ({ lib, ... }: {
 
       # Now adding an alias to ensure that the certs are updated
       specialisation.nginx-aliases.configuration = { pkgs, ... }: {
-        services.nginx.virtualHosts."a.example.test" = {
+        services.nginx.virtualHosts."a.example.test" = (vhostBase pkgs) // {
           serverAliases = [ "b.example.test" ];
         };
       };
 
+      # Must be run after nginx-aliases
+      specialisation.remove-extra-domain.configuration = { pkgs, ... } : {
+        # This also validates that useACMEHost doesn't unexpectedly add the domain.
+        services.nginx.virtualHosts."b.example.test" = (vhostBase pkgs) // {
+          useACMEHost = "a.example.test";
+        };
+      };
+
       # Test OCSP Stapling
       specialisation.ocsp-stapling.configuration = { pkgs, ... }: {
         security.acme.certs."a.example.test" = {
@@ -408,6 +416,18 @@ in import ./make-test-python.nix ({ lib, ... }: {
           check_connection(client, "a.example.test")
           check_connection(client, "b.example.test")
 
+      with subtest("Can remove extra domains from a cert"):
+          switch_to(webserver, "remove-extra-domain")
+          webserver.wait_for_unit("acme-finished-a.example.test.target")
+          webserver.wait_for_unit("nginx.service")
+          check_connection(client, "a.example.test")
+          rc, _ = client.execute(
+              "openssl s_client -CAfile /tmp/ca.crt -connect b.example.test:443"
+              " </dev/null 2>/dev/null | openssl x509 -noout -text"
+              " | grep DNS: | grep b.example.test"
+          )
+          assert rc > 0, "Removed extraDomainName was not removed from the cert"
+
       with subtest("Can request certificates for vhost + aliases (apache-httpd)"):
           try:
               switch_to(webserver, "httpd-aliases")