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

dovecot2: added ssl_dh using security.dhparams #39507

Merged
merged 1 commit into from May 8, 2018

Conversation

qknight
Copy link
Member

@qknight qknight commented Apr 25, 2018

Request
  • please merge into master
  • please merge into 18.03
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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@fgaz
Copy link
Member

fgaz commented Apr 25, 2018

This and #39288 are duplicates

aszlig added a commit to aszlig/nixpkgs that referenced this pull request 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 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
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.

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
Copy link
Member

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)) {
Copy link
Member

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?

Copy link
Member Author

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;

Copy link
Member

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;
Copy link
Member

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)

@leenaars
Copy link
Contributor

If the folder /var/lib/dhparams/ does not exist, I think this fails. The patch should probably include a:

mkdir -p /var/lib/dhparams

@hrdinka
Copy link
Contributor

hrdinka commented Apr 29, 2018

@leenaars , the path will be created and managed by the dhparams service. It should however use config.security.dhparams.path instead of the hardcoded one as pointed out by @Ekleog

The 18.03 channel includes dovecot 2.3, which requires ssl_dh to be set.
-> fixes nixcloud/nixcloud-webservices#21
@qknight
Copy link
Member Author

qknight commented Apr 30, 2018

@Ekleog updated as requested

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.

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
Copy link
Member

aszlig commented May 8, 2018

@vcunat: If we add this to 18.03, it might be a good idea to also merge #39526 as well, because it allows to query the individual paths to the DH params files. This change here however would need to be adjusted to not only use the right path but also not set a bit size.

@abbradar
Copy link
Member

abbradar commented May 8, 2018

Tested that Dovecot service builds and configuration looks sensible, didn't test that it fully works -- I expect this has already been tested. Thanks!

@abbradar abbradar merged commit 851d5d7 into NixOS:master May 8, 2018
@abbradar
Copy link
Member

abbradar commented May 8, 2018

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.

@abbradar
Copy link
Member

abbradar commented May 8, 2018

On the other hand this change is not fully backwards compatible because before people have probably specified ssh_dh in their extra configuration -- suddenly they have different values coming from them and from this change. Not sure this is a good idea after all.

@qknight
Copy link
Member Author

qknight commented May 8, 2018

@abbradar your observation is correct and i think that dovecot will then have a runtime breakage

however, in nixcloud.email we cover both types of setup and when you decide to backport it we just adapt and whoever runs a mailserver on top of nixpkgs WILL be able to handle this breakage and possibly even welcome it. your choice.

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

Dovecot 2.3 requires ssl_dh=</path/to/dh.pem
8 participants