Merge pull request #39526 (improve dhparams)

This introduces an option that allows us to turn off stateful generation
of Diffie-Hellman parameters, which in some way is still "stateful" as
the generated DH params file is non-deterministic.

However what we can avoid with this is to have an increased surface for
failures during system startup, because generation of the parameters is
done during build-time.

Aside from adding a NixOS VM test it also restructures the type of the
security.dhparams.params option, so that it's a submodule.

A new defaultBitSize option is also there to allow users to set a
system-wide default.

I added a release notes entry that described what has changed and also
included a few notes for module developers using this module, as the
first usage already popped up in NixOS/nixpkgs#39507.

Thanks to @Ekleog and @abbradar for reviewing.

4 files changed, 344 insertions, 81 deletions

diff --git a/nixos/doc/manual/release-notes/rl-1809.xml b/nixos/doc/manual/release-notes/rl-1809.xml
index 7136f4540502..7259be4c904c 100644
--- a/nixos/doc/manual/release-notes/rl-1809.xml
+++ b/nixos/doc/manual/release-notes/rl-1809.xml
@@ -175,6 +175,58 @@ $ nix-instantiate -E '(import <nixpkgsunstable> {}).gitFull'
      for further reference.
     </para>
    </listitem>
+   <listitem>
+    <para>
+     The module for <option>security.dhparams</option> has two new options now:
+    </para>
+
+    <variablelist>
+     <varlistentry>
+      <term><option>security.dhparams.stateless</option></term>
+      <listitem><para>
+       Puts the generated Diffie-Hellman parameters into the Nix store instead
+       of managing them in a stateful manner in
+       <filename class="directory">/var/lib/dhparams</filename>.
+      </para></listitem>
+     </varlistentry>
+     <varlistentry>
+      <term><option>security.dhparams.defaultBitSize</option></term>
+      <listitem><para>
+       The default bit size to use for the generated Diffie-Hellman parameters.
+      </para></listitem>
+     </varlistentry>
+    </variablelist>
+
+    <note><para>
+     The path to the actual generated parameter files should now be queried
+     using
+     <literal>config.security.dhparams.params.<replaceable>name</replaceable>.path</literal>
+     because it might be either in the Nix store or in a directory configured
+     by <option>security.dhparams.path</option>.
+    </para></note>
+
+    <note>
+     <title>For developers:</title>
+     <para>
+      Module implementers should not set a specific bit size in order to let
+      users configure it by themselves if they want to have a different bit
+      size than the default (2048).
+     </para>
+     <para>
+      An example usage of this would be:
+<programlisting>
+{ config, ... }:
+
+{
+  security.dhparams.params.myservice = {};
+  environment.etc."myservice.conf".text = ''
+    dhparams = ${config.security.dhparams.params.myservice.path}
+  '';
+}
+</programlisting>
+     </para>
+    </note>
+   </listitem>
   </itemizedlist>
  </section>
 </section>
diff --git a/nixos/modules/security/dhparams.nix b/nixos/modules/security/dhparams.nix
index 55c75713101d..e2b84c3e3b38 100644
--- a/nixos/modules/security/dhparams.nix
+++ b/nixos/modules/security/dhparams.nix
@@ -1,107 +1,173 @@
 { config, lib, pkgs, ... }:
 
-with lib;
 let
+  inherit (lib) mkOption types;
   cfg = config.security.dhparams;
-in
-{
+
+  bitType = types.addCheck types.int (b: b >= 16) // {
+    name = "bits";
+    description = "integer of at least 16 bits";
+  };
+
+  paramsSubmodule = { name, config, ... }: {
+    options.bits = mkOption {
+      type = bitType;
+      default = cfg.defaultBitSize;
+      description = ''
+        The bit size for the prime that is used during a Diffie-Hellman
+        key exchange.
+      '';
+    };
+
+    options.path = mkOption {
+      type = types.path;
+      readOnly = true;
+      description = ''
+        The resulting path of the generated Diffie-Hellman parameters
+        file for other services to reference. This could be either a
+        store path or a file inside the directory specified by
+        <option>security.dhparams.path</option>.
+      '';
+    };
+
+    config.path = let
+      generated = pkgs.runCommand "dhparams-${name}.pem" {
+        nativeBuildInputs = [ pkgs.openssl ];
+      } "openssl dhparam -out \"$out\" ${toString config.bits}";
+    in if cfg.stateful then "${cfg.path}/${name}.pem" else generated;
+  };
+
+in {
   options = {
     security.dhparams = {
+      enable = mkOption {
+        type = types.bool;
+        default = false;
+        description = ''
+          Whether to generate new DH params and clean up old DH params.
+        '';
+      };
+
       params = mkOption {
-        description =
-          ''
-            Diffie-Hellman parameters to generate.
-
-            The value is the size (in bits) of the DH params to generate. The
-            generated DH params path can be found in
-            <filename><replaceable>security.dhparams.path</replaceable>/<replaceable>name</replaceable>.pem</filename>.
-
-            Note: The name of the DH params is taken as being the name of the
-            service it serves: the params will be generated before the said
-            service is started.
-
-            Warning: If you are removing all dhparams from this list, you have
-            to leave security.dhparams.enable for at least one activation in
-            order to have them be cleaned up. This also means if you rollback to
-            a version without any dhparams the existing ones won't be cleaned
-            up.
-          '';
-        type = with types; attrsOf int;
+        type = with types; let
+          coerce = bits: { inherit bits; };
+        in attrsOf (coercedTo int coerce (submodule paramsSubmodule));
         default = {};
-        example = { nginx = 3072; };
+        example = lib.literalExample "{ nginx.bits = 3072; }";
+        description = ''
+          Diffie-Hellman parameters to generate.
+
+          The value is the size (in bits) of the DH params to generate. The
+          generated DH params path can be found in
+          <literal>config.security.dhparams.params.<replaceable>name</replaceable>.path</literal>.
+
+          <note><para>The name of the DH params is taken as being the name of
+          the service it serves and the params will be generated before the
+          said service is started.</para></note>
+
+          <warning><para>If you are removing all dhparams from this list, you
+          have to leave <option>security.dhparams.enable</option> for at
+          least one activation in order to have them be cleaned up. This also
+          means if you rollback to a version without any dhparams the
+          existing ones won't be cleaned up. Of course this only applies if
+          <option>security.dhparams.stateful</option> is
+          <literal>true</literal>.</para></warning>
+
+          <note><title>For module implementers:</title><para>It's recommended
+          to not set a specific bit size here, so that users can easily
+          override this by setting
+          <option>security.dhparams.defaultBitSize</option>.</para></note>
+        '';
+      };
+
+      stateful = mkOption {
+        type = types.bool;
+        default = true;
+        description = ''
+          Whether generation of Diffie-Hellman parameters should be stateful or
+          not. If this is enabled, PEM-encoded files for Diffie-Hellman
+          parameters are placed in the directory specified by
+          <option>security.dhparams.path</option>. Otherwise the files are
+          created within the Nix store.
+
+          <note><para>If this is <literal>false</literal> the resulting store
+          path will be non-deterministic and will be rebuilt every time the
+          <package>openssl</package> package changes.</para></note>
+        '';
+      };
+
+      defaultBitSize = mkOption {
+        type = bitType;
+        default = 2048;
+        description = ''
+          This allows to override the default bit size for all of the
+          Diffie-Hellman parameters set in
+          <option>security.dhparams.params</option>.
+        '';
       };
 
       path = mkOption {
-        description =
-          ''
-            Path to the directory in which Diffie-Hellman parameters will be
-            stored.
-          '';
         type = types.str;
         default = "/var/lib/dhparams";
-      };
-
-      enable = mkOption {
-        description =
-          ''
-            Whether to generate new DH params and clean up old DH params.
-          '';
-        default = false;
-        type = types.bool;
+        description = ''
+          Path to the directory in which Diffie-Hellman parameters will be
+          stored. This only is relevant if
+          <option>security.dhparams.stateful</option> is
+          <literal>true</literal>.
+        '';
       };
     };
   };
 
-  config = mkIf cfg.enable {
+  config = lib.mkIf (cfg.enable && cfg.stateful) {
     systemd.services = {
       dhparams-init = {
-        description = "Cleanup old Diffie-Hellman parameters";
-        wantedBy = [ "multi-user.target" ]; # Clean up even when no DH params is set
+        description = "Clean Up Old Diffie-Hellman Parameters";
+
+        # Clean up even when no DH params is set
+        wantedBy = [ "multi-user.target" ];
+
+        serviceConfig.RemainAfterExit = true;
         serviceConfig.Type = "oneshot";
-        script =
-          # Create directory
-          ''
-            if [ ! -d ${cfg.path} ]; then
-              mkdir -p ${cfg.path}
-            fi
-          '' +
+
+        script = ''
+          if [ ! -d ${cfg.path} ]; then
+            mkdir -p ${cfg.path}
+          fi
+
           # Remove old dhparams
-          ''
-            for file in ${cfg.path}/*; do
-              if [ ! -f "$file" ]; then
-                continue
-              fi
-          '' + concatStrings (mapAttrsToList (name: value:
-          ''
-              if [ "$file" == "${cfg.path}/${name}.pem" ] && \
-                  ${pkgs.openssl}/bin/openssl dhparam -in "$file" -text | head -n 1 | grep "(${toString value} bit)" > /dev/null; then
+          for file in ${cfg.path}/*; do
+            if [ ! -f "$file" ]; then
+              continue
+            fi
+            ${lib.concatStrings (lib.mapAttrsToList (name: { bits, path, ... }: ''
+              if [ "$file" = ${lib.escapeShellArg path} ] && \
+                 ${pkgs.openssl}/bin/openssl dhparam -in "$file" -text \
+                 | head -n 1 | grep "(${toString bits} bit)" > /dev/null; then
                 continue
               fi
-          ''
-          ) cfg.params) +
-          ''
-              rm $file
-            done
-
-            # TODO: Ideally this would be removing the *former* cfg.path, though this
-            # does not seem really important as changes to it are quite unlikely
-            rmdir --ignore-fail-on-non-empty ${cfg.path}
-          '';
+            '') cfg.params)}
+            rm $file
+          done
+
+          # TODO: Ideally this would be removing the *former* cfg.path, though
+          # this does not seem really important as changes to it are quite
+          # unlikely
+          rmdir --ignore-fail-on-non-empty ${cfg.path}
+        '';
       };
-    } //
-      mapAttrs' (name: value: nameValuePair "dhparams-gen-${name}" {
-        description = "Generate Diffie-Hellman parameters for ${name} if they don't exist yet";
-        after = [ "dhparams-init.service" ];
-        before = [ "${name}.service" ];
-        wantedBy = [ "multi-user.target" ];
-        serviceConfig.Type = "oneshot";
-        script =
-          ''
-            mkdir -p ${cfg.path}
-            if [ ! -f ${cfg.path}/${name}.pem ]; then
-              ${pkgs.openssl}/bin/openssl dhparam -out ${cfg.path}/${name}.pem ${toString value}
-            fi
-          '';
-      }) cfg.params;
+    } // lib.mapAttrs' (name: { bits, path, ... }: lib.nameValuePair "dhparams-gen-${name}" {
+      description = "Generate Diffie-Hellman Parameters for ${name}";
+      after = [ "dhparams-init.service" ];
+      before = [ "${name}.service" ];
+      wantedBy = [ "multi-user.target" ];
+      unitConfig.ConditionPathExists = "!${path}";
+      serviceConfig.Type = "oneshot";
+      script = ''
+        mkdir -p ${lib.escapeShellArg cfg.path}
+        ${pkgs.openssl}/bin/openssl dhparam -out ${lib.escapeShellArg path} \
+          ${toString bits}
+      '';
+    }) cfg.params;
   };
 }
diff --git a/nixos/release.nix b/nixos/release.nix
index 55b4f19b8688..ae70b535a5e2 100644
--- a/nixos/release.nix
+++ b/nixos/release.nix
@@ -269,6 +269,7 @@ in rec {
   tests.containers-macvlans = callTest tests/containers-macvlans.nix {};
   tests.couchdb = callTest tests/couchdb.nix {};
   tests.deluge = callTest tests/deluge.nix {};
+  tests.dhparams = callTest tests/dhparams.nix {};
   tests.docker = callTestOnMatchingSystems ["x86_64-linux"] tests/docker.nix {};
   tests.docker-tools = callTestOnMatchingSystems ["x86_64-linux"] tests/docker-tools.nix {};
   tests.docker-tools-overlay = callTestOnMatchingSystems ["x86_64-linux"] tests/docker-tools-overlay.nix {};
diff --git a/nixos/tests/dhparams.nix b/nixos/tests/dhparams.nix
new file mode 100644
index 000000000000..d11dfeec5d0c
--- /dev/null
+++ b/nixos/tests/dhparams.nix
@@ -0,0 +1,144 @@
+let
+  common = { pkgs, ... }: {
+    security.dhparams.enable = true;
+    environment.systemPackages = [ pkgs.openssl ];
+  };
+
+in import ./make-test.nix {
+  name = "dhparams";
+
+  nodes.generation1 = { pkgs, config, ... }: {
+    imports = [ common ];
+    security.dhparams.params = {
+      # Use low values here because we don't want the test to run for ages.
+      foo.bits = 16;
+      # Also use the old format to make sure the type is coerced in the right
+      # way.
+      bar = 17;
+    };
+
+    systemd.services.foo = {
+      description = "Check systemd Ordering";
+      wantedBy = [ "multi-user.target" ];
+      unitConfig = {
+        # This is to make sure that the dhparams generation of foo occurs
+        # before this service so we need this service to start as early as
+        # possible to provoke a race condition.
+        DefaultDependencies = false;
+
+        # We check later whether the service has been started or not.
+        ConditionPathExists = config.security.dhparams.params.foo.path;
+      };
+      serviceConfig.Type = "oneshot";
+      serviceConfig.RemainAfterExit = true;
+      # The reason we only provide an ExecStop here is to ensure that we don't
+      # accidentally trigger an error because a file system is not yet ready
+      # during very early startup (we might not even have the Nix store
+      # available, for example if future changes in NixOS use systemd mount
+      # units to do early file system initialisation).
+      serviceConfig.ExecStop = "${pkgs.coreutils}/bin/true";
+    };
+  };
+
+  nodes.generation2 = {
+    imports = [ common ];
+    security.dhparams.params.foo.bits = 18;
+  };
+
+  nodes.generation3 = common;
+
+  nodes.generation4 = {
+    imports = [ common ];
+    security.dhparams.stateful = false;
+    security.dhparams.params.foo2.bits = 18;
+    security.dhparams.params.bar2.bits = 19;
+  };
+
+  nodes.generation5 = {
+    imports = [ common ];
+    security.dhparams.defaultBitSize = 30;
+    security.dhparams.params.foo3 = {};
+    security.dhparams.params.bar3 = {};
+  };
+
+  testScript = { nodes, ... }: let
+    getParamPath = gen: name: let
+      node = "generation${toString gen}";
+    in nodes.${node}.config.security.dhparams.params.${name}.path;
+
+    assertParamBits = gen: name: bits: let
+      path = getParamPath gen name;
+    in ''
+      $machine->nest('check bit size of ${path}', sub {
+        my $out = $machine->succeed('openssl dhparam -in ${path} -text');
+        $out =~ /^\s*DH Parameters:\s+\((\d+)\s+bit\)\s*$/m;
+        die "bit size should be ${toString bits} but it is $1 instead."
+          if $1 != ${toString bits};
+      });
+    '';
+
+    switchToGeneration = gen: let
+      node = "generation${toString gen}";
+      inherit (nodes.${node}.config.system.build) toplevel;
+      switchCmd = "${toplevel}/bin/switch-to-configuration test";
+    in ''
+      $machine->nest('switch to generation ${toString gen}', sub {
+        $machine->succeed('${switchCmd}');
+        $main::machine = ''$${node};
+      });
+    '';
+
+  in ''
+    my $machine = $generation1;
+
+    $machine->waitForUnit('multi-user.target');
+
+    subtest "verify startup order", sub {
+      $machine->succeed('systemctl is-active foo.service');
+    };
+
+    subtest "check bit sizes of dhparam files", sub {
+      ${assertParamBits 1 "foo" 16}
+      ${assertParamBits 1 "bar" 17}
+    };
+
+    ${switchToGeneration 2}
+
+    subtest "check whether bit size has changed", sub {
+      ${assertParamBits 2 "foo" 18}
+    };
+
+    subtest "ensure that dhparams file for 'bar' was deleted", sub {
+      $machine->fail('test -e ${getParamPath 1 "bar"}');
+    };
+
+    ${switchToGeneration 3}
+
+    subtest "ensure that 'security.dhparams.path' has been deleted", sub {
+      $machine->fail(
+        'test -e ${nodes.generation3.config.security.dhparams.path}'
+      );
+    };
+
+    ${switchToGeneration 4}
+
+    subtest "check bit sizes dhparam files", sub {
+      ${assertParamBits 4 "foo2" 18}
+      ${assertParamBits 4 "bar2" 19}
+    };
+
+    subtest "check whether dhparam files are in the Nix store", sub {
+      $machine->succeed(
+        'expr match ${getParamPath 4 "foo2"} ${builtins.storeDir}',
+        'expr match ${getParamPath 4 "bar2"} ${builtins.storeDir}',
+      );
+    };
+
+    ${switchToGeneration 5}
+
+    subtest "check whether defaultBitSize works as intended", sub {
+      ${assertParamBits 5 "foo3" 30}
+      ${assertParamBits 5 "bar3" 30}
+    };
+  '';
+}