From 1b6cfd9796788a3c5b8e8f27b49271f4a423c9a7 Mon Sep 17 00:00:00 2001
From: Lucas Savva <lucas@m1cr0man.com>
Date: Thu, 3 Sep 2020 15:31:06 +0100
Subject: nixos/acme: Fix race condition, dont be smart with keys

Attempting to reuse keys on a basis different to the cert (AKA,
storing the key in a directory with a hashed name different to
the cert it is associated with) was ineffective since when
"lego run" is used it will ALWAYS generate a new key. This causes
issues when you revert changes since your "reused" key will not
be the one associated with the old cert. As such, I tore out the
whole keyDir implementation.

As for the race condition, checking the mtime of the cert file
was not sufficient to detect changes. In testing, selfsigned
and full certs could be generated/installed within 1 second of
each other. cmp is now used instead.

Also, I removed the nginx/httpd reload waiters in favour of
simple retry logic for the curl-based tests
---
 nixos/modules/security/acme.nix | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

(limited to 'nixos/modules')

diff --git a/nixos/modules/security/acme.nix b/nixos/modules/security/acme.nix
index 91b7dd0c989fe..51392f6ce885c 100644
--- a/nixos/modules/security/acme.nix
+++ b/nixos/modules/security/acme.nix
@@ -106,7 +106,6 @@ let
     mkHash = with builtins; val: substring 0 20 (hashString "sha256" val);
     certDir = mkHash hashData;
     othersHash = mkHash "${toString acmeServer} ${data.keyType}";
-    keyDir = "key-" + othersHash;
     accountDir = "/var/lib/acme/.lego/accounts/" + othersHash;
 
     protocolOpts = if useDns then (
@@ -215,7 +214,7 @@ let
       # https://github.com/NixOS/nixpkgs/pull/81371#issuecomment-605526099
       wantedBy = optionals (!config.boot.isContainer) [ "multi-user.target" ];
 
-      path = with pkgs; [ lego coreutils ];
+      path = with pkgs; [ lego coreutils diffutils ];
 
       serviceConfig = commonServiceConfig // {
         Group = data.group;
@@ -223,14 +222,13 @@ let
         # AccountDir dir will be created by tmpfiles to ensure correct permissions
         # And to avoid deletion during systemctl clean
         # acme/.lego/${cert} is listed so that it is deleted during systemctl clean
-        StateDirectory = "acme/${cert} acme/.lego/${cert} acme/.lego/${cert}/${certDir} acme/.lego/${cert}/${keyDir}";
+        StateDirectory = "acme/${cert} acme/.lego/${cert} acme/.lego/${cert}/${certDir}";
 
         # Needs to be space separated, but can't use a multiline string because that'll include newlines
         BindPaths =
           "${accountDir}:/tmp/accounts " +
           "/var/lib/acme/${cert}:/tmp/out " +
-          "/var/lib/acme/.lego/${cert}/${certDir}:/tmp/certificates " +
-          "/var/lib/acme/.lego/${cert}/${keyDir}:/tmp/keys";
+          "/var/lib/acme/.lego/${cert}/${certDir}:/tmp/certificates ";
 
         # Only try loading the credentialsFile if the dns challenge is enabled
         EnvironmentFile = mkIf useDns data.credentialsFile;
@@ -240,9 +238,6 @@ let
       script = ''
         set -euo pipefail
 
-        # Safely copy keyDir contents into certificates (it might be empty).
-        cp -af keys/. certificates/
-
         # Check if we can renew
         if [ -e 'certificates/${keyName}.key' -a -e 'certificates/${keyName}.crt' ]; then
           lego ${renewOpts}
@@ -258,17 +253,15 @@ let
         # Group might change between runs, re-apply it
         chown 'acme:${data.group}' certificates/*
 
-        # Copy the key to keyDir
-        cp -pf 'certificates/${keyName}.key' 'keys/'
-
         # Copy all certs to the "real" certs directory
         CERT='certificates/${keyName}.crt'
         CERT_CHANGED=no
-        if [ -e "$CERT" -a "$CERT" -nt out/fullchain.pem ]; then
+        if [ -e "$CERT" ] && ! cmp -s "$CERT" out/fullchain.pem; then
           CERT_CHANGED=yes
-          cp -p 'certificates/${keyName}.crt' out/fullchain.pem
-          cp -p 'certificates/${keyName}.key' out/key.pem
-          cp -p 'certificates/${keyName}.issuer.crt' out/chain.pem
+          echo Installing new certificate
+          cp -vp 'certificates/${keyName}.crt' out/fullchain.pem
+          cp -vp 'certificates/${keyName}.key' out/key.pem
+          cp -vp 'certificates/${keyName}.issuer.crt' out/chain.pem
           ln -sf fullchain.pem out/cert.pem
           cat out/key.pem out/fullchain.pem > out/full.pem
         fi
-- 
cgit 1.4.1