- Notifications
You must be signed in to change notification settings - Fork2.2k
net/dns/{publicdns,resolver}: add NextDNS DoH support#5556
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dee6825 tob6d6cbdCompare
crawshaw left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure our code is going to do the right thing if your only DNS server is NextDNS, so there is no IPv4 address. It should work, because we have an address to give the OS, but I got lost trying to read through the code.
| // hasDefaultIPResolversOnly reports whether the only resolvers in c are | ||
| // DefaultResolvers, and that those resolvers are simple IP addresses. | ||
| // DefaultResolvers, and that those resolvers are simple IP addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have spent a minute trying to decide if that should be anand or anor. I don't know. Maybe this function needs a different name?tailscaleOwnsDNS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From the code, it seems to be 'and'.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
maisem left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume you've tested that this works?
bradfitz commentedSep 7, 2022
I've tested it but will do more after latest control changes. I also have some questions for NextDNS on how to test it better. |
bradfitz commentedSep 8, 2022
And added more tests. |
41aa508 toe49c102Compare
danderson left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks reasonable. My main request is to try and clarify in the publicdns package the difference between IPs that can be used for UDP querying, and those which are just static pre-resolutions of DoH resolver names. It took me several confused reads through the forwarder code to figure out that there were two non-equivalent sets of IPs (IPs indicating you should DoH to NextDNS, and IPs to which you should send those DoH queries) being kicked around when configuring for NextDNS resolution.
| // hasDefaultIPResolversOnly reports whether the only resolvers in c are | ||
| // DefaultResolvers, and that those resolvers are simple IP addresses. | ||
| // DefaultResolvers, and that those resolvers are simple IP addresses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
From the code, it seems to be 'and'.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
net/dns/publicdns/publicdns.go Outdated
| } | ||
| // DoHIPsOfBase returns a DoH base URL's IP addresses. | ||
| // It is basically the inverse of DoHEndpointFromIP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it basically the inverse, or exactly the inverse? I think you can drop "basically".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Okay, it's not exactly the inverse, because this one also returns IPv4s.
Can you make the docs of this function clarify that it's not returning IPs which are the equivalent of the DoH URL, it's returning a static resolution for the URL's hostname? It took me many re-reads to figure out that the returned IPs from this function cannot be used for UDP queries to NextDNS, only as a static resolution of the URL's hostname.
And drop the mention of being the inverse of DoHEndpointFromIP, that to me suggested that, just like DohEndpointFromIP simply lets you uplift a UDP DNS query into DoH, this one lets you go the other way - which would be true except for the IPv4 results which lose information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated comment.
| ifb,ok:=m[ip];ok { | ||
| returnb,false,true | ||
| } | ||
| ifnextDNSv6RangeA.Contains(ip)||nextDNSv6RangeB.Contains(ip) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about the IPv4 NextDNS servers? Constants are declared for them, but they don't seem to be in KnownDOH or handled specially here. Shouldn't we at least do "naked" DoH without the client ID in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We only do DoH to NextDNS if we know the profile ID. The IPv4 addresses are only used for dialing DoH, not for regular DNS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated comment.
| { | ||
| base:"https://dns.nextdns.io/c3a884", | ||
| want:ips( | ||
| "45.90.28.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How will this work? If I translate the DoH name to IPs, the IPv4s lack a client ID, so presumably are not going to behave the same as the v6 endpoints? Should this function only return v6 addrs, so that you don't lose fidelity roundtripping from URL > IP > URL ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These are the IPs for dialing DoH.
On the wire from control to clients we'll only ever send the NextDNS v6 IP containing the profile ID in the lower bytes. We'll eat any NextDNS v4 address server side as if the user didn't specify it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated the comment on the func
net/dns/publicdns/publicdns.go Outdated
| // The returned map should not be modified. | ||
| funcKnownDoH()map[netip.Addr]string { | ||
| populateOnce.Do(populate) | ||
| returnknownDoH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can't comment any higher, but KnownDOH is now broken, it doesn't return all known DoH. How does this affect callers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed it. It was only used by tests.
NextDNS is unique in that users create accounts and then getuser-specific DNS IPs & DoH URLs.For DoH, the customer ID is in the URL path.For IPv6, the IP address includes the customer ID in the lower bits.For IPv4, there's a fragile "IP linking" mechanism to associate yourpublic IPv4 with an assigned NextDNS IPv4 and that tuple maps to yourcustomer ID.We don't use the IP linking mechanism.Instead, NextDNS is DoH-only. Which means using NextDNS necessarilyshunts all DNS traffic through 100.100.100.100 (programming the OS touse 100.100.100.100 as the global resolver) because operating systemscan't usually do DoH themselves.Once it's in Tailscale's DoH client, we then connect out to the knownNextDNS IPv4/IPv6 anycast addresses.If the control plane sends the client a NextDNS IPv6 address, we thenmap it to the corresponding NextDNS DoH with the same client ID, andwe dial that DoH server using the combination of v4/v6 anycast IPs.Updates#2452Change-Id: I3439d798d21d5fc9df5a2701839910f5bef85463Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Uh oh!
There was an error while loading.Please reload this page.
NextDNS is unique in that users create accounts and then get
user-specific DNS IPs & DoH URLs.
For DoH, the customer ID is in the URL path.
For IPv6, the IP address includes the customer ID in the lower bits.
For IPv4, there's a fragile "IP linking" mechanism to associate your
public IPv4 with an assigned NextDNS IPv4 and that tuple maps to your
customer ID.
We don't use the IP linking mechanism.
Instead, NextDNS is DoH-only. Which means using NextDNS necessarily
shunts all DNS traffic through 100.100.100.100 (programming the OS to
use 100.100.100.100 as the global resolver) because operating systems
can't usually do DoH themselves.
Once it's in Tailscale's DoH client, we then connect out to the known
NextDNS IPv4/IPv6 anycast addresses.
If the control plane sends the client a NextDNS IPv6 address, we then
map it to the corresponding NextDNS DoH with the same client ID, and
we dial that DoH server using the combination of v4/v6 anycast IPs.
Updates#2452