1 files changed, 53 insertions, 52 deletions

diff --git a/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix b/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix
index 8a015bb3556..39bcb14e5af 100644
--- a/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix
+++ b/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix
@@ -6,23 +6,23 @@ let
   cfg = config.services.nginx;
   certs = config.security.acme.certs;
   vhostsConfigs = mapAttrsToList (vhostName: vhostConfig: vhostConfig) virtualHosts;
-  acmeEnabledVhosts = filter (vhostConfig: vhostConfig.enableACME && vhostConfig.useACMEHost == null) vhostsConfigs;
+  acmeEnabledVhosts = filter (vhostConfig: vhostConfig.enableACME || vhostConfig.useACMEHost != null) vhostsConfigs;
+  dependentCertNames = unique (map (hostOpts: hostOpts.certName) acmeEnabledVhosts);
   virtualHosts = mapAttrs (vhostName: vhostConfig:
     let
       serverName = if vhostConfig.serverName != null
         then vhostConfig.serverName
         else vhostName;
+      certName = if vhostConfig.useACMEHost != null
+        then vhostConfig.useACMEHost
+        else serverName;
     in
     vhostConfig // {
-      inherit serverName;
-    } // (optionalAttrs vhostConfig.enableACME {
-      sslCertificate = "${certs.${serverName}.directory}/fullchain.pem";
-      sslCertificateKey = "${certs.${serverName}.directory}/key.pem";
-      sslTrustedCertificate = "${certs.${serverName}.directory}/full.pem";
-    }) // (optionalAttrs (vhostConfig.useACMEHost != null) {
-      sslCertificate = "${certs.${vhostConfig.useACMEHost}.directory}/fullchain.pem";
-      sslCertificateKey = "${certs.${vhostConfig.useACMEHost}.directory}/key.pem";
-      sslTrustedCertificate = "${certs.${vhostConfig.useACMEHost}.directory}/fullchain.pem";
+      inherit serverName certName;
+    } // (optionalAttrs (vhostConfig.enableACME || vhostConfig.useACMEHost != null) {
+      sslCertificate = "${certs.${certName}.directory}/fullchain.pem";
+      sslCertificateKey = "${certs.${certName}.directory}/key.pem";
+      sslTrustedCertificate = "${certs.${certName}.directory}/chain.pem";
     })
   ) cfg.virtualHosts;
   enableIPv6 = config.networking.enableIPv6;
@@ -463,14 +463,6 @@ in
         '';
       };
 
-      enableSandbox = mkOption {
-        default = false;
-        type = types.bool;
-        description = ''
-          Starting Nginx web server with additional sandbox/hardening options.
-        '';
-      };
-
       user = mkOption {
         type = types.str;
         default = "nginx";
@@ -691,8 +683,12 @@ in
     systemd.services.nginx = {
       description = "Nginx Web Server";
       wantedBy = [ "multi-user.target" ];
-      wants = concatLists (map (vhostConfig: ["acme-${vhostConfig.serverName}.service" "acme-selfsigned-${vhostConfig.serverName}.service"]) acmeEnabledVhosts);
-      after = [ "network.target" ] ++ map (vhostConfig: "acme-selfsigned-${vhostConfig.serverName}.service") acmeEnabledVhosts;
+      wants = concatLists (map (certName: [ "acme-finished-${certName}.target" ]) dependentCertNames);
+      after = [ "network.target" ] ++ map (certName: "acme-selfsigned-${certName}.service") dependentCertNames;
+      # Nginx needs to be started in order to be able to request certificates
+      # (it's hosting the acme challenge after all)
+      # This fixes https://github.com/NixOS/nixpkgs/issues/81842
+      before = map (certName: "acme-${certName}.service") dependentCertNames;
       stopIfChanged = false;
       preStart = ''
         ${cfg.preStart}
@@ -700,7 +696,10 @@ in
       '';
       serviceConfig = {
         ExecStart = execCommand;
-        ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
+        ExecReload = [
+          "${execCommand} -t"
+          "${pkgs.coreutils}/bin/kill -HUP $MAINPID"
+        ];
         Restart = "always";
         RestartSec = "10s";
         StartLimitInterval = "1min";
@@ -721,7 +720,6 @@ in
         CapabilityBoundingSet = [ "CAP_NET_BIND_SERVICE" "CAP_SYS_RESOURCE" ];
         # Security
         NoNewPrivileges = true;
-      } // optionalAttrs cfg.enableSandbox {
         # Sandboxing
         ProtectSystem = "strict";
         ProtectHome = mkDefault true;
@@ -746,38 +744,41 @@ in
       source = configFile;
     };
 
-    systemd.services.nginx-config-reload = mkIf cfg.enableReload {
-      wants = [ "nginx.service" ];
-      wantedBy = [ "multi-user.target" ];
-      restartTriggers = [ configFile ];
-      # commented, because can cause extra delays during activate for this config:
-      #      services.nginx.virtualHosts."_".locations."/".proxyPass = "http://blabla:3000";
-      # stopIfChanged = false;
-      serviceConfig.Type = "oneshot";
-      serviceConfig.TimeoutSec = 60;
-      script = ''
-        if /run/current-system/systemd/bin/systemctl -q is-active nginx.service ; then
-          ${execCommand} -t && \
-            /run/current-system/systemd/bin/systemctl reload nginx.service
-        fi
-      '';
-      serviceConfig.RemainAfterExit = true;
+    # postRun hooks on cert renew can't be used to restart Nginx since renewal
+    # runs as the unprivileged acme user. sslTargets are added to wantedBy + before
+    # which allows the acme-finished-$cert.target to signify the successful updating
+    # of certs end-to-end.
+    systemd.services.nginx-config-reload = let
+      sslServices = map (certName: "acme-${certName}.service") dependentCertNames;
+      sslTargets = map (certName: "acme-finished-${certName}.target") dependentCertNames;
+    in mkIf (cfg.enableReload || sslServices != []) {
+      wants = optionals (cfg.enableReload) [ "nginx.service" ];
+      wantedBy = sslServices ++ [ "multi-user.target" ];
+      # Before the finished targets, after the renew services.
+      # This service might be needed for HTTP-01 challenges, but we only want to confirm
+      # certs are updated _after_ config has been reloaded.
+      before = sslTargets;
+      after = sslServices;
+      restartTriggers = optionals (cfg.enableReload) [ configFile ];
+      # Block reloading if not all certs exist yet.
+      # Happens when config changes add new vhosts/certs.
+      unitConfig.ConditionPathExists = optionals (sslServices != []) (map (certName: certs.${certName}.directory + "/fullchain.pem") dependentCertNames);
+      serviceConfig = {
+        Type = "oneshot";
+        TimeoutSec = 60;
+        ExecCondition = "/run/current-system/systemd/bin/systemctl -q is-active nginx.service";
+        ExecStart = "/run/current-system/systemd/bin/systemctl reload nginx.service";
+      };
     };
 
-    security.acme.certs = filterAttrs (n: v: v != {}) (
-      let
-        acmePairs = map (vhostConfig: { name = vhostConfig.serverName; value = {
-            user = cfg.user;
-            group = lib.mkDefault cfg.group;
-            webroot = vhostConfig.acmeRoot;
-            extraDomains = genAttrs vhostConfig.serverAliases (alias: null);
-            postRun = ''
-              /run/current-system/systemd/bin/systemctl reload nginx
-            '';
-          }; }) acmeEnabledVhosts;
-      in
-        listToAttrs acmePairs
-    );
+    security.acme.certs = let
+      acmePairs = map (vhostConfig: nameValuePair vhostConfig.serverName {
+        group = mkDefault cfg.group;
+        webroot = vhostConfig.acmeRoot;
+        extraDomainNames = vhostConfig.serverAliases;
+      # Filter for enableACME-only vhosts. Don't want to create dud certs
+      }) (filter (vhostConfig: vhostConfig.useACMEHost == null) acmeEnabledVhosts);
+    in listToAttrs acmePairs;
 
     users.users = optionalAttrs (cfg.user == "nginx") {
       nginx = {