Support OverrideDomain is DNS01Solver

[crccw]

CNAME can be used to delegate answering the chanllenge to another DNS
zone, in order to reduce the exposure of the DNS credential [1]. The
solver already follows CNAME when checking propagation against the
authoritative source, it should follow CNAME when setting the TXT record
as well.

[1] https://letsencrypt.org/docs/challenge-types/#dns-01-challenge

[francislavoie]

This is related to caddyserver/caddy#4071

So from my reading of the code, this would allow DNS records to directly affect where certmagic would attempt to mutate the TXT record.

I'm not sure that's proper. It is kinda cool to make it work dynamically like that, but it also kinda scares me, that it might have some unintended vector for abuse. But maybe I'm being paranoid. I just think we need to be careful.

I'd be more comfortable if there was an explicit domain override configurable instead of following the CNAME record. This is how it's set up in the DuckDNS plugin, so that is explicit at the config level. That config could be in certmagic itself instead, one layer higher (and Caddy would add an option for it).

Maybe if you could enumerate the ways this could be used, to prove it would be infeasible to abuse, it would feel more comfortable.

[crccw]

Yes it would allow DNS record to affect where certmagic would attempt to mutate the TXT record.

It certainly makes sense to be paranoid when it comes to security. Though, I'm possibly being naive here, I don't feel it increases the exposure. In the end ACME checks the TXT record, which is independent from how the record is set. If a malicious party is able to put the TXT record there with this commit, they should be able to put it there without this commit too.

Being said, I like the idea of having a domain override configurable. It's simpler than following CNAME, and is more test friendly. If we do this, should we also stop following CNAME when checking propagation for consistency?

[mholt]

Yeah, I think if it's specified in the config directly it should not be overrideable by any DNS records.

[mholt]

@crccw Any interest in continuing work/discussion on this PR?

[crccw]

Yes! Sorry for the delay, I'll update the PR in a day or two.

[crccw]

Updated. I've tested the change together with caddyserver/caddy#4596. Let me know if there's a way to write a test against. I guess we need to mock a DNS server?

[mholt]

Ah, thanks for the update. LGTM now, let's give it a try. Really appreciate your contribution here! I get antsy trying to mess with this kind of code myself.

If there are new issues reported related to this, would you be able to help maintain this change?

[crccw]

Yeah I'm happy to help.

[emilylange]

Hi @crccw :)
What was the motivation behind 4be5af5#diff-0d423173eaa496408ee9e7d82ae770feaa3b860783ca6a155f6d7551cb5d4d14R337-R341

checkDNSPropagation and checkAuthoritativeNss don't seem to be actually interchangeable in that case here:

• dnsName has to be a fqdn in checkAuthoritativeNss but not in checkDNSPropagation

  certmagic/dnsutil.go

  Lines 212 to 215 in 9a56fcd

  	func checkDNSPropagation(fqdn, value string, resolvers []string) (bool, error) {
  	if !strings.HasSuffix(fqdn, ".") {
  	fqdn += "."
  	}

• resolvers don't share a common format: checkAuthoritativeNss takes resolvers without port to add :53 by itself, while checkDNSPropagation assumes the port already set.

• certmagic/dnsutil.go

  Line 243 in 9a56fcd

  r, err := dnsQuery(fqdn, dns.TypeTXT, []string{net.JoinHostPort(ns, "53")}, false)

  needs to be recursive=true in my testings. (Though I am not sure what exactly that changes, but wouldn't resolve with false for me)

Using checkDNSPropagation in both cases (so dropping the above if/else again) seems to work just fine on the other hand.

[crccw]

Hi @IndeedNotJames ,

The motivation is that if OverrideDomain is set and we succeeded in adding a TXT record on it, that domain should have no CNAME record as CNAME record cannot co-exist with other records. Since it has no CNAME record, there's no need trying to follow the CNAME record.

(Now I feel that this applies to the case without OverrideDomain, but it's good to keep the behavior unchanged for now)

As for the other points I think you're there're some bugs here, let me try to fix it.

[mholt]

@crccw Was it intentional that the override domain always has to be prefixed with _acme_challenge.? Since that's part of the DNS-01 challenge spec, I am not sure why we would always want to make the user include that in the override domain...

[francislavoie]

IMO we could adjust it to add that prefix to the configured override if it doesn't have the prefix already.

[mholt]

Yeah, as long as there's not a legitimate reason to not have that subdomain, I'm good with that. I can make that patch shortly.

[crccw]

@mholt My memory has faded a little, but IIRC OverrideDomain can be anything. For the delegation use case, the user needs to create a CNAME record on the _acme_challenge.original.domain that points to the OverrideDomain. In this way they only need to expose the control of _acme_challenge.original.domain to caddy, instead of the original.domain. My understanding is that the CNAME should be, in some means, transparent to the ACME server; they just follow the CNAME chains when checking the TXT record on the _acme_challenge.original.domain.

Have you run into any issues if the override domain doesn't start with _acme_challenge?

[mholt]

@crccw Thanks for replying! Yeah, we're trying to figure out, from this topic in the forum here: https://caddy.community/t/global-dns-challenge-and-dns-challenge-override-domain/16773

Is our patch (prepend _acme-challenge. if it doesn't exist) correct, do you think?

[crccw]

I don't think so. In the post the author has a CNAME record from _acme-challenge.manage.clientsite.com > _acme-challenge.myexamplesite.com, therefore they need to set override_domain to the CNAME target _acme-challenge.myexamplesite.com. The CNAME target can be anything, e.g. _some-random-stuff.myexamplesite.com, in which case override_domain should be _some-random-stuff.myexamplesite.com accordingly.

[mholt]

Oh I see. So since the original CNAME record has _acme-challenge. subdomain, the configured override domain must also have it.

If they just set it to manage.clientsite.com, then they would not need it in override domain, correct? And that is probably the normal use case.

So their need seems to be a bit of an edge case, in which specifying the _acme-challenge subdomain in the override domain is the correct solution.

I'll look into reverting my patch.

[mholt]

Reverted in 9e63f36

[crccw]

That's right, if the CNAME points to manage.clientsite.com, then override domain should be set to manage.clientsite.com.