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
dovecot2: added ssl_dh using security.dhparams #39507
Conversation
This and #39288 are duplicates |
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; I'll merge in several days if no issues arise.
@@ -25,6 +25,7 @@ let | |||
ssl_cert = <${cfg.sslServerCert} | |||
ssl_key = <${cfg.sslServerKey} | |||
${optionalString (!(isNull cfg.sslCACert)) ("ssl_ca = <" + cfg.sslCACert)} | |||
ssl_dh = </var/lib/dhparams/dovecot2.pem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use ${config.security.dhparams.path}
instead of /var/lib/dhparams
here, in order to be resilient to change ;)
security.pam.services.dovecot2 = mkIf cfg.enablePAM {}; | ||
|
||
services.dovecot2.protocols = | ||
security.dhparams = mkIf (! isNull(cfg.sslServerCert)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a presentation note, wouldn't the usual style in nixpkgs be ! isNull cfg.sslServerCert
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems there all kind of forms:
lib/sources.nix: in if isNull matchRef
lib/sources.nix: in if isNull matchRef
lib/trivial.nix: mapNullable = f: a: if isNull a then a else f a;
nixos/maintainers/option-usages.nix: if !(isNull testOption) then [ testOption ]
nixos/modules/config/fonts/fontconfig-penultimate.nix: fcPackage = if builtins.isNull version
nixos/modules/config/fonts/fontconfig.nix: fcPackage = if builtins.isNull version
nixos/modules/config/sysctl.nix: checkType = x: isBool x || isString x || isInt x || isNull x;
nixos/modules/programs/ssmtp.nix: ${optionalString (!isNull cfg.authPassFile) "AuthPassFile=${cfg.authPassFile}"}
nixos/modules/services/backup/znapzend.nix: nullOff = b: if isNull b then "off" else toString b;
nixos/modules/services/backup/znapzend.nix: "" = optionalString (! isNull host) "${host}:" + dataset;
nixos/modules/services/continuous-integration/buildkite-agent.nix: { assertion = cfg.hooksPath == hooksDir || all isNull (attrValues cfg.hooks);
nixos/modules/services/continuous-integration/jenkins/default.nix: if isNull cfg.plugins
nixos/modules/services/databases/pgmanage.nix: ${optionalString (!isNull cfg.loginGroup) "login_group = ${cfg.loginGroup}"}
nixos/modules/services/databases/pgmanage.nix: ${optionalString (!isNull cfg.tls) ''
my favorite:
nixos/modules/services/networking/znc.nix: notNull = a: ! isNull a;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh indeed, I was mostly thinking about the parenthesis-around-a-single-argument point :)
security.dhparams = mkIf (! isNull(cfg.sslServerCert)) { | ||
enable = true; | ||
params = { | ||
dovecot2 = 4096; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need to set 4096 bits for dhparams here by default, that's very slow to generate and there's not much value in having it so high, 2048 should be enough for most use cases, and 3072 for the paranoid (like me). See for instance certbot/certbot#4973, where Let's Encrypt gives hardcoded 2048-bit DH params (which is way worse in security than what we have in NixOS with the DH params auto-generated, as they aren't used by others)
If the folder /var/lib/dhparams/ does not exist, I think this fails. The patch should probably include a: mkdir -p /var/lib/dhparams |
@leenaars , the path will be created and managed by the dhparams service. It should however use |
The 18.03 channel includes dovecot 2.3, which requires ssl_dh to be set. -> fixes nixcloud/nixcloud-webservices#21
456f5d3
to
1a1dc79
Compare
@Ekleog updated as requested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qknight Thanks! Just, I think you lost a space at the beginning of https://github.com/nixcloud/nixpkgs/blob/1a1dc797a7c938f3cfaaa094d05ef6245930d879/nixos/modules/services/mail/dovecot.nix#L309? :)
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.
Tested that Dovecot service builds and configuration looks sensible, didn't test that it fully works -- I expect this has already been tested. Thanks! |
About 18.03 merge -- I'm personally okay with merging #39526 too as it is backwards compatible. If no contra voices arise in the coming days I can go ahead. |
On the other hand this change is not fully backwards compatible because before people have probably specified |
@abbradar your observation is correct and i think that dovecot will then have a runtime breakage however, in |
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
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)
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)
Request
Motivation for this change
The 18.03 channel includes dovecot 2.3, which requires ssl_dh to be set.
-> fixes nixcloud/nixcloud-webservices#21
without this fix, dovecot2's IMAP functionality using ACME won't work
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)