nixos/acme: Features and bug fixes #147784
Conversation
github-actions
bot
added
6.topic: nixos
github-actions
bot
added
8.has: changelog
8.has: documentation
ofborg
bot
added
10.rebuild-darwin: 0
m1cr0man
force-pushed
the
acme
branch
2 times, most recently
from
1cb0fba to
a37c994
Compare
|
The PR is now ready for review + user testing. If you are directly affected/helped by one of the changes (e.g. the new defaults option, using DNS validation + a web server, removing extraDomainNames entries) it would be awesome if you could give these changes a try. |
|
Rebased to cover #147498, and moved security.acme.enableDebugLogs -> security.acme.defaults.enableDebugLogs |
|
I tested this for some days already with dns and web server validation. |
|
I hope you don't mind, I pushed a commit to this PR to clean up the defaults handling a bit! |
| @@ -732,7 +717,7 @@ in { | |||
|
|
|||
| certs = mkOption { | |||
| default = { }; | |||
| type = with types; attrsOf (submodule certOpts); | |||
| type = with types; attrsOf (submodule [ (inheritableModule false) certOpts ]); | |||
There was a problem hiding this comment.
Thanks for the update @infinisil ! I never thought of using inherit (function call) resultValues before.
I don't quite follow what is happening on this line though. Comparing the diff, am I correct in saying that the attrs in both inheritableModule + certOpts are being recursively merged? No objections to this, it's a much cleaner solution to what I was doing.
|
Rebased on nixos-unstable's current commit (for easy testing) and added an automated test for #125256 EDIT: Oh, there's still merge conflicts.. guess I'll rebase on master. |
ofborg
bot
added
the
2.status: merge conflict
|
Merge conflicts resolved now. It would be nice to get this merged soonish so that it doesn't keep generating conflics. |
ofborg
bot
removed
the
2.status: merge conflict
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/problem-with-security-acme-and-listenhttp-option/16790/5 |
selfsignedDeps is already appended to the after and wants of a cert's renewal service, making these redundant. You can see this if you run the following command: systemctl list-dependencies --all --reverse acme-selfsigned-mydomain.com.service
Closes NixOS#108237 When a user first adds an ACME cert to their configuration, it's likely to fail to renew due to DNS misconfig. This is non-fatal for other services since selfsigned certs are (usually) put in place to let dependant services start. Tell the user about this in the logs, and exit 2 for differentiation purposes.
Closes NixOS#129838 It is possible for the CA to revoke a cert that has not yet expired. We must run lego to validate this before expiration, but we must still ignore failures on unexpired certs to retain compatibility with NixOS#85794 Also changed domainHash logic such that a renewal will only be attempted at all if domains are unchanged, and do a full run otherwises. Resolves NixOS#147540 but will be partially reverted when go-acme/lego#1532 is resolved + available.
Allows configuring many default settings for certificates, all of which can still be overridden on a per-cert basis. Some options have been moved into .defaults from security.acme, namely email, server, validMinDays and renewInterval. These changes will not break existing configurations thanks to mkChangedOptionModule. With this, it is also now possible to configure DNS-01 with web servers whose virtualHosts utilise enableACME. The only requirement is you set `acmeRoot = null` for each vhost. The test suite has been revamped to cover these additions and also to generally make it easier to maintain. Test config for apache and nginx has been fully standardised, and it is now much easier to add a new web server if it follows the same configuration patterns as those two. I have also optimised the use of switch-to-configuration which should speed up testing.
- Added defaultText for all inheritable options. - Add docs on using new defaults option to configure DNS validation for all domains. - Update DNS docs to show using a service to configure rfc2136 instead of manual steps.
In the process I also found that the CapabilityBoundingSet was restricting the service from listening on port 80, and the AmbientCapabilities was ineffective. Fixed appropriately.
This test is technically broken since reloading caddy does not seem to load new certs. This needs to be fixed in caddy.
|
Rebased, and I added a test for #147973, however I discovered that Caddy doesn't seem to load new certificates on reload. Pinging @aanderse so he can confirm this. I was going to make acme restart Caddy on renewal by default but this feels wrong for a web server. I have augmented the test suite for now, to prevent blocking this PR further. |
There was a problem hiding this comment.
@mweinelt yeah I don't want to hold this up. I did some research last night and didn't find a concrete answer, just implications that a reload should be sufficient. I'm going to ping upstream and get some answers and can create PRs to amend this is appropriate.
Please good ahead with this PR 👍
Motivation for this change
Closes #147348, #138478, #129838, #108237, #140906, #101389, #140709, #101389
Things done
Add
security.acme.defaultsandsecurity.acme.certs.<name>.inheritDefaultsoptions for easy setting of configuration across multiple certs. Additionally the following options have been moved into defaults: server, email, validMinDays and renewInterval.Add
security.acme.useRootto allow having cert files owned by root. This is a dirtier solution than usingLoadCredentialbut the majority of people probably will use this regardless. It at least gives users the choice when it comes to the (quite niche) security concerns it raises. See Allow configuring root as the owner of acme certificate files #140709Always try a
lego reneweven if within validMinDays to check for revocation by CA. Errors are ignored if within validMinDays to respect ACME renewals fail due to DNS being unavailable during switch #85794Always perform a
lego runif the domainHash is changed, to resolve Allow configuring root as the owner of acme certificate files #140709 and work around Renew only specified domains go-acme/lego#1532 (which I plan to try and fix myself upstream).Modify
services.nginx.virtualHosts.<name>.acmeRootto accept null and subsequently support inheriting the webroot value from the newsecurity.acme.defaults. The intention here is usingenableACMEwith DNS-01 validation.Refactor of the test suite to reduce the number of configuration switches, remove old race condition workarounds and standardise test suite between Nginx and HTTPd whilst making it easier to add new web server tests in the future. It also covers the above
enableACME+DNS-01 use case too. Total run time is 192 seconds on my system.Add
StartLimitIntervalSec=0to avoid unwanted rate limiting (despite theConditionPathExists) of the selfsigned cert management services.Updated the documentation on configuring DNS validation to use a service to generate the tsig key rather than some manual steps. This resolves nixos/acme: 20.03 -> 20.09 regression with opensmtpd #101389
Built on platform(s)
For non-Linux: Is
sandbox = trueset innix.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 usageTested basic functionality of all binary files (usually in
./result/bin/)21.11 Release Notes (or backporting 21.05 Release notes)
nixos/doc/manual/md-to-db.shto update generated release notesFits CONTRIBUTING.md.