Replace simp-le with lego and support DNS-01 challenge #77578

m1cr0man · 2020-01-12T21:40:13Z

Motivation for this change

Continuation of #63613

Closes #34941, #35168

I have the following things still to do:

Add a deprecation notice for security.acme.certs.<name>.plugins (I don't know how, need help)
Similarly add some equivalent of mkChangedOptionModule to convert extraDomains to a list since custom webroots are no longer possible.

It is mostly backwards compatible, with a few caveats.

extraDomains can no longer have different webroots to the main webroot for the cert.
An email address is now mandatory for CA registration. I added a more globally scoped security.acme.email to make transitioning easier, and an assertion.
Accepting terms of service was a topic discussed in the other PR. For now, I have added
security.acme.acceptTerms and defaulted it to true to avoid breaking user's configs. There may be reason to change this default depending on what reviewers think.

The following other changes were required:

Deprecate security.acme.certs.<name>.plugins, as this was specific to simp-le
Rename security.acme.validMin to validMinDays, to avoid confusion and errors. Lego requires the TTL to be specified in days. I have added a mkChangedOptionModule to cover this.
Add options to cover DNS-01 challenge (dnsProvider, credentialsFile, dnsPropagationCheck)
A shared state directory is now used (/var/lib/acme/.lego) to avoid account creation rate limits and share credentials between certs

I have tested converting my own simp-le generated certs to lego and it worked fine. I also have lego in production generating 28 HTTP validated certs and a DNS-01 cert with a bind DNS server (also on a NixOS box 😄 ). This is a non-breaking change if you haven't used custom webroots or plugins.

Also I have tried to keep the diff as small as possible but that other PR was so old it took me a full day to manually rebase on top of all the more recent changes to the ACME module. I'd be tempted to do some other cleanup to the module but there's good git blame as it is now.

Things done

Lego allows users to use the DNS-01 challenge to validate their certificates. It is mostly backwards compatible, with a few caveats. - extraDomains can no longer have different webroots to the main webroot for the cert. - An email address is now mandatory for account creation The following other changes were required: - Deprecate security.acme.certs.<name>.plugins, as this was specific to simp-le - Rename security.acme.validMin to validMinDays, to avoid confusion and errors. Lego requires the TTL to be specified in days - Add options to cover DNS challenge (dnsProvider, credentialsFile, dnsPropagationCheck) - A shared state directory is now used (/var/lib/acme/.lego) to avoid account creation rate limits and share credentials between certs

m1cr0man · 2020-01-12T21:54:48Z

nixos/modules/security/acme.nix

-                          ++ concatMap (p: [ "-f" p ]) data.plugins
-                          ++ concatLists (mapAttrsToList (name: root: [ "-d" (if root == null then name else "${name}:${root}")]) data.extraDomains)
+                email = if data.email == null then cfg.email else data.email;
+                globalOpts = [ "-d" data.domain "--email" email "--path" "." ]


A quick note on --path .. Somewhere between lego 3.0.2 and lego 3.2.0, the default path changed from ./.lego to ~/.lego. I had to add this statement so that lego always runs in the service's WorkingDirectory.

m1cr0man · 2020-01-12T21:55:59Z

Ping @aanderse

Finally, the PR is here.

aanderse · 2020-01-12T23:17:00Z

Thanks @m1cr0man! This is great work. I hope to review and test this over the next few weeks, switching some of my test (and hopefully production) systems over to this.

arianvp · 2020-01-13T08:55:09Z

I will have a ook at this Tuesday. Thanks for the work

arianvp · 2020-01-13T09:44:59Z

nixos/modules/security/acme.nix

+                    # Only try loading the credentialsFile if the dns challenge is enabled
+                    EnvironmentFile = if data.dnsProvider != null then data.credentialsFile else null;
+                    ExecStart = pkgs.writeScript "acme-start" ''
+                      #!${pkgs.runtimeShell} -e


Is it not possible to do what we want in one run of the binary?

Does not look like it does: go-acme/lego#216

In order to enforce the validMinDays you have to run renew instead of run, which seems like an oversight in lego honestly. Run creates the account credentials if they don't exist but forces a new cert to be generated. I was tempted to add explicit logic to check for the cert's credentials files but I feel that's too solution specific. This conditional logic is the same as in the other PR.

aanderse

A couple notes as I work through this...

Some issues with webroot at the moment. The tmpfiles rules reference webroot but for DNS certs this won't be defined.

nixos/modules/security/acme.nix

aanderse · 2020-01-13T15:13:16Z

nixos/modules/security/acme.nix

@@ -173,6 +189,15 @@ in
        '';
      };

+      acceptTerms = mkOption {


What do other people think of this? Personally I think it is overkill... but am fairly indifferent I guess 🤷‍♂️

If Let's Encrypt wants users to accept their terms I don't think implicitly accepting it for endusers should be the way to go. This knob is the exact place where we can point users to the terms of services of Lets Encrypt.

This doesn't make much sense though when overriding the API endpoints (-s|--server) because then we can't know where the correct ToS are.

I don't have a really strong feeling about this. We were implicitly accepting it before no? This seems to make the behaviour slightly better than before

m1cr0man · 2020-01-15T09:30:48Z

If anyone can help me on my two todo's mentioned in the description that would be great. I tried adding a regular mkRemovedOptionModule under certOpts, with a relative attribute path. In order to change extraDomains I tried mkChangedOptionModule but I doubt this works since the attribute path has not changed, and the warning it would produce wouldn't explain the change to a list.

   cfg = config.security.acme;
 
   certOpts = { name, ... }: {
+    imports = [
+      (mkRemovedOptionModule [ "plugins" ] ''
+        This option has been removed since simp-le was replaced with LEGo.
+        You may be able to achieve the desired functionality with postRun
+      '')
+      (mkChangedOptionModule [ "extraDomains" ] [ "extraDomains" ] (config: let
+        domains = config.security.acme."${name}".extraDomains;
+      in if isList domains then domains else attrNames domains))
+    ];
+

These are the errors I got:

The option `security.acme.certs.mydomain.warnings' defined in `/root/nixpkgs/nixos/modules/security/acme.nix' does not exist.
# Removing the mkChangedOptionModule....
The option `security.acme.certs.mydomain.assertions' defined in `/root/nixpkgs/nixos/modules/security/acme.nix' does not exist.

nixos/modules/security/acme.nix

aanderse · 2020-01-15T15:03:11Z

If anyone can help me on my two todo's mentioned in the description that would be great. I tried adding a regular mkRemovedOptionModule under certOpts, with a relative attribute path. In order to change extraDomains I tried mkChangedOptionModule but I doubt this works since the attribute path has not changed, and the warning it would produce wouldn't explain the change to a list.

@m1cr0man this is what @infinisil had to say on the matter: https://logs.nix.samueldr.com/nixos-dev/2020-01-15#1579087078-1579096064

Mic92 · 2020-01-16T10:45:49Z

It also seems that simp-le still uses acmev1 which will be deprecated by letsencrypt in June.

arianvp · 2020-01-16T11:17:57Z

@Mic92 nope don't worry; we already addressed this #71953

aanderse · 2020-01-16T11:22:38Z

I'm very happy with the new DNS challenge functionality. After a few minor changes I mentioned it worked flawlessly. Next step is to test migration of existing HTTP challenge based certificates.

Great work @m1cr0man!

arianvp · 2020-01-16T11:29:08Z

@aanderse how do you envision that migration path? for example; would keeping around account information and private keys from simp_le around be a requirement for you?

m1cr0man · 2020-01-16T11:59:43Z

@arianvp Those are already being kept around. I'm not explicitly deleting the JSON files in /var/lib/acme/${name} so you can revert back pretty painlessly.

m1cr0man · 2020-02-03T21:48:23Z

Sorry for the triple-comment, but I just want to point out that I never figured out how to add the assertions to the submodule. The plugins attribute will raise an assertion anyway since it's not defined anymore. extraModules is currently unchanged but setting a custom webroot is now impossible so it makes more sense to convert it to a list. I'm happy to merge without these and just the tests for now, and work on them later.

nixos-discourse · 2020-02-06T11:51:02Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixos-20-03-feature-freeze/5655/27

m1cr0man · 2020-02-09T02:11:33Z

Ok it was super difficult to figure out, but I got the DNS-01 challenge test working. However in doing this I've somehow broken the test for b.example.com and I'm not sure how. Would someone else be able to take a look at it(@arianvp)?

m1cr0man · 2020-02-09T11:34:49Z

Fixed it - http-01 validation was running before nginx was started. :) This is ready for review again

nixos/doc/manual/release-notes/rl-2003.xml

nixos/tests/acme.nix

flokli · 2020-02-09T16:15:35Z

nixos/tests/acme.nix

+            user = config.services.nginx.user;
+            group = config.services.nginx.group;


Why is this necessary? Can't we use the per-vhost enableACME option in the nginx module, and just reconfigure the dns provider in the acme module?

I'm testing wildcard certs. You can't specify a wildcard cert through enableACME, and that option is tested elsewhere. Personally I use the cert for more than just nginx so I want to ensure it works hard way.

I'm not sure - I'd expect serverAliases to work with that - if not, I'd consider it a bug.

I just find this test hard to understand. - If we can't use the nginx-provided ACME functionalities, probably it'd be more understandable to just not use nginx in the test, only configure the acme module, and check test certificate files appear.

I don't think it's sufficient to simply check does the file exist to truly test wildcard certs. It should work when used with any service (in this case, nginx) serving under that domain. Using nginx here keeps things consistent with the other tests. You're right I could've jused used serverAliases and added *.example.com there and it would've worked fine, but I'm testing the acme module, not nginx.

I meant using nginx, but not the acme-specific support of it makes the test much more verbose. Maybe a nginx test withserverAliases, and a simple one which just looks at the certificate via openssl x509 -in full.pem -text -noout | grep DNS?

flokli · 2020-02-09T16:15:53Z

nixos/tests/acme.nix

+            user = config.services.nginx.user;
+            group = config.services.nginx.group;
+          };
+          systemd.targets."acme-finished-example.com" = {};


Why do we do that? This looks like you're workarounding something…

There's a comment on the earlier ones. It's so that it can be waited upon in the tests.

Please copy that comment over - I got drowned in the rest.

flokli · 2020-02-09T16:16:16Z

nixos/tests/acme.nix

+          systemd.services."acme-example.com" = {
+            wants = [ "acme-finished-example.com.target" ];
+            before = [ "acme-finished-example.com.target" "nginx.service" ];
+            wantedBy = [ "nginx.service" ];
+          };


Same here - I wouldn't expect to have to specify that.

nginx was starting after the certs consistently when I was testing, causing the HTTP-01 validation to fail. Roll it back and run nix-build acme.nix and you'll see what I mean

That's only for the wildcard certificate, right?

No it was for the HTTP-01 challenge certs, aka the ones using enableACME

flokli · 2020-02-09T16:16:59Z

nixos/tests/acme.nix

+            wantedBy = [ "nginx.service" ];
+          };
+          services.nginx.virtualHosts."c.example.com" = {
+            forceSSL = true;


why can't we use enableACME here?

I'm testing wildcard certs. I don't want a cert for c.example.com

flokli · 2020-02-09T16:17:11Z

nixos/tests/acme.nix

+            sslCertificate = config.security.acme.certs."example.com".directory + "/cert.pem";
+            sslTrustedCertificate = config.security.acme.certs."example.com".directory + "/full.pem";
+            sslCertificateKey = config.security.acme.certs."example.com".directory + "/key.pem";


This is then probably not necessary anymore, too.

Merge remote-tracking branch 'remotes/upstream/master'

flokli · 2020-02-09T16:44:34Z

nixos/tests/acme.nix

+            user = config.services.nginx.user;
+            group = config.services.nginx.group;
+          };
+          systemd.targets."acme-finished-example.com" = {};


Please copy that comment over - I got drowned in the rest.

flokli · 2020-02-09T16:45:57Z

nixos/tests/acme.nix

+            user = config.services.nginx.user;
+            group = config.services.nginx.group;


I'm not sure - I'd expect serverAliases to work with that - if not, I'd consider it a bug.

I just find this test hard to understand. - If we can't use the nginx-provided ACME functionalities, probably it'd be more understandable to just not use nginx in the test, only configure the acme module, and check test certificate files appear.

flokli · 2020-02-09T16:46:23Z

nixos/tests/acme.nix

+          systemd.services."acme-example.com" = {
+            wants = [ "acme-finished-example.com.target" ];
+            before = [ "acme-finished-example.com.target" "nginx.service" ];
+            wantedBy = [ "nginx.service" ];
+          };


That's only for the wildcard certificate, right?

flokli · 2020-02-09T16:47:43Z

nixos/tests/acme.nix

          systemd.targets."acme-finished-b.example.com" = {};
          systemd.services."acme-b.example.com" = {
            wants = [ "acme-finished-b.example.com.target" ];
            before = [ "acme-finished-b.example.com.target" ];
+            after = [ "nginx.service" ];
          };
          services.nginx.virtualHosts."b.example.com" = {
            enableACME = true;


we should name these "subdomain certs" a bit more self-explaining, like "webroot-nginx" maybe?

flokli · 2020-02-09T16:47:56Z

nixos/tests/acme.nix

+            wantedBy = [ "nginx.service" ];
+          };
+          services.nginx.virtualHosts."c.example.com" = {
+            forceSSL = true;


disassembler · 2020-02-10T01:58:41Z

@m1cr0man are you going to be able to get this addressed before feature freeze tomorrow?

flokli · 2020-02-10T02:40:44Z

This is mostly me being picky with the tests being a bit too complicated for my taste - the module itself was already tested in "production" environments. FWIW, this can and should be merged for 20.03 - the tests can be improved later on, too.

m1cr0man · 2020-02-10T08:15:02Z

@flokli in general I found these acme tests to be a bit too complex. I would love to rewrite the whole lot of them and incorporate some Apache based tests too, but I can't do that today. I feel that given that there is at least a working test for DNS-01 and the numerous production instances of this code now running that it is stable for 20.03

arianvp · 2020-02-10T08:22:20Z

Yep LGTM and improve the test suite at a later stage

…

On Mon, Feb 10, 2020, 09:15 Lucas Savva ***@***.***> wrote: @flokli <https://github.com/flokli> in general I found these acme tests to be a bit too complex. I would love to rewrite the whole lot of them and incorporate some Apache based tests too, but I can't do that today. I feel that given that there is at least a working test for DNS-01 and the numerous production instances of this code now running that it is stable for 20.03 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77578?email_source=notifications&email_token=AAEZNI2GHLEBHL6W6FGFFMTRCEEIRA5CNFSM4KF2BI5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELHTF7I#issuecomment-584004349>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEZNI7BVEKDKZMO2HZKIJTRCEEIRANCNFSM4KF2BI5A> .

flokli · 2020-02-10T10:47:25Z

Okay, let's merge this, and improve the tests later on.

nixos-discourse · 2021-09-16T21:18:54Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/setup-a-wildcard-certificate-with-acme-on-a-custom-domain-name-hosted-by-powerdns/15055/1

m1cr0man added 2 commits January 12, 2020 21:04

maintainers: add m1cr0man

832d1f4

nixos/acme: Add logic to select right email address

9467f2b

m1cr0man commented Jan 12, 2020

View reviewed changes

aanderse requested review from arianvp, fpletz, wizeman, flokli, worldofpeace, disassembler and infinisil January 12, 2020 23:15

arianvp reviewed Jan 13, 2020

View reviewed changes

ghost mentioned this pull request Jan 13, 2020

[WIP] nixos/acme: switch to lego and support DNS challenge #63613

Closed

10 tasks

aanderse requested changes Jan 13, 2020

View reviewed changes

nixos/acme: ignore tmpfiles rules for null webroots

61665e3

aanderse reviewed Jan 15, 2020

View reviewed changes

nixos/modules/security/acme.nix Outdated Show resolved Hide resolved

nixos/acme: add dns-01 test, fix cert locating bug

ac983cf

nixos/acme: Fix b.example.com test

636eb23

nixos/acme: update release notes for 20.03

d8e697b

mweinelt reviewed Feb 9, 2020

View reviewed changes

nixos/doc/manual/release-notes/rl-2003.xml Outdated Show resolved Hide resolved

mweinelt reviewed Feb 9, 2020

View reviewed changes

nixos/tests/acme.nix Outdated Show resolved Hide resolved

ofborg bot added 8.has: changelog 8.has: documentation This PR adds or changes documentation 2.status: merge conflict This PR has merge conflicts with the target branch labels Feb 9, 2020

flokli reviewed Feb 9, 2020

View reviewed changes

nixos/acme: Update release note, remove redundant requires

75fa802

Merge remote-tracking branch 'remotes/upstream/master'

flokli reviewed Feb 9, 2020

View reviewed changes

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 9, 2020

flokli merged commit 4e0fea3 into NixOS:master Feb 10, 2020

arianvp mentioned this pull request Feb 19, 2020

Merge pull request #80542 from xfix/acme-fix-a-example-com-test #80542

Merged

10 tasks

m1cr0man mentioned this pull request Feb 23, 2020

nixos/acme: adjust renewal timer options #80856

Merged

10 tasks

emilazy mentioned this pull request Feb 23, 2020

nixos/acme: Must-Staple and extra flags #80900

Merged

10 tasks

turboMaCk mentioned this pull request Feb 28, 2020

Unable to resolve breaking changes in ACME NixOS/nixops#1231

Closed

m1cr0man mentioned this pull request Apr 13, 2020

Split lego state directories causes account rate limits #85152

Closed

pvgoran mentioned this pull request Apr 19, 2020

Introducing services.acme.email encourages setups which potentially leak security information #85534

Closed

flokli mentioned this pull request Apr 27, 2020

nixos/acme: document DNS-01 challenge #86114

Closed

		user = config.services.nginx.user;
		group = config.services.nginx.group;

Replace simp-le with lego and support DNS-01 challenge #77578

Replace simp-le with lego and support DNS-01 challenge #77578

Conversation

m1cr0man commented Jan 12, 2020

Motivation for this change

Things done

Choose a reason for hiding this comment

m1cr0man commented Jan 12, 2020

aanderse commented Jan 12, 2020

arianvp commented Jan 13, 2020

Choose a reason for hiding this comment

Mic92 Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aanderse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1cr0man commented Jan 15, 2020

aanderse commented Jan 15, 2020

Mic92 commented Jan 16, 2020 • edited Loading

arianvp commented Jan 16, 2020

aanderse commented Jan 16, 2020

arianvp commented Jan 16, 2020

m1cr0man commented Jan 16, 2020

m1cr0man commented Feb 3, 2020

nixos-discourse commented Feb 6, 2020

m1cr0man commented Feb 9, 2020

m1cr0man commented Feb 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disassembler commented Feb 10, 2020

flokli commented Feb 10, 2020 via email

m1cr0man commented Feb 10, 2020

arianvp commented Feb 10, 2020 via email

flokli commented Feb 10, 2020

nixos-discourse commented Sep 16, 2021

Mic92 Jan 13, 2020 •

edited

Loading

Mic92 commented Jan 16, 2020 •

edited

Loading