net/dns/resolver: upgrade forwarded MagicDNS queries to DoH when IP known#2435
net/dns/resolver: upgrade forwarded MagicDNS queries to DoH when IP known#2435
Conversation
crawshaw
left a comment
crawshaw
left a comment
There was a problem hiding this comment.
Is adding DoH targets via the control plane future work?
bradfitz
force-pushed
the
bradfitz/doh
branch
2 times, most recently
from
cbe8e4a to
d3e62df
Compare
Yeah. That requires more plumbing. I linked a bunch of those bugs in the commit message now. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", dohType) |
There was a problem hiding this comment.
I think setting both Accept and Content-Type would be good
There was a problem hiding this comment.
I see it included in examples (https://datatracker.ietf.org/doc/html/rfc8484#page-5) but looks like the Accept one is pretty optional, though:
The DoH client SHOULD include an HTTP Accept request header field to
indicate what type of content can be understood in response.
Irrespective of the value of the Accept request header field, the
client MUST be prepared to process "application/dns-message"
(and empirically isn't required by any provider)
Worth the bandwidth? With HTTP/2 it's not much, but iOS doesn't use HTTP/2 yet so it's a bit more.
There was a problem hiding this comment.
Updated code with comment:
diff --git a/net/dns/resolver/forwarder.go b/net/dns/resolver/forwarder.go
index 8a15c099..a3184f8c 100644
--- a/net/dns/resolver/forwarder.go
+++ b/net/dns/resolver/forwarder.go
@@ -259,6 +259,12 @@ func (f *forwarder) sendDoH(ctx context.Context, urlBase string, c *http.Client,
return nil, err
}
req.Header.Set("Content-Type", dohType)
+ // Note: we don't currently set the Accept header (which is
+ // only a SHOULD in the spec) as iOS doesn't use HTTP/2 and
+ // we'd rather save a few bytes on outgoing requests when
+ // empirically no provider cares about the Accept header's
+ // absence.
+
hres, err := c.Do(req)
if err != nil {
return nil, err
…nown Recognize Cloudflare, Google, Quad9 which are by far the majority of upstream DNS servers that people use. RELNOTE=MagicDNS now uses DNS-over-HTTPS when querying popular upstream resolvers, so DNS queries aren't sent in the clear over the Internet. Updates #915 (might fix it?) Updates #988 (gets us closer, if it fixes Android) Updates #74 (not yet configurable, but progress) Updates #2056 (not yet configurable, dup of #74?) Signed-off-by: Brad Fitzpatrick <[email protected]>
The whole cache part of the spec is super weird. There aren't going to be rando cache intermediates here and Google et al aren't using rando off-the-shelf HTTP caches in front of their DNS servers. Any caching they're doing is part of the application layer after it's parsed the HTTP framing. |
|
Is it possible to support nextdns.io? Some of the power users have custom blocklists and adblock lists. DNS-Over-HTTPS with magicDNS and nextDNS nameservers would be a great addition. |
|
Could you file that as a feature request in https://github.com/tailscale/tailscale/issues ? |
5 participants
Recognize Cloudflare, Google, Quad9 which are by far the
majority of upstream DNS servers that people use.
RELNOTE=MagicDNS now uses DNS-over-HTTPS when querying popular upstream resolvers,
so DNS queries aren't sent in the clear over the Internet.
Updates #915 (might fix it?)
Updates #988 (gets us closer, if it fixes Android)
Updates #74 (not yet configurable, but progress)
Updates #2056 (not yet configurable, dup of #74?)