I am planning to deprecate the activationDelay option as its implementation is a bit confusing, and it can better be replaced with systemd dependencies instead of waiting for an arbitrary time. (e.g. what happens if the time passes, but whatever script you run during the delay didn't finish??). It is currently implemented by calling systemctl start in a postRun which requires PermissisionStartOnly which I want to get rid of because it is deprecated

I think a good replacement instead would be to create a acme-wait-dns-published.service yourself ,, similar to network-wait-online.service which busyloops until a condition is met and then do:

# acme-wait-dns-published.service
RequiredBy=acme-${cert}.service
After=acme-${cert}.service

This way, the delay isn't an arbitrary number, but mandated by the script that you use to publish to DANE/TLSA.
and it greatly simplifies the letsencrypt activation logic, which is now hard to follow.

See e70d293#commitcomment-34131909 for context

CC @pngwjpgh

SGTM. Any fixed "delay" value is inherently wrong.

One big problem:

You can't waitForUnit on oneshot services, as their state goes from inactive => activating => inactive. This makes the tests currently fail.

If anybody has an idea on how to test for oneshots firing, please let me know

@grahamc I've fixed the test by letting each oneshot activate a target (Which remains active after running) to use as a probe to test for unit activation. It's a bit hacky, but it works!

@GrahamcOfBorg test acme

A few problems remain (that were already present before I started coding):

1. certificates add dependencies to nginx and lighttpd even if they're not used by said services.
   Say you create a certificate, and an nginx vhost, but that vhost doesn't have enableACME. then nginx will still wait for certificates to refresh even if those aren't used by nginx at all
2. lighttpd seems to have no direct ACME enable support whatsoever so it seems weird to make lighttpd.service pull in all these certs even if it doesn't use them

Number 2. was introduced by @bjornfor in 7a0e958

Would you mind if I remove this coupling altogether, and document instead that people
can do:

lighttpd.wants  =  [ "acme-${cert}.service acme-selfsigned-${cert}.service" ];
lighttpd.after = ["acme-selfsigned-${cert}.service"];

whenever you want to depend on certs being generated on startup?

For nginx we can probably do this automatically when a vhost has enableACME = true; but lets make sure it only pulls in the certs for that specific vhost instead of all certs

This will also fix #62958

@arianvp: I don't mind. I only added that line because nginx had it, and it felt like the natural thing to do at that time.

This is ready for review. I can refactor even more but lets keep that for a new PR or a new NixOS Module altogether. At least the bugs that were present before are fixed now.

I think we should do the big refactor to DNS challenges and lego in a separate PR and maybe even in a separate NixOS module. it's not worth keeping backwards compatibility with what we have currently I think. It might hamper us coming up with better designs. @mweinelt would you perhaps want to take a look at this PR?

Thanks!

FWIW, I was actually using security.acme.directory, so removing that has given me some grief >.>.

Feel free to come up with an alternative. We could use systemd.tmpfiles for example to manage the directory instead of StateDirectory. What is important is that we have the behaviour without depending on the deprecated PermissionStartOnly behaviour of systemd. however I don't personally have time to work on this at least the coming two weeks. But I'm happy to review the PR that reintroduces that option. I would poke @aanderse for advice if he has time.

Not sure tmpfiles is the right approach either... the usage of it in the refactored module is also somehow not working right for me -- the activation script snippet for it is failing with "Detected unsafe path transition" =/. I'll take a look into this tonight.

This is error log is generated because of:

        /* Returns true if the transition from a to b is safe, i.e. that we never transition from unprivileged to
         * privileged files or directories. Why bother? So that unprivileged code can't symlink to privileged files
         * making us believe we read something safe even though it isn't safe in the specific context we open it in. */

https://github.com/systemd/systemd/blob/6fd79cca68a170c88e87b5d95cd7a44953df94e2/src/basic/fs-util.c#L663
*/

Note that it's just a warning that is logged. not something that makes fatally fail. But still; it's something worth having a look at.

Poking @aanderse

I still think that tmpfiles is the right approach here. Note that tmpfiles is a bit of a deceptive name (because of historic reasons) but is the adviced tool to use if StateDirectory= does not suffice to set up state directories for a service (e.g. when you want to allow state to live outside of /var/lib) (See #56265 for context)

1. Entirely out of curiosity: what directory are you using, and will symlinks not solve your problem?
2. If tmpfiles.d is not explicitly told who ownsa directory it will assign root as the owner. This includes parent directories of the directory you specify. Under certain permission schemes tmpfiles.d decides this is not safe (the right choice) and throws the error/warning. See nixos/deluge: fix directory creation errors #67888 for concrete example.

I'm happy to hear the what and why of your custom directory to change my opinion, but as it currently stands if symlinks would do the job I continue to agree with the decision to remove the directory option.

@arianvp Hmm, yeah, I see why my setup is causing the warning now. My config predates services.nginx.virtualHosts.<name>.enableACME existing, so the link between the acme webroot and the nginx one was manually defined, and the ownership of that portion of the directory tree ended up being a little odd. I'll change it to use the newer stuff, which is nicer, so that's good.

@aanderse Sure, I can detail the reasoning a bit: essentially, I keep any service-related data that is worth backing up* under a directory tree rooted at /srv, which simplifies administration in a few ways (e.g. I don't have to back up different service directories per-server, I can configure filesystem/disk-related stuff for all relevant service data on a system in a single place).

/var/lib doesn't qualify for this by because it is just generically for service/application state, not specifically for stuff I consider valuable, meaning it inevitably ends up containing a bunch of random stuff I don't care about and don't want consuming backup space. The semantics don't match up.

Could I use a symlink? Often. But then I have more problems:

• If I decide to manually create the symlink, I now have to remember to do that if I am ever moving or replicating this configuration elsewhere.
• If I decide to write a service or activation script or something else to ensure the symlink exists, I then have to test that, ensure it has the appropriate dependencies, and try to cover any edge cases that might come up.
• I can always symlink from /srv/<something> to elsewhere, but I can't always do the reverse -- the application in question may not like being given a symlink in its path, for one reason or another, so instead I'd have to use a bind mount (which then has its own potential issues).

Pointing the service in question directly to the directory I actually want it to use is always going to be the better option.

*: By "worth backing up", I mean "not trivially reproduceable". LE certs fall under this classification because acquiring them is rate-limited, so having to recreate them en masse would potentially be problematic. Combine that with HTTP Strict Transport Security and you could end up with effectively inaccessible domains for a while.

@Shados to address your first two points: if you use systemd.tmpfiles.rules to create the symlink and those problems disappear. I see your third point as something worth serious consideration, but on a service by service basis.

I'm not against having an option for specifying the state directory when there is a justification for such an option (like an application that can't deal with symlinks as you mentioned), but it would be nice if there was justification, as opposed to preference. But then again... this is just my opinion, and I don't make the rules.

@aanderse I got around to taking the symlink approach with systemd.tmpfiles.rules. It works, but it is deceptively complicated, and it quickly became apparent that this is really not what systemd-tmpfiles is designed for. Here's a bit of a post-mortem, so to speak:

Initially, I went with just a directory entry for /srv/acme (with the same user and owner that I have the acme services running as), and a symlink entry for /var/lib/acme -> /srv/acme, but this caused an issue.

Namely, I was back to getting the "unsafe path transition" warnings. And while this is just a warning for systemd-tmpfiles, and it still creates the path(s), it is an error for NixOS because the non-0 return code causes the activation script snippet to fail, which means the system profile activation does not continue.

The cause of the warning was: systemd-tmpfiles will ensure that leading directories are implicitly created if needed, but they will be owned by root with an access mode of 0755. As a result, there was an unsafe path transition going from /srv/acme to /srv/acme/acme-challenge, because the latter is implicitly created as root on the way to making /var/lib/acme/acme-challenge/.well-known/acme-challenge (through the symlink).

man tmpfiles.d says In order to create [the leading directories] with different modes or ownership make sure to add appropriate d lines, so I wrote a Nix helper to map a portion of a path to a list of largely identical systemd.tmpfiles.rules directory creation lines. I do note an issue with this approach currently:
If you have two separate entries creating the same directory with different permissions or ownership, you'll get a warning on stderr to that effect during activation, but it appears the return code is still 0, so it is non-obvious.
We could solve that in NixOS by making systemd.tmpfiles.rules a structured attrsOf tmpfileRuleType instead of a listOf str, and I would strongly suggest this be done if the recommendation going forward is to handle service/etc. directory creation using systemd.tmpfiles.rules. It would also then be possible to catch some potential "unsafe path transitions" at eval-time.

All up: I do think that using a tool designed to declaratively manipulate a filesystem is the right approach to solving this kind of issue in NixOS, but that using systemd-tmpfiles as-is may not be. It ended up taking about as much testing and handling of edge cases as I expected a hand-written, imperative approach would have.

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

https://discourse.nixos.org/t/please-update-to-latest-19-09-if-using-letsencrypt/4595/3