NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.06k stars 14.12k forks source link

acme.defaults.dnsProvider is not actually used as a default #210807

Open andrewhamon opened 1 year ago

andrewhamon commented 1 year ago

Describe the bug

I've set acme.defaults.dnsProvider = "cloudflare";, however my certs don't seem to inherit this particular default and continue to use http validation.

Steps To Reproduce

Minimal repro (I know this isn't a complete config, but its enough to demonstrate the lack of inheritance):

flake.nix

{
  outputs = { self, nixpkgs }: {
    nixosConfigurations.test = nixpkgs.lib.nixosSystem {
      system = "x86_64-linux";
      modules = [
        ({ ... }: {
          security.acme.defaults.email = "default@example.com";
          security.acme.defaults.dnsProvider = "cloudflare";
          security.acme.defaults.credentialsFile = "/var/run/secrets/cloudflare";

          services.nginx.enable = true;
          services.nginx.virtualHosts."example.com" = {
            enableACME = true;
          };
        })
      ];
    };
  };
}

nix repl session:

nix repl --extra-experimental-features 'flakes repl-flake' .
nix-repl> nixosConfigurations.test.config.security.acme.defaults.dnsProvider
"cloudflare"

nix-repl> nixosConfigurations.test.config.security.acme.certs."example.com".dnsProvider
null

nix-repl> nixosConfigurations.test.config.security.acme.certs."example.com".email
"default@example.com"

As you can see, "example.com" does not have a dnsProvider set, however it does correctly inherit email.

Expected behavior

nixosConfigurations.test.config.security.acme.certs."example.com".dnsProvider should be set to the default, "cloudflare" in this case.

Notify maintainers

@aanderse @andrew-d @arianvp @emilazy @flokli @m1cr0man

Metadata

Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 5.15.85, NixOS, 22.11 (Raccoon), 22.11pre-git`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.11.1`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`
andrewhamon commented 1 year ago

I think this behavior is caused by the nginx module here: https://github.com/NixOS/nixpkgs/blob/a83ed85c14fcf242653df6f4b0974b7e1c73c6c6/nixos/modules/services/web-servers/nginx/default.nix#L990-L999

andrewhamon commented 1 year ago

I was able to make all my nginx vhosts respect my acme defaults by overriding the default value of acmeRoot to null. Here is my nixos module:

{ lib, ... }:
with lib;
{
  options.services.nginx.virtualHosts = lib.mkOption {
    type = lib.types.attrsOf (lib.types.submodule {
      config.acmeRoot = lib.mkDefault null;
    });
  };
}
m1cr0man commented 1 year ago

Just above the quoted text in your first comment is the hasRoot definition https://github.com/NixOS/nixpkgs/blob/a83ed85c14fcf242653df6f4b0974b7e1c73c6c6/nixos/modules/services/web-servers/nginx/default.nix#L989-L995

In nginx' vhost options, there is a default set for the acmeRoot https://github.com/NixOS/nixpkgs/blob/1cb8b4a401a3dbd952c4aff24441d5afa6d52335/nixos/modules/services/web-servers/nginx/vhost-options.nix#L87-L89

As you discovered, this means that without explicitly setting acmeRoot to null, security.acme.defaults won't be inherited. The way this works is based on the fact optionDefaults have a priority of 1500 and thus the priority of 2000 conditionally applied here means that the default in the acme module (aka the value in security.acme.defaults) will be used instead.

I believe this issue hasn't popped up before because people usually either use HTTP-01 for single domains with the default acmeRoot or use DNS-01 wildcard certs which require setting useAcmeHost on the vhost anyway. Your use case appears DNS-01 but not with wildcard certificates.

We can't change default behaviour, certainly we cannot remove the default value for acmeRoot, so the best bet here would be documenting this behaviour in the manual so that people understand why it happens without knowing the priority of default options and all the other complexity happening here.

andrewhamon commented 1 year ago

I believe this issue hasn't popped up before because people usually either use HTTP-01 for single domains with the default acmeRoot or use DNS-01 wildcard certs which require setting useAcmeHost on the vhost anyway. Your use case appears DNS-01 but not with wildcard certificates.

In my case, DNS validation is preferred for some non-public hosts, and while using a wildcard would work, it would lead to a slightly more complex (and less portable) vhost config :)

We can't change default behaviour, certainly we cannot remove the default value for acmeRoot, so the best bet here would be documenting this behaviour in the manual so that people understand why it happens without knowing the priority of default options and all the other complexity happening here.

Would logging a warning be appropriate as well?

peperunas commented 1 year ago

Is there any update on this issue? I am encountering it as well.

m1cr0man commented 1 year ago

Is there any update on this issue? I am encountering it as well.

The solution for now is to set your virtualHost's acmeRoot to null. I'm considering other solutions beyond just adding a warning, such as maybe checking if the default dnsProvider is set. Unfortunately there are very few solutions that wouldn't be potentially breaking changes for other users.

peperunas commented 1 year ago

I see, thank you very much!

Giulio De Pasquale

PhD Candidate in Program Analysis for Computer Security King’s College London, Strand Campus

Website: https://pepe.runas.rocks Schedule a meeting: https://cal.com/giulio-de-pasquale/hello

On 28 May 2023 at 02:02 -0700, Lucas Savva @.***>, wrote:

Is there any update on this issue? I am encountering it as well. The solution for now is to set your virtualHost's acmeRoot to null. I'm considering other solutions beyond just adding a warning, such as maybe checking if the default dnsProvider is set. Unfortunately there are very few solutions that wouldn't be potentially breaking changes for other users. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>