acme-dns: init at 0.8 #83474
acme-dns: init at 0.8 #83474
Conversation
ofborg
bot
added
6.topic: nixos
ofborg
bot
added
11.by: package-maintainer
emilazy
force-pushed
the
add-acme-dns
branch
4 times, most recently
from
f2e8e89 to
92e50f3
Compare
|
Bugs found during week writing PR: 50 @GrahamcOfBorg test acme acme-dns |
|
Rebased to fix conflicts. @GrahamcOfBorg test acme acme-dns |
|
I'll take a look today :) |
|
Rebased. @GrahamcOfBorg test acme acme-dns cc @ma72 @mweinelt @arianvp @immae @m1cr0man @aanderse @flokli (perhaps we should have a maintainer group like GNOME and corresponding @NixOS/acme-maintainers ping?); it's a bit fiddly to keep the test cleanups and module changes updated against churn, so I'd like to get this upstreamed sooner rather than later if possible :) I can split the non-acme-dns changes into a separate PR if desired (otherwise when this lands I'll backport them to 20.03). |
|
@GrahamcOfBorg test acme acme-dns (#85376) |
|
@emilazy contrary to your reasoning I prefer we just put this on the back burner until the I'll mention I carefully chose the word "prefer" over "insist" above. Please don't feel I'm insisting. I'm just stating my opinion, and hopefully a reasonable explanation of why. Your idea about a maintainers team is a fantastic idea and I fully support that. |
|
My feeling was that the test cleanups are pretty simple and it'll be a pain to have to backport changes across them if they're not included in 20.03. I do understand the reticence to touch things so close to release, but feel a bit like the recent churn (#84781, #85185, #85333, #85366, ...) is kinda making that a moot point anyway :) I'll defer to what other people think as far as 20.03 goes, but it'd at least be nice to get this into master (we'll have to deal with the test discrepancies if it's merged at any time in the next release cycle anyway). I do appreciate how in need of refactoring the module is, though, so I can keep this rebased in my local tree if people would rather hold off, but the test changes (which are the bulk of the changes in this PR that are hard to keep rebased) should apply just as well to any rewritten module. |
|
I’m only a passing by (not even a real contributor yet :p ) who happened to get poked, so I’ll just give my feeling of it:
Considering that, I think it better to either merge it now or in a few weeks/months after the dust of the ACME change settles (preference for now, since it will become cumbersome to follow every change in your PR), and in any case make both the pre-release and master identical as far as the acme module is concerned. |
|
I'd prefer to have this merged in this release cycle, given there are more risky PRs that are being merged into 20.03 that touch ACME (this one only features new codepath and test renames, so low-risk). |
| @@ -1,27 +1,27 @@ | |||
| # The certificate for the ACME service is exported as: | |||
| # | |||
| # config.test-support.letsencrypt.caCert | |||
| # config.test-support.acme.caCert | |||
There was a problem hiding this comment.
This is not true anymore since pebble. pebble generates its own CA on startup; and we download that from their internal endpoint. This testing code is a bit messy becuase of this.
There was a problem hiding this comment.
edit: nvm I see we configure pebble to use this cert
There was a problem hiding this comment.
My understanding is that this is the CA that issues the static TLS certificate that is used to authenticate access to https://acme.test/ itself, rather than the CA that Pebble uses to issue ACME certificates. Hope this makes sense... the distinction confused me at first too.
There was a problem hiding this comment.
See https://github.com/letsencrypt/pebble#avoiding-client-https-errors; we just generate our own CA rather than using the bundled one. That might not be worth it, though.
There was a problem hiding this comment.
Okay, we can't use the upstream certificates because they're only valid for pebble and localhost. We could reuse their CA since they include the private key and just issue our own certificates for the API, but at that point the simplification is marginal and I'm going to say it's out of scope for this PR.
nixos/tests/acme.nix
Outdated
| client.succeed( | ||
| "curl --cacert /tmp/ca.crt https://c.example.com/ | grep -qF 'hello world'" | ||
| "curl --cacert /tmp/ca.crt https://c.example.test/ | grep -qF 'hello world'" |
There was a problem hiding this comment.
Given we now have
security.pki.certificateFiles = [ acme-ca ];
}
shouldn't we be able to remove these --cacert /tmp/ca.crt calls? If not, why not?
There was a problem hiding this comment.
For the same reason as my previous reply :)
|
Fixed a dangling reference to @ofborg test acme acme-dns |
| set -uo pipefail | ||
|
|
||
| if ! [ -e "$ACME_DNS_STORAGE_PATH" ]; then | ||
| # We use --retry because the acme-dns server might | ||
| # not be up when the service starts (especially if | ||
| # it's local). | ||
| response=$(curl --fail --silent --show-error \ | ||
| --request POST "$ACME_DNS_API_BASE/register" \ | ||
| --max-time 30 --retry 5 --retry-connrefused \ | ||
| | jq ${escapeShellArg "{${builtins.toJSON cert}: .}"}) | ||
| # Write the response. We do this separately to the | ||
| # request to ensure that $ACME_DNS_STORAGE_PATH | ||
| # doesn't get written to if curl or jq fail. | ||
| echo "$response" > "$ACME_DNS_STORAGE_PATH" | ||
| fi | ||
|
|
||
| src='_acme-challenge.${cert}.' | ||
| if ! target=$(jq --exit-status --raw-output \ | ||
| '.${builtins.toJSON cert}.fulldomain' \ | ||
| "$ACME_DNS_STORAGE_PATH"); then | ||
| echo "$ACME_DNS_STORAGE_PATH has invalid format." | ||
| echo "Try removing it and then running:" | ||
| echo ' systemctl restart acme-${cert}.service' | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ! dig +short CNAME "$src" | grep -qF "$target"; then | ||
| echo "Required CNAME record for $src not found." | ||
| echo "Please add the following DNS record:" | ||
| echo " $src CNAME $target." | ||
| echo "and then run:" | ||
| echo ' systemctl restart acme-${cert}.service' | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Please add comments to these issues into the comment at the beginning of the block.
| after = [ "network.target" "network-online.target" ]; | ||
|
|
||
| after = [ "network.target" "network-online.target" ] | ||
| ++ acmeDnsDeps; |
There was a problem hiding this comment.
This dependency shouldn't be added if acme-dns isn't running on the same machine.
I don't think it would hurt to keep both. I propose to document |
Another option is to move it to a wiki page for SSL certs. Considering the importance/size of this module as a whole it would be nice to have regardless. Also I would argue that there are certainly people going in corporate or self hosted environments that do have existing DNS infrastructure which they would want their challenges to go through. You could definitely correlate this to needing wildcard certs in the first place. |
ryantm
added
2.status: merge conflict
| default = {}; | ||
| type = types.submodule { | ||
| options = { | ||
| ip = lib.mkOption { |
There was a problem hiding this comment.
This should be called listen or listenAddress instead.
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
|
@emilazy |
stale
bot
removed
the
2.status: stale
| inherit (src.meta) homepage; | ||
| changelog = "${meta.homepage}/blob/v${version}/README.md#changelog"; |
There was a problem hiding this comment.
Setting homepage explicitly would allow me to visit it right now without copy pasting the url togeter.
| changelog = "${meta.homepage}/blob/v${version}/README.md#changelog"; | ||
| license = lib.licenses.mit; | ||
| maintainers = with lib.maintainers; [ emily ]; | ||
| platforms = lib.platforms.all; |
There was a problem hiding this comment.
This is not required to be set and actually wrong. Go is only available on linux and darwin and duplicating that is not required.
stale
bot
added
the
2.status: stale
|
What is status of this PR? |
stale
bot
removed
the
2.status: stale
|
stale and with merge conflicts, going to close it |
|
@emilazy You already did a considerable work on this. Do you intend to make a new PR? |
|
I stopped using acme-dns quite a long while ago; I'd be happy for someone else to take over this code if they find it useful but I'm unlikely to put more work into the PR myself. Sorry for letting it rot without conclusion. |
|
@emilazy Thank you for the work you have done so far. Did you find an alternative to |
|
It should be possible to use a regular DNS server for the ACME zone (e.g. with lego's RFC 2136 support); I believe some of the "big cloud" DNS providers (e.g. I believe Azure can do this, and probably Amazon and Google too?) let you set up fine-grained enough API key permissions to use with a CNAME'd zone, too. I never got around to setting it up - I just fell back to HTTP validation once this PR bitrotted enough. I forget exactly why I decided against pursuing acme-dns further, but I think it was due to the project's dormancy (at the time; it has had activity since, although it still doesn't seem to be very actively developed and the maintainer was apparently planning a complete rewrite last year) and some lacking features and/or infelicities in its DNS implementation. I also planned to move away from lego in general for ACME so I didn't really have much use left for this PR. |
|
FWIW, the acme test suite uses rfc2136 for DNS-01 validation and the manual covers configuring it too. Potentially you could set up an rfc2136-capable DNS server within NixOS to do what acme-dns achieves, and in the same fashion (CNAME delegation). If there was still interest in an acme-dns-like solution, we could just write a manual section on how to configure a minimal bind to do the job. |
|
Granular API permissions on DNS is virtually non-existent amongst most providers. While we did work on acme-dns for internal use for a while, we have long switched to PowerDNS with a custom granular authorization proxy written in Haskell that wraps the PowerDNS API: https://github.com/wobcom/powerdns-gerd This allows us to grant individual users (or machines in our case) very limited access to the PowerDNS API (constrained by endpoint and a domain name pattern possibly with wildcards) With some work, it could be turned into a nixpkgs derivation along with a NixOS module, but it does require a PowerDNS API somewhere. |
|
I redid parts of this in #234207 in case anyone is still interested :) Without lego/ |
Motivation for this change
acme-dns allows for easy and secure configuration of ACME DNS-01 validation, without the need for DNS-provider-specific hooks or API keys with total control over DNS zones. These commits add acme-dns client integration to the
security.acmemodule, a package and NixOS module for the server, and a test that ties them all together.There are a few things worth noting about this module compared to the average NixOS module:
Uses the same option names and structure as the upstream project; this reduces the amount of boilerplate and templating required in the module and allows for easier porting of configurations to NixOS, at the expense of option names that are less harmonized with the rest of NixOS.
Hardened with an extensive set of sandboxing options, under the general principle that a publicly-exposed network service that essentially has complete power over your certificate renewal ought to run in as restricted an environment as possible. Unfortunately, at present this mostly means it's better-hardened than the web servers it helps issue keys for...
There's also some miscellaneous refactoring and clean-ups to the acme test included. I'm successfully using this to issue certificates with DNS-01 validation on my server.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)