net/dns/{publicdns,resolver}: add NextDNS DoH support by bradfitz · Pull Request #5556 · tailscale/tailscale

bradfitz · 2022-09-06T20:23:58Z

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

crawshaw

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.

crawshaw · 2022-09-07T04:14:10Z

net/dns/config.go


 // 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


I have spent a minute trying to decide if that should be an and or an or. I don't know. Maybe this function needs a different name? tailscaleOwnsDNS?

From the code, it seems to be 'and'.

net/dns/publicdns/publicdns.go

tailcfg/tailcfg.go

net/dns/publicdns/publicdns.go

maisem

I assume you've tested that this works?

bradfitz · 2022-09-07T04:59:18Z

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 · 2022-09-08T04:45:22Z

And added more tests.

danderson

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.

danderson · 2022-09-08T16:48:16Z

net/dns/config.go


 // 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


From the code, it seems to be 'and'.

net/dns/publicdns/publicdns.go

danderson · 2022-09-08T16:52:34Z

net/dns/publicdns/publicdns.go

+}
+
+// DoHIPsOfBase returns a DoH base URL's IP addresses.
+// It is basically the inverse of DoHEndpointFromIP.


Is it basically the inverse, or exactly the inverse? I think you can drop "basically".

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.

Updated comment.

danderson · 2022-09-08T16:55:53Z

net/dns/publicdns/publicdns.go

+	if b, ok := m[ip]; ok {
+		return b, false, true
+	}
+	if nextDNSv6RangeA.Contains(ip) || nextDNSv6RangeB.Contains(ip) {


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?

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.

Updated comment.

danderson · 2022-09-08T16:57:05Z

net/dns/publicdns/publicdns_test.go

+		{
+			base: "https://dns.nextdns.io/c3a884",
+			want: ips(
+				"45.90.28.0",


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 ?

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.

Updated the comment on the func

danderson · 2022-09-08T16:59:05Z

net/dns/publicdns/publicdns.go

@@ -22,11 +29,63 @@ func KnownDoH() map[netip.Addr]string {
 	return knownDoH


I can't comment any higher, but KnownDOH is now broken, it doesn't return all known DoH. How does this affect callers?

Removed it. It was only used by tests.

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 Change-Id: I3439d798d21d5fc9df5a2701839910f5bef85463 Signed-off-by: Brad Fitzpatrick <[email protected]>

bradfitz force-pushed the bradfitz/nextdns branch 3 times, most recently from dee6825 to b6d6cbd Compare September 7, 2022 03:52

bradfitz marked this pull request as ready for review September 7, 2022 03:55

bradfitz requested review from crawshaw, danderson and maisem September 7, 2022 03:55

crawshaw approved these changes Sep 7, 2022

View reviewed changes

maisem reviewed Sep 7, 2022

View reviewed changes

net/dns/publicdns/publicdns.go Show resolved Hide resolved

maisem approved these changes Sep 7, 2022

View reviewed changes

bradfitz force-pushed the bradfitz/nextdns branch from b6d6cbd to c0039fb Compare September 8, 2022 04:43

bradfitz force-pushed the bradfitz/nextdns branch 2 times, most recently from 41aa508 to e49c102 Compare September 8, 2022 05:04

danderson approved these changes Sep 8, 2022

View reviewed changes

bradfitz force-pushed the bradfitz/nextdns branch from e49c102 to e487bc0 Compare September 8, 2022 19:41

bradfitz merged commit 58abae1 into main Sep 8, 2022

bradfitz deleted the bradfitz/nextdns branch September 8, 2022 19:50

alexelisenko mentioned this pull request Jun 23, 2023

Add Control D support, along with DoH client metadata #8417

Closed

		@@ -22,11 +29,63 @@ func KnownDoH() map[netip.Addr]string {
		return knownDoH

Conversation

bradfitz commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crawshaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maisem left a comment

Choose a reason for hiding this comment

Uh oh!

bradfitz commented Sep 7, 2022

Uh oh!

bradfitz commented Sep 8, 2022

Uh oh!

danderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bradfitz commented Sep 6, 2022 •

edited

Loading