dhparams module: initialize #22634
dhparams module: initialize #22634
Conversation
|
@Ekleog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers. |
|
See also #12912 |
|
OK, I have just been told about #11505 and #12912 . Compared to the first one, it avoids non-determinism, and compared to the second one, it is way shorter. The aim is that it makes services using this as a backend (like nginx could, by default) avoid having to bother about cleaning up the state when the dhparams are no longer needed. That said, it can be deemed useless, and given the current status of the two other PRs, it wouldn't be surprising, but I think if at least 4 persons have a use for it (only counting people who post on github, with the authors of 3 PRs and an issue) it may actually be used? |
|
In the absence of the nginx module doing this by itself, I would use this. (I.e. I need this for nginx.) |
nixos/modules/security/dhparams.nix
Outdated
| 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 `${dir}/[name].pem` |
There was a problem hiding this comment.
If you want markup, please use the appropriate docbook elements here, <filename> perhaps.
There was a problem hiding this comment.
Something like <filename>${dir}/<replaceable>name</replaceable>.pem</filename> might do it.
nixos/modules/security/dhparams.nix
Outdated
| 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 `${dir}/[name].pem` |
There was a problem hiding this comment.
Could dir be configurable or at least exposed somehow? I'd rather not hard-code this path but use something like ${security.dhparams.name.path} in my nginx config.
There was a problem hiding this comment.
Hmm, I get the issue but can't think of a clean UI to do it. Adding a .path requires to move the size to a .size argument (that's OK), but then people could override the .path argument… which would not be an issue, if I had any idea of how to make a default value for an argument dependent on the name of its parent (to be able to default to ${dir}/[name].pem.
Do you have an idea? Or would just putting a security.dhparamsPath defaulting to /var/cache/dhparams be OK with your use case?
|
If we want to have a generic solution like my old PR I think that doing that in systemd is better since it greatly improves boot/switch times (imagine you'd need to wait for 3-4 dhparams to be created if done via activation script vs. doing this concurrently with other dhparams and other services). |
|
@Ekleog I'd be willing to help push these changes through. Let me know. |
|
@abbradar: That's completely true, your old PR cut down to the bare minimum (DH params don't actually need to be hidden from anyone in any threat model that I know of, and the main issue with your PR was that it was too long, as far as I read it) is most likely a better option than this one. I can also rewrite this PR as a couple of systemd services, as it also handles cleanup of removed dhparams, as you want. (I wouldn't want to "steal" the systemd service idea from you.) How do you feel about taking this forwards? |
|
@Ekleog Admittedly I have little time to give love to my old PR nowadays, so I'm completely okay with you stealing the credit ^_^. One small feature that I'd like to be saved is automatic add of cc @edolstra -- how do you feel about this? With old dhparams cleaning this becomes a bit more than my old PR (not a one-line call now :D) I agree that it still may be strange to have this outside nginx, but OTOH a lot of services (any TLS-supporting server in fact) are in need of the same functionality. |
|
OK, I just pushed an overhaul of the PR using a systemd service. I also added two features I had forgotten about: most importantly, the fact that if the number of bits asked for is changed, the DH parameters have to be rebuilt; and also trying to remove also now-empty directories after removing DH params from the config. From an UI point of view, I added the That said, I think the signal/noise ratio of the module is pretty good: it has 11 lines of documentation and 18 lines that would have to be duplicated between all TLS-supporting services for just 18 lines that qualify as non-trivial module-supporting interface (including the 8 lines of non-trivial option definitions). What do you think about this renewed PR? |
|
@Ekleog I think you pushed the wrong commit, I can't see a systemd service in the diff. I agree with your comments and your basic approach though. 👍 |
|
@Ekleog I agree that we don't need a separate option for |
Ekleog
force-pushed
the
dhparams
branch
2 times, most recently
from
f4173f6
to
31bb2c0
Compare
|
Oh, I pushed the wrong commit indeed -_- Here it is, thanks for the notice! |
|
Hmm, is the "approved these changes" a new policy for nixpkgs, is it for making hydra build the PR, or is it for some other reason? It's the first time I've seen this in use. Also, for putting this in a systemd service, as it's only one call to openssl per dh params that were enabled at last generation, I'm thinking it's negligible in time? ( (edit: I had failed my |
|
@Ekleog We don't have a policy around the approved changes feature yet. You may want to use different dhparams per service if you want to make fingerprint TLS harder. For instance if you run multiple tor hidden services you might want to hide the fact that they're running on the same machine. There are other fingerprinting techniques of course, but dhparams is one of them and if you use custom dhparams it's even more important to use different ones. ;) |
|
Here is a version using a systemd service instead of an activation script for old dhparams cleanup. Is this looking all good now? :) (edit: oh, and I had forgotten that one may change the size of DH params from 512 bits to 1512 bits, which would not have been reflected by the module, now the parenthesis are included in the openssl match so it should be OK, even though a very rare occurrence) |
|
How's this coming? I'm pretty excited about this. |
|
I don't really know what this is waiting for, but given the 17.03 milestone I'm hopeful, as branch-off is in 3 days :) |
|
I was waiting for input from other contributors that didn't open a pull request for a dhparams module before (me and @abbradar basically 😃). We're a bit biased on that topic. |
|
Great it's OK, then! 😃 |
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
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 (cherry picked from commit 3e11ff6)
Motivation for this change
Generating per-computer Diffie-Hellman parameters is both something advisable in security, and something that is slow and painful to do by hand.
This also fixes #22632
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)(Most checkboxes just don't apply to this module-only PR)
Hope this helps!
Leo