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/caddy: set extraDomainNames for acme #239982

Closed

Conversation

jian-lin
Copy link
Contributor

Description of changes

Doing so is reasonable and nginx module also does so.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 26, 2023
@jian-lin jian-lin requested review from aanderse and emilylange June 26, 2023 18:39
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 26, 2023
@jian-lin jian-lin requested a review from m1cr0man June 27, 2023 13:40
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

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

  • does this merge properly if you have multiple virtual hosts using the same useACMEHost value?
  • does this PR solve a specific problem you are having in your configuration?

@jian-lin
Copy link
Contributor Author

jian-lin commented Jun 27, 2023

does this merge properly if you have multiple virtual hosts using the same useACMEHost value?

Good catch. Currently, it does not.

does this PR solve a specific problem you are having in your configuration?

This does solve a problem, which I do not have because I do not use serverAliases.

The problem occurs when the acme module (lego) uses caddy to complete http-01 challenges.

  • The acme module is needed because the service FOO which uses those certs needs to get notified when the certs are renewed.
  • caddy is needed because it is used as a reverse proxy for some other services.
  • http-01 challenge is used because dns-01 is not possible. tls-alpn-01 challenge is similar to http-01 challenge here.

In this case, it's easy to overlook that extraDomainNames needs to be set to serverAliases. I guess that's why the nginx module sets it.

An example for FOO is NixOS Mailserver with non-empty mailserver.certificateDomains and mailserver.certificateScheme = "acme".

So one decides to mimic the nginx config of NixOS Mailserver for caddy:

{ config, ... }:

let
  fqdn = "mail.example.com";
in
{
  mailserver = {
    ...
    fqdn = "mail.example.com";
    certificateDomains = [ "imap.example.com" ];
    certificateScheme = "acme";
  };
  services.caddy = {
    ...
    virtualHosts.${fqdn} = {
      useACMEHost = fqdn;
      serverAliases = config.mailserver.certificateDomains;
      extraConfig = ''
        file_server /.well-known/acme-challenge/* {
          root ${config.security.acme.certs.${fqdn}.webroot}
        }
      '';
    };
  };
  security.acme.certs.${fqdn} = {
    webroot = "...";
    # XXX he overlooks extraDomainNames
  };
}

IMHO, setting extraDomainNames in caddy module is a good idea.

@aanderse
Copy link
Member

The acme module is needed because the service FOO which uses those certs needs to get notified when the certs are renewed.

Wouldn't it make sense to just have caddy ping your mail server when new certificates are available? It looks like @emilylange already had some thoughts and comments on that in caddyserver/caddy#3643.

Really the useACMEHost was added a workaround to avoid building caddy with plugins. I'm not sure if it is a good idea to expand it much or not 🤔

What do you think @emilylange?

@jian-lin
Copy link
Contributor Author

jian-lin commented Jul 1, 2023

Wouldn't it make sense to just have caddy ping your mail server when new certificates are available?

That would be great but I do not think caddy is able to do that.

does this merge properly if you have multiple virtual hosts using the same useACMEHost value?
Really the useACMEHost was added a workaround to avoid building caddy with plugins.

What is this use case? From the description you added for useACMEHost option and the above lines, it seems that you want to use a wildcard cert (dns-01 challenge) for multiple domains with caddy. That is also a quite less common use case. Since this PR is based on the nginx module, does the nginx module support that?

That said, it is possible to change this PR to properly merge domains. However, I consider my implementation inelegant.

This is more complicate than I thought, and I do not have enough time for it. So I will close this PR.

Thanks for your review.

@jian-lin jian-lin closed this Jul 1, 2023
@jian-lin jian-lin deleted the feature-caddy-acme-extraDomaines branch July 1, 2023 06:09
@aanderse
Copy link
Member

aanderse commented Jul 1, 2023

I think caddy does support this with the additions of events.

@emilylange
Copy link
Member

A bit late to the party, but yeah, Caddy does support a variant of this via events using e.g. https://github.com/mholt/caddy-events-exec

But that is still in a somewhat early stage, and building Caddy with plugins in nixpkgs is quite a hassle as of right now, unfortunately.
See #14671

@jian-lin
Copy link
Contributor Author

jian-lin commented Jul 2, 2023

Thanks for the info of caddy event handler, which did not exist last time I checked1.

Using https://github.com/mholt/caddy-events-exec and giving the caddy user permissions to reload the needed service seems doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants