Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/dhparams: Introduce a 'stateful' option #39526

Merged
merged 7 commits into from May 8, 2018

Conversation

aszlig
Copy link
Member

@aszlig aszlig commented Apr 26, 2018

This option 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.

Another advantage of this is that we no longer need to take care of cleaning up the files that are no longer used and in my humble opinion I would have preferred that #11505 (which puts the DH params in the Nix store) would have been merged instead of #22634 (which we have now).

Nevertheless, this now gives the user the choice to either get non-determinism in some store paths or an increased failure vector introduced during bootup.

Besides of the more obvious advantages pointed out, this also effects test runtime if more services are starting to use this (for example see #39507 and #39288), because generating DH params could take a long time depending on the bit size which adds up to test runtime.

If we generate the DH params in a separate derivation, subsequent test runs won't need to wait for DH params generation during bootup.

Apart from just adding the option I added a NixOS VM test to make sure I don't break the existing implementation plus I've cleaned up the module expression a bit.

Cc: @Ekleog

We're going to make changes to the dhparams module so we really want to
make sure we don't break it, so having a NixOS VM test is to make sure
we don't blow things up and can iterate on it.

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @Ekleog
We're going to implement an option which allows us to turn off stateful
handling of Diffie-Hellman parameter files by putting them into the Nix
store.

However, modules now might need a way to reference these files, so we
add a now path option to every param specified, which carries a
read-only value of the path where to find the corresponding DH params
file.

I've also improved the description of security.dhparams.params a bit so
that it uses <warning/> and <note/>.

The NixOS VM test also reflects this change and checks whether the old
way to specify the bit size still works.

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @Ekleog
This option 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.

Another advantage of this is that we no longer need to take care of
cleaning up the files that are no longer used and in my humble opinion I
would have preferred that NixOS#11505 (which puts the dhparams in the Nix
store) would have been merged instead of NixOS#22634 (which we have now).

Luckily we can still change that and this change gives the user the
option to put the dhparams into the Nix store.

Beside of the more obvious advantages pointed out here, this also
effects test runtime if more services are starting to use this (for
example see NixOS#39507 and NixOS#39288), because generating DH params could take
a long time depending on the bit size which adds up to test runtime.

If we generate the DH params in a separate derivation, subsequent test
runs won't need to wait for DH params generation during bootup.

Of course, tests could still mock this by force-disabling the service
and adding a service or activation script that places pre-generated DH
params in /var/lib/dhparams but this would make tests less readable and
the workaround would have to be made for each test affected.

Note that the 'stateful' option is still true by default so that we are
backwards-compatible with existing systems.

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @Ekleog, @abbradar, @fpletz
First of all let's start with a clean up the multiline string
indentation for descriptions, because having two indentation levels
after description is a waste of screen estate.

A quick survey in the form of the following also reveals that the
majority of multiline strings in nixpkgs is starting the two beginning
quotes in the same line:

$ find -name '*.nix' -exec sed -n -e '/=$/ { n; /'\'\''/p }' {} + | wc -l
817
$ find -name '*.nix' -exec grep "= *'' *\$" {} + | wc -l
14818

The next point is to get the type, default and example attributes on top
of the description because that's the way it's rendered in the manual.

Most services have their enable option close to the beginning of the
file, so let's move it to the top.

Also, I found the script attribute for dhparams-init.service a bit hard
to read as it was using string concatenation to split a "for" loop.

Now for the more substantial clean ups rather than just code style:

  * Remove the "with lib;" at the beginning of the module, because it
    makes it easier to do a quick check with "nix-instantiate --parse".
  * Use ConditionPathExists instead of test -e for checking whether we
    need to generate the dhparams file. This avoids spawning a shell if
    the file exists already and it's probably more common that it will
    exist, except for the initial creation of course.
  * When cleaning up old dhparams file, use RemainAfterExit so that the
    unit won't be triggered again whenever we stop and start a service
    depending on it.
  * Capitalize systemd unit descriptions to be more in par with most
    other unit descriptions (also see 0c5e837).
  * Use "=" instead of "==" for conditionals using []. It's just a very
    small nitpick though and it will only fail for POSIX shells. Bash on
    the other side accepts it anyway.

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @Ekleog
@aszlig
Copy link
Member Author

aszlig commented Apr 26, 2018

As a side note, I'd also want to add another option to use only a single dhparams file for all services (to reduce generation overhead further - after all the dhparams are public and sent to the user during key exchange), but I think that's something for another pull request.

Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIce addition! Personally I don't like putting something non-deterministic into store but I do see advantages of that.

Copy link
Member

@Ekleog Ekleog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! Just a tiny question about bit size (not trying to ignite a flame war, if you think it's better with 4096 after reading my comment then please do so :))

name = "bits";
description = "integer of at least 16 bits";
};
default = 4096;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a default of 4096 is maybe too much? See certbot/certbot#4973 ; Let's Encrypt supposedly know what they are doing and use a pre-generated 2048-bit DH params (and using the same DH params as others is quite bad, even compared to lower bit size, if I correctly remember the attacks available -- because it increases by as much the value of breaking the group).

Basically I don't have anything personal against 4096, but fear it may re-start the arms race: people like having “more security” than their distributions, and having NixOS already having more security than is actually useful (I personally don't know whether a real-size quantum computer will come before or after our being able to break 2048-bit keys, let alone 3072-bit ones -- see wikipedia for some numbers).

So basically, I'd have set it to 3072 in order to both decrease build time and avoid having people setting it to 8192 and complaining about how slow things are, but that's just my opinion. :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you that 4096 bit is too much, so I'll change it to 2048 instead which is also the OpenSSL default for numbits. Another thing I have been thinking about is for users to be able to set the default size for all of the dhparams generated.

@Ekleog writes in NixOS#39526:

> I think a default of 4096 is maybe too much? See certbot/certbot#4973;
> Let's Encrypt supposedly know what they are doing and use a
> pre-generated 2048-bit DH params (and using the same DH params as
> others is quite bad, even compared to lower bit size, if I correctly
> remember the attacks available -- because it increases by as much the
> value of breaking the group).

> Basically I don't have anything personal against 4096, but fear it may
> re-start the arms race: people like having "more security" than their
> distributions, and having NixOS already having more security than is
> actually useful (I personally don't know whether a real-size quantum
> computer will come before or after our being able to break 2048-bit
> keys, let alone 3072-bit ones -- see wikipedia for some numbers).

> So basically, I'd have set it to 3072 in order to both decrease build
> time and avoid having people setting it to 8192 and complaining about
> how slow things are, but that's just my opinion. :)

While he suggests is 3072 I'm using 2048 now, because it's the default
of "openssl dhparam". If users want to have a higher value, they can
still change it.

Signed-off-by: aszlig <aszlig@nix.build>
This allows to set the default bit size for all the Diffie-Hellman
parameters defined in security.dhparams.params and it's particularly
useful so that we can set it to a very low value in tests (so it doesn't
take ages to generate).

Regardless for the use in testing, this also has an impact in production
systems if the owner wants to set all of them to a different size than
2048, they don't need to set it individually for every params that are
set.

I've added a subtest to the "dhparams" NixOS test to ensure this is
working properly.

Signed-off-by: aszlig <aszlig@nix.build>
This is not only to make users aware of the changes but also to give a
heads up to developers which are using the module. Specifically if they
rely on security.dhparams.path only.

Signed-off-by: aszlig <aszlig@nix.build>
@aszlig
Copy link
Member Author

aszlig commented May 7, 2018

@Ekleog, @abbradar: I've added a new option defaultBitSize, which can be used to set a default for all the bit sizes in the defined parameters. In addition I updated the documentation a bit to give module developers a few hints about how to use the module in other modules and a release notes entry to make users aware of this.

@Ekleog
Copy link
Member

Ekleog commented May 7, 2018

LGTM, thanks! 👍

@aszlig aszlig merged commit a8b7372 into NixOS:master May 8, 2018
aszlig added a commit that referenced this pull request May 8, 2018
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 #39507.

Thanks to @Ekleog and @abbradar for reviewing.
aszlig added a commit that referenced this pull request May 10, 2018
The pull request that added dhparams (#39507) was made at the time where
the dhparams module overhaul (#39526) wasn't done yet, so it's still
using the old mechanics of the module.

As stated in the release notes:

  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).

  An example usage of this would be:

    { config, ... }:

    {
      security.dhparams.params.myservice = {};
      environment.etc."myservice.conf".text = ''
        dhparams = ${config.security.dhparams.params.myservice.path}
      '';
    }

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @qknight, @abbradar, @hrdinka, @leenaars
globin pushed a commit to mayflower/nixpkgs that referenced this pull request May 24, 2018
The pull request that added dhparams (NixOS#39507) was made at the time where
the dhparams module overhaul (NixOS#39526) wasn't done yet, so it's still
using the old mechanics of the module.

As stated in the release notes:

  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).

  An example usage of this would be:

    { config, ... }:

    {
      security.dhparams.params.myservice = {};
      environment.etc."myservice.conf".text = ''
        dhparams = ${config.security.dhparams.params.myservice.path}
      '';
    }

Signed-off-by: aszlig <aszlig@nix.build>
Cc: @qknight, @abbradar, @hrdinka, @leenaars
(cherry picked from commit 67a8c66)
globin pushed a commit to mayflower/nixpkgs that referenced this pull request May 24, 2018
@Ekleog writes in NixOS#39526:

> I think a default of 4096 is maybe too much? See certbot/certbot#4973;
> Let's Encrypt supposedly know what they are doing and use a
> pre-generated 2048-bit DH params (and using the same DH params as
> others is quite bad, even compared to lower bit size, if I correctly
> remember the attacks available -- because it increases by as much the
> value of breaking the group).

> Basically I don't have anything personal against 4096, but fear it may
> re-start the arms race: people like having "more security" than their
> distributions, and having NixOS already having more security than is
> actually useful (I personally don't know whether a real-size quantum
> computer will come before or after our being able to break 2048-bit
> keys, let alone 3072-bit ones -- see wikipedia for some numbers).

> So basically, I'd have set it to 3072 in order to both decrease build
> time and avoid having people setting it to 8192 and complaining about
> how slow things are, but that's just my opinion. :)

While he suggests is 3072 I'm using 2048 now, because it's the default
of "openssl dhparam". If users want to have a higher value, they can
still change it.

Signed-off-by: aszlig <aszlig@nix.build>
(cherry picked from commit b3d5ca8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants