nixos/acme: Ensure certs are always protectedorigin/backport-121750-to-release-21.05

As per #121293, I ensured the UMask is set correctly
and removed any unnecessary chmod/chown/chgrp commands.
The test suite already partially covered permissions
checking but I added an extra check for the selfsigned
cert permissions.

(cherry picked from commit 083aba4f83b105c30a1386bdb214cb6c85e119e6)

2 files changed, 27 insertions, 13 deletions

diff --git a/nixos/modules/security/acme.nix b/nixos/modules/security/acme.nix
index c0250171109f..22bf34198a30 100644
--- a/nixos/modules/security/acme.nix
+++ b/nixos/modules/security/acme.nix
@@ -46,6 +46,7 @@ let
     serviceConfig = commonServiceConfig // {
       StateDirectory = "acme/.minica";
       BindPaths = "/var/lib/acme/.minica:/tmp/ca";
+      UMask = 0077;
     };
 
     # Working directory will be /tmp
@@ -54,8 +55,6 @@ let
         --ca-key ca/key.pem \
         --ca-cert ca/cert.pem \
         --domains selfsigned.local
-
-      chmod 600 ca/*
     '';
   };
 
@@ -196,6 +195,7 @@ let
 
       serviceConfig = commonServiceConfig // {
         Group = data.group;
+        UMask = 0027;
 
         StateDirectory = "acme/${cert}";
 
@@ -220,10 +220,12 @@ let
         cat cert.pem chain.pem > fullchain.pem
         cat key.pem fullchain.pem > full.pem
 
-        chmod 640 *
-
         # Group might change between runs, re-apply it
         chown 'acme:${data.group}' *
+
+        # Default permissions make the files unreadable by group + anon
+        # Need to be readable by group
+        chmod 640 *
       '';
     };
 
@@ -340,8 +342,6 @@ let
         fi
 
         mv domainhash.txt certificates/
-        chmod 640 certificates/*
-        chmod -R u=rwX,g=,o= accounts/*
 
         # Group might change between runs, re-apply it
         chown 'acme:${data.group}' certificates/*
@@ -357,6 +357,10 @@ let
           ln -sf fullchain.pem out/cert.pem
           cat out/key.pem out/fullchain.pem > out/full.pem
         fi
+
+        # By default group will have no access to the cert files.
+        # This chmod will fix that.
+        chmod 640 out/*
       '';
     };
   };
diff --git a/nixos/tests/acme.nix b/nixos/tests/acme.nix
index fe8c4af3ea21..6532fc4ac1d4 100644
--- a/nixos/tests/acme.nix
+++ b/nixos/tests/acme.nix
@@ -330,30 +330,38 @@ in import ./make-test-python.nix ({ lib, ... }: {
 
       with subtest("Can request certificate with HTTPS-01 challenge"):
           webserver.wait_for_unit("acme-finished-a.example.test.target")
-          check_fullchain(webserver, "a.example.test")
-          check_issuer(webserver, "a.example.test", "pebble")
-          check_connection(client, "a.example.test")
 
       with subtest("Certificates and accounts have safe + valid permissions"):
           group = "${nodes.webserver.config.security.acme.certs."a.example.test".group}"
           webserver.succeed(
-              f"test $(stat -L -c \"%a %U %G\" /var/lib/acme/a.example.test/* | tee /dev/stderr | grep '640 acme {group}' | wc -l) -eq 5"
+              f"test $(stat -L -c '%a %U %G' /var/lib/acme/a.example.test/*.pem | tee /dev/stderr | grep '640 acme {group}' | wc -l) -eq 5"
           )
           webserver.succeed(
-              f"test $(stat -L -c \"%a %U %G\" /var/lib/acme/.lego/a.example.test/**/* | tee /dev/stderr | grep '640 acme {group}' | wc -l) -eq 5"
+              f"test $(stat -L -c '%a %U %G' /var/lib/acme/.lego/a.example.test/**/a.example.test* | tee /dev/stderr | grep '600 acme {group}' | wc -l) -eq 4"
           )
           webserver.succeed(
-              f"test $(stat -L -c \"%a %U %G\" /var/lib/acme/a.example.test | tee /dev/stderr | grep '750 acme {group}' | wc -l) -eq 1"
+              f"test $(stat -L -c '%a %U %G' /var/lib/acme/a.example.test | tee /dev/stderr | grep '750 acme {group}' | wc -l) -eq 1"
           )
           webserver.succeed(
-              f"test $(find /var/lib/acme/accounts -type f -exec stat -L -c \"%a %U %G\" {{}} \\; | tee /dev/stderr | grep -v '600 acme {group}' | wc -l) -eq 0"
+              f"test $(find /var/lib/acme/accounts -type f -exec stat -L -c '%a %U %G' {{}} \\; | tee /dev/stderr | grep -v '600 acme {group}' | wc -l) -eq 0"
           )
 
+      with subtest("Certs are accepted by web server"):
+          webserver.succeed("systemctl start nginx.service")
+          check_fullchain(webserver, "a.example.test")
+          check_issuer(webserver, "a.example.test", "pebble")
+          check_connection(client, "a.example.test")
+
+      # Selfsigned certs tests happen late so we aren't fighting the system init triggering cert renewal
       with subtest("Can generate valid selfsigned certs"):
           webserver.succeed("systemctl clean acme-a.example.test.service --what=state")
           webserver.succeed("systemctl start acme-selfsigned-a.example.test.service")
           check_fullchain(webserver, "a.example.test")
           check_issuer(webserver, "a.example.test", "minica")
+          # Check selfsigned permissions
+          webserver.succeed(
+              f"test $(stat -L -c '%a %U %G' /var/lib/acme/a.example.test/*.pem | tee /dev/stderr | grep '640 acme {group}' | wc -l) -eq 5"
+          )
           # Will succeed if nginx can load the certs
           webserver.succeed("systemctl start nginx-config-reload.service")
 
@@ -376,6 +384,8 @@ in import ./make-test-python.nix ({ lib, ... }: {
           webserver.wait_for_unit("acme-finished-a.example.test.target")
           check_connection_key_bits(client, "a.example.test", "384")
           webserver.succeed("grep testing /var/lib/acme/a.example.test/test")
+          # Clean to remove the testing file (and anything else messy we did)
+          webserver.succeed("systemctl clean acme-a.example.test.service --what=state")
 
       with subtest("Correctly implements OCSP stapling"):
           switch_to(webserver, "ocsp-stapling")