nixos/acme: Various fixes #191861
nixos/acme: Various fixes #191861
Conversation
Fixes NixOS#191794 Lego threw a permission denied error binding to port 80. AmbientCapabilities with CAP_NET_BIND_SERVICE was required. Also added a test for this.
Fixes NixOS#190493 Check if an actual key file exists. This does not completely cover the work accountHash does to ensure that a new account is registered when account related options are changed.
Lego has a built-in mechanism for sleeping for a random amount of time before renewing a certificate. In our environment this is not only unnecessary (as our systemd timer takes care of it) but also unwanted since it slows down the execution of the systemd service encompassing it, thus also slowing down the start up of any services its depending on. Also added FixedRandomDelay to the timer for more predictability.
|
Implemented the changes discussed in review comments. I am marking this as ready for review now since I don't know what the best course of action is to resolve #180980 for now, and will fix it at a later date. |
|
@ofborg test acme |
| # Avoid a random 0-8 minute sleep when testing renewals. | ||
| # We are not using LE servers in testing so this is not | ||
| # going to impact their load. | ||
| # See https://github.com/go-acme/lego/issues/1656 | ||
| extraLegoRenewFlags = ["-no-random-sleep"]; |
There was a problem hiding this comment.
Considering this is removed in the subsequent commit, would it make sense to just not add it in this commit?
There was a problem hiding this comment.
Hm, I'm normally pretty adverse to editing commits because it alerts all the related issues again when you force push. Also I think these commits show a good evolution of the feature?
|
re: "This does not completely cover the work accountHash does to ensure that a new account is registered when account related options are changed." What is this referencing, the new mechanism? |
So accountHash works to ensure that we are putting accounts for particular configurations in their own folder. The check for an existing cert file is solving a different issue, that's what I'm implying. Does that make sense? |
|
Yeah, well enough. :) |
We didn't actually have an automated test for that configuration 😅 I added one in this PR though. Thanks for merging! |
Description of changes
Closes #191794 - Adds an AmbientCapabilities line to permit binding on port 80
Closes #190493 - Adds a check for an actual account key file rather than any file/folder in accounts
WIP #180980 - Will fix the web server config for nginx.No response will fix in another PR.CC @NixOS/acme
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes