Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with resolvers #219

Closed
MisterDuval opened this issue Mar 15, 2023 · 20 comments
Closed

Issue with resolvers #219

MisterDuval opened this issue Mar 15, 2023 · 20 comments
Labels
bug Something isn't working

Comments

@MisterDuval
Copy link

Hi,
I'm having issue with resolvers, my caddyFile looks like:

[...]
tls {
dns cloudflare MY_API_KEY
resolvers 1.1.1.1
}
But I'm getting this error because IT blocks DNS requests except from 1.1.1.1.
"error": "[URL] solving challenges: waiting for solver certmagic.solverWrapper to be ready: checking DNS propagation of "_acme-challenge.URL": dial tcp 108.162.195.175:53: i/o timeout

This IP is related to Cloudflare NS

@mholt
Copy link
Member

mholt commented Mar 15, 2023

Did you verify that the config you think you're running is actually the config that's running?

In isolation, the config you gave works. What is your full config that we can reproduce the issue? (Including domain names)

@MisterDuval
Copy link
Author

Yes, my config file is valid and tested, maybe that "resolvers" is just ignored?
As soon as I allowed that other DNS IP (108.162.195.175) in firewall, it has worked. You can try with a ca https://acme-staging-v02.api.letsencrypt.org/directory and a firewall blocking all DNS requests except 1.1.1.1 (port TCP 53)

Caddy v2.6.4 (on Windows), compiled with xcaddy and caddy-exec + caddy-dns-cloudflare

caddyFile:

{
    email MY_EMAIL
}

https://mycustomdomain.ltd
{
    tls {
   	    dns cloudflare MYAPIKEY
	    resolvers 1.1.1.1
	}

    root htdocs

    encode gzip
    file_server
    php_fastcgi 127.0.0.1:9123
    log {
       output file logs/caddy-error.log
       level ERROR
    }
}

@mholt
Copy link
Member

mholt commented Mar 16, 2023

Thanks for the follow-up. It seems unlikely that they're being ignored, however, if you have a moment to try putting a fmt.Println() line just before this line:

ready, err = checkDNSPropagation(dnsName, keyAuth, resolvers)

And maybe a few more in the call stack down the checkDNSPropagation() function would likely prove which resolvers are being used. If it's not 1.1.1.1 then it should be easier to figure out why.

@mholt mholt added the bug Something isn't working label Mar 16, 2023
@mholt
Copy link
Member

mholt commented Jun 15, 2023

@MisterDuval Any interest in digging into this?

@MisterDuval
Copy link
Author

Sorry I had no time for this for now.

@mholt mholt added the help wanted Extra attention is needed label Jun 16, 2023
@mholt
Copy link
Member

mholt commented Jun 16, 2023

No worries. I'll probably close it for now, since I think we'll need your help to troubleshoot this. We can reopen when ready 👍

@mholt mholt closed this as not planned Won't fix, can't repro, duplicate, stale Jun 16, 2023
@pgeh
Copy link
Contributor

pgeh commented Mar 5, 2024

I seem to have run into the same problem with caddy and a split DNS setup. As far es I can tell the problem is that the checkDNSPropagation function doesnt' use the given resolvers to get the IPs for den authoritative name servers of the domain. In my case this results in a request to the name servers internal IP and zone, where there are no TXT records, and in @MisterDuval a request to the Cloudflare DNS Server.

The problematic code is the call to checkAuthoritativeNss(...) which doesn't use the given resolvers to resolve the authoritative name servers IPs.

I tried a quick and dirty fix by resolving and returning the name server names via the resolvers in lookupNameservers and returning the IPs instead of the fqdns. With that it worked and caddy could finish the cert creation, but I don't think that would be a valid solution.

@mholt Does that explanation make sense to you? Imho this is a bug as the explicitly given resolvers are not used throughout the whole checkDNSPropagation(...) process. A solution would be most welcome as that prevents me to use caddy in my environment.

@mholt
Copy link
Member

mholt commented Mar 5, 2024

@pgeh Actually I'd be interested in seeing the patch as a PR so I can review it, if you'd be ok with that! Why wouldn't it be a valid solution?

@pgeh
Copy link
Contributor

pgeh commented Mar 6, 2024

@mholt I added a PR with a possible fix.
BUT I now learned that this doesn't actually work with my split DNS setup when I'm actually in the internal network (instead of using VPN where it worked yesterday). It should probably fix it for @MisterDuval but not for me.

Also this PR does only use A records at the moment and doesn't work with IPv6 as I'm not sure how and if I should implement this properly.

My question now is: Would a working solution be to skip the check of the authoritative nameservers if there are resolvers given? I think it wouldn't contradict the documentation for the resolvers option and would fit the intention of the user in checking if the TXT is available via the given resolvers.

@mholt
Copy link
Member

mholt commented Mar 6, 2024

@pgeh Thanks for the PR! I think I see what you mean now.

My question now is: Would a working solution be to skip the check of the authoritative nameservers if there are resolvers given?

That... might make sense, yeah. The authoritative check is, of course, important to get a view like what the CA will when it does its authoritative lookup. If specifying your own resolvers is the only way to overcome the split horizon DNS situation, then maybe we should just do that. The user will have to configure resolvers that it is confident will return an authoritative lookup though.

@pgeh
Copy link
Contributor

pgeh commented Mar 8, 2024

@mholt Any suggestions on how to move forward with this? I would gladly provide a PR with my idea of skipping the authoritative nameserver check when resolvers are given, but I can't tell if you actually approved the idea. I know this is probably a very niche problem, but it would allow me to switch from certbot (which works with my DNS setup, but needs to be replaced due to other reasons) to caddy.

@mholt
Copy link
Member

mholt commented Mar 8, 2024

Yeah let's see a PR if you have a moment! Sorry to be unclear. I think it's worth exploring it just sounds like it's up to the user to choose good resolvers that reflect what the CA would see accurately.

@francislavoie
Copy link
Member

FWIW I think this explains a lot of confusing behaviour I've seen when helping some users in the forums regarding propagation checks. Sometimes they'd configure resolvers 1.1.1.1 or whatever and it would still fail, surprisingly. Thanks for finding this @pgeh! I think I agree with the changes.

@pgeh
Copy link
Contributor

pgeh commented Mar 12, 2024

There are still one problem/question with this solution: The current implementation in my PR checks all given resolvers for the correct TXT entry, but you would normally give more then one resolver to have fallbacks in case one doesn't work (i.e. 1.1.1.1 and 1.0.0.1). So the question is, should it be enough if one of the resolvers is valid? This would reduce the "quality" of the check further but maybe fit user expectations better. What do you think?

@francislavoie
Copy link
Member

I think the point of the propagation checks is to just make a best effort "did our DNS operation work?" check. So if a single resolver says it's good, then the change was successful, and it can be handed off to the ACME issuer.

@pgeh
Copy link
Contributor

pgeh commented Mar 12, 2024

Ok, I can change my PR to require only one successful check. Can this also change for the default behavior so that one successful authoritative nameserver check is enough? It would be an easier fix, I just have to adjust the loop in checkNameservers(). But if I should preserve the original behavior that's ok too, just takes some more effort.

@francislavoie
Copy link
Member

IMO yes, one success is enough, but Matt may have a different opinion so I'll let him chime in first.

@mholt
Copy link
Member

mholt commented Mar 14, 2024

I agree one authoritative check -- for our purposes -- is enough. Of course it's more brittle than multiple, multi-vantage lookups, but we're not a CA here :)

@pgeh
Copy link
Contributor

pgeh commented Mar 14, 2024

@mholt I updated the PR request to now require only one nameserver check to have the record in order to succeed. #274

@mholt
Copy link
Member

mholt commented Mar 14, 2024

Thank you @pgeh! We'll give it a shot :)

@mholt mholt closed this as completed Mar 14, 2024
@mholt mholt removed the help wanted Extra attention is needed label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants