From 135580a5a8f26bcbdc4c3beaaafd12f146a3b8a9 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 29 Nov 2021 14:18:09 -0800 Subject: [PATCH] tailcfg, ipn/ipnlocal, net/dns: forward exit node DNS on Unix to system DNS Updates #1713 Change-Id: I4c073fec0992d9e01a9a4ce97087d5af0efdc68d Signed-off-by: Brad Fitzpatrick --- ipn/ipnlocal/dnsconfig_test.go | 45 +++++++++ ipn/ipnlocal/local.go | 31 ++++++ ipn/ipnlocal/peerapi.go | 9 +- net/dns/resolver/forwarder.go | 17 ++-- net/dns/resolver/tsdns.go | 171 ++++++++++++++++++++++++++------- tailcfg/tailcfg.go | 15 +++ tailcfg/tailcfg_clone.go | 20 ++-- 7 files changed, 251 insertions(+), 57 deletions(-) diff --git a/ipn/ipnlocal/dnsconfig_test.go b/ipn/ipnlocal/dnsconfig_test.go index a4ec6060c..abbadd1dc 100644 --- a/ipn/ipnlocal/dnsconfig_test.go +++ b/ipn/ipnlocal/dnsconfig_test.go @@ -344,3 +344,48 @@ func TestDNSConfigForNetmap(t *testing.T) { }) } } + +func TestAllowExitNodeDNSProxyToServeName(t *testing.T) { + b := &LocalBackend{} + if b.allowExitNodeDNSProxyToServeName("google.com") { + t.Fatal("unexpected true on backend with nil NetMap") + } + + b.netMap = &netmap.NetworkMap{ + DNS: tailcfg.DNSConfig{ + ExitNodeFilteredSet: []string{ + ".ts.net", + "some.exact.bad", + }, + }, + } + tests := []struct { + name string + want bool + }{ + // Allow by default: + {"google.com", true}, + {"GOOGLE.com", true}, + + // Rejected by suffix: + {"foo.TS.NET", false}, + {"foo.ts.net", false}, + + // Suffix doesn't match + {"ts.net", true}, + + // Rejected by exact match: + {"some.exact.bad", false}, + {"SOME.EXACT.BAD", false}, + + // But a prefix is okay. + {"prefix-okay.some.exact.bad", true}, + } + for _, tt := range tests { + got := b.allowExitNodeDNSProxyToServeName(tt.name) + if got != tt.want { + t.Errorf("for %q = %v; want %v", tt.name, got, tt.want) + } + } + +} diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d1fd826b7..0cefb70d7 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3053,3 +3053,34 @@ func (b *LocalBackend) OfferingExitNode() bool { } return def4 && def6 } + +// allowExitNodeDNSProxyToServeName reports whether the Exit Node DNS +// proxy is allowed to serve responses for the provided DNS name. +func (b *LocalBackend) allowExitNodeDNSProxyToServeName(name string) bool { + b.mu.Lock() + defer b.mu.Unlock() + nm := b.netMap + if nm == nil { + return false + } + name = strings.ToLower(name) + for _, bad := range nm.DNS.ExitNodeFilteredSet { + if bad == "" { + // Invalid, ignore. + continue + } + if bad[0] == '.' { + // Entries beginning with a dot are suffix matches. + if dnsname.HasSuffix(name, bad) { + return false + } + continue + } + // Otherwise entries are exact matches. They're + // guaranteed to be lowercase already. + if name == bad { + return false + } + } + return true +} diff --git a/ipn/ipnlocal/peerapi.go b/ipn/ipnlocal/peerapi.go index 2706872d4..906a3a822 100644 --- a/ipn/ipnlocal/peerapi.go +++ b/ipn/ipnlocal/peerapi.go @@ -832,7 +832,7 @@ func (h *peerAPIHandler) handleDNSQuery(w http.ResponseWriter, r *http.Request) ctx, cancel := context.WithTimeout(r.Context(), arbitraryTimeout) defer cancel() - res, err := h.ps.resolver.HandleExitNodeDNSQuery(ctx, q, h.remoteAddr) + res, err := h.ps.resolver.HandleExitNodeDNSQuery(ctx, q, h.remoteAddr, h.ps.b.allowExitNodeDNSProxyToServeName) if err != nil { h.logf("handleDNS fwd error: %v", err) if err := ctx.Err(); err != nil { @@ -918,14 +918,19 @@ func writePrettyDNSReply(w io.Writer, res []byte) (err error) { j, _ := json.Marshal(struct { Error string }{err.Error()}) + j = append(j, '\n') w.Write(j) return } }() var p dnsmessage.Parser - if _, err := p.Start(res); err != nil { + hdr, err := p.Start(res) + if err != nil { return err } + if hdr.RCode != dnsmessage.RCodeSuccess { + return fmt.Errorf("DNS RCode = %v", hdr.RCode) + } if err := p.SkipAllQuestions(); err != nil { return err } diff --git a/net/dns/resolver/forwarder.go b/net/dns/resolver/forwarder.go index 7776fc5da..0e8cbca9a 100644 --- a/net/dns/resolver/forwarder.go +++ b/net/dns/resolver/forwarder.go @@ -580,9 +580,9 @@ func (f *forwarder) forward(query packet) error { // It either sends to responseChan and returns nil, or returns a // non-nil error (without sending to the channel). // -// If backupResolvers are specified, they're used in the case that no -// upstreams are available. -func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, responseChan chan<- packet, backupResolvers ...resolverAndDelay) error { +// If resolvers is non-empty, it's used explicitly (notably, for exit +// node DNS proxy queries), otherwise f.resolvers is used. +func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, responseChan chan<- packet, resolvers ...resolverAndDelay) error { metricDNSFwd.Add(1) domain, err := nameFromQuery(query.bs) if err != nil { @@ -601,13 +601,12 @@ func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, respo clampEDNSSize(query.bs, maxResponseBytes) - resolvers := f.resolvers(domain) if len(resolvers) == 0 { - resolvers = backupResolvers - } - if len(resolvers) == 0 { - metricDNSFwdErrorNoUpstream.Add(1) - return errNoUpstreams + resolvers = f.resolvers(domain) + if len(resolvers) == 0 { + metricDNSFwdErrorNoUpstream.Add(1) + return errNoUpstreams + } } fq := &forwardQuery{ diff --git a/net/dns/resolver/tsdns.go b/net/dns/resolver/tsdns.go index ee67995e4..7a1ed6075 100644 --- a/net/dns/resolver/tsdns.go +++ b/net/dns/resolver/tsdns.go @@ -8,11 +8,14 @@ package resolver import ( "bufio" + "bytes" "context" "encoding/hex" "errors" "fmt" "io" + "net" + "os" "runtime" "sort" "strings" @@ -20,12 +23,15 @@ import ( "sync/atomic" "time" + "go4.org/mem" dns "golang.org/x/net/dns/dnsmessage" "inet.af/netaddr" + "tailscale.com/net/tsaddr" "tailscale.com/types/dnstype" "tailscale.com/types/logger" "tailscale.com/util/clientmetric" "tailscale.com/util/dnsname" + "tailscale.com/util/lineread" "tailscale.com/wgengine/monitor" ) @@ -303,48 +309,83 @@ func (r *Resolver) NextResponse() (packet []byte, to netaddr.IPPort, err error) } } +// parseExitNodeQuery parses a DNS request packet. +// It returns nil if it's malformed or lacking a question. +func parseExitNodeQuery(q []byte) *response { + p := dnsParserPool.Get().(*dnsParser) + defer dnsParserPool.Put(p) + p.zeroParser() + defer p.zeroParser() + if err := p.parseQuery(q); err != nil { + return nil + } + return p.response() +} + // HandleExitNodeDNSQuery handles a DNS query that arrived from a peer // via the peerapi's DoH server. This is only used when the local // node is being an exit node. -func (r *Resolver) HandleExitNodeDNSQuery(ctx context.Context, q []byte, from netaddr.IPPort) (res []byte, err error) { - metricDNSQueryForPeer.Add(1) +// +// The provided allowName callback is whether a DNS query for a name +// (as found by parsing q) is allowed. +// +// In most (all?) cases, err will be nil. A bogus DNS query q will +// still result in a response DNS packet (saying there's a failure) +// and a nil error. +// TODO: figure out if we even need an error result. +func (r *Resolver) HandleExitNodeDNSQuery(ctx context.Context, q []byte, from netaddr.IPPort, allowName func(name string) bool) (res []byte, err error) { + metricDNSExitProxyQuery.Add(1) ch := make(chan packet, 1) - err = r.forwarder.forwardWithDestChan(ctx, packet{q, from}, ch) - if err == errNoUpstreams { - // Handle to the system resolver. - switch runtime.GOOS { - case "linux": - // Assume for now that we don't have an upstream because - // they're using systemd-resolved and we're in Split DNS mode - // where we don't know the base config. - // - // TODO(bradfitz): this is a lazy assumption. Do better, and - // maybe move the HandleExitNodeDNSQuery method to the dns.Manager - // instead? But this works for now. - err = r.forwarder.forwardWithDestChan(ctx, packet{q, from}, ch, resolverAndDelay{ - name: dnstype.Resolver{ - Addr: "127.0.0.1:53", - }, - }) - default: - // TODO(bradfitz): if we're on an exit node - // on, say, Windows, we need to parse the DNS - // packet in q and call OS-native APIs for - // each question. But we'll want to strip out - // questions for MagicDNS names probably, so - // they don't loop back into - // 100.100.100.100. We don't want to resolve - // MagicDNS names across Tailnets once we - // permit sharing exit nodes. - // - // For now, just return an error. + resp := parseExitNodeQuery(q) + if resp == nil { + return nil, errors.New("bad query") + } + name := resp.Question.Name.String() + if !allowName(name) { + metricDNSExitProxyErrorName.Add(1) + resp.Header.RCode = dns.RCodeRefused + return marshalResponse(resp) + } + + switch runtime.GOOS { + default: + return nil, errors.New("unsupported exit node OS") + case "windows": + // TODO: use DnsQueryEx and write to ch. + // See https://docs.microsoft.com/en-us/windows/win32/api/windns/nf-windns-dnsqueryex. + return nil, errors.New("TODO: windows exit node suport") + case "darwin": + // /etc/resolv.conf is a lie and only says one upstream DNS + // but for now that's probably good enough. Later we'll + // want to blend in everything from scutil --dns. + fallthrough + case "linux", "freebsd", "openbsd", "illumos": + nameserver, err := stubResolverForOS() + if err != nil { + r.logf("stubResolverForOS: %v", err) + metricDNSExitProxyErrorResolvConf.Add(1) + return nil, err + } + // TODO: more than 1 resolver from /etc/resolv.conf? + + var resolvers []resolverAndDelay + if nameserver == tsaddr.TailscaleServiceIP() { + // If resolv.conf says 100.100.100.100, it's coming right back to us anyway + // so avoid the loop through the kernel and just do what we + // would've done anyway. By not passing any resolvers, the forwarder + // will use its default ones from our DNS config. + } else { + resolvers = []resolverAndDelay{{ + name: dnstype.Resolver{Addr: net.JoinHostPort(nameserver.String(), "53")}, + }} + } + + err = r.forwarder.forwardWithDestChan(ctx, packet{q, from}, ch, resolvers...) + if err != nil { + metricDNSExitProxyErrorForward.Add(1) return nil, err } - } - if err != nil { - metricDNSQueryForPeerError.Add(1) - return nil, err } select { case p, ok := <-ch: @@ -357,6 +398,59 @@ func (r *Resolver) HandleExitNodeDNSQuery(ctx context.Context, q []byte, from ne } } +type resolvConfCache struct { + mod time.Time + size int64 + ip netaddr.IP + // TODO: inode/dev? +} + +// resolvConfCacheValue contains the most recent stat metadata and parsed +// version of /etc/resolv.conf. +var resolvConfCacheValue atomic.Value // of resolvConfCache + +var errEmptyResolvConf = errors.New("resolv.conf has no nameservers") + +// stubResolverForOS returns the IP address of the first nameserver in +// /etc/resolv.conf. +func stubResolverForOS() (ip netaddr.IP, err error) { + fi, err := os.Stat("/etc/resolv.conf") + if err != nil { + return netaddr.IP{}, err + } + cur := resolvConfCache{ + mod: fi.ModTime(), + size: fi.Size(), + } + if c, ok := resolvConfCacheValue.Load().(resolvConfCache); ok && c.mod == cur.mod && c.size == cur.size { + return c.ip, nil + } + err = lineread.File("/etc/resolv.conf", func(line []byte) error { + if !ip.IsZero() { + return nil + } + line = bytes.TrimSpace(line) + if len(line) == 0 || line[0] == '#' { + return nil + } + if mem.HasPrefix(mem.B(line), mem.S("nameserver ")) { + s := strings.TrimSpace(strings.TrimPrefix(string(line), "nameserver ")) + ip, err = netaddr.ParseIP(s) + return err + } + return nil + }) + if err != nil { + return netaddr.IP{}, err + } + if !ip.IsValid() { + return netaddr.IP{}, errEmptyResolvConf + } + cur.ip = ip + resolvConfCacheValue.Store(cur) + return ip, nil +} + // resolveLocal returns an IP for the given domain, if domain is in // the local hosts map and has an IP corresponding to the requested // typ (A, AAAA, ALL). @@ -538,6 +632,7 @@ func (p *dnsParser) zeroParser() { p.parser = dns.Parser{} } // p.Question. func (p *dnsParser) parseQuery(query []byte) error { defer p.zeroParser() + p.zeroParser() var err error p.Header, err = p.parser.Start(query) if err != nil { @@ -837,8 +932,10 @@ var ( metricDNSMagicDNSSuccessName = clientmetric.NewCounter("dns_query_magic_success_name") metricDNSMagicDNSSuccessReverse = clientmetric.NewCounter("dns_query_magic_success_reverse") - metricDNSQueryForPeer = clientmetric.NewCounter("dns_query_peerapi") - metricDNSQueryForPeerError = clientmetric.NewCounter("dns_query_peerapi_error") + metricDNSExitProxyQuery = clientmetric.NewCounter("dns_exit_node_query") + metricDNSExitProxyErrorName = clientmetric.NewCounter("dns_exit_node_error_name") + metricDNSExitProxyErrorForward = clientmetric.NewCounter("dns_exit_node_error_forward") + metricDNSExitProxyErrorResolvConf = clientmetric.NewCounter("dns_exit_node_error_resolvconf") metricDNSFwd = clientmetric.NewCounter("dns_query_fwd") metricDNSFwdDropBonjour = clientmetric.NewCounter("dns_query_fwd_drop_bonjour") diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 8a6dfc748..3dc0dbc1c 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -907,6 +907,21 @@ type DNSConfig struct { // ExtraRecords contains extra DNS records to add to the // MagicDNS config. ExtraRecords []DNSRecord `json:",omitempty"` + + // ExitNodeFilteredSuffixes are the the DNS suffixes that the + // node, when being an exit node DNS proxy, should not answer. + // + // The entries do not contain trailing periods and are always + // all lowercase. + // + // If an entry starts with a period, it's a suffix match (but + // suffix ".a.b" doesn't match "a.b"; a prefix is required). + // + // If an entry does not start with a period, it's an exact + // match. + // + // Matches are case insensitive. + ExitNodeFilteredSet []string } // DNSRecord is an extra DNS record to add to MagicDNS. diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index c3e1f0bea..2f162bf53 100644 --- a/tailcfg/tailcfg_clone.go +++ b/tailcfg/tailcfg_clone.go @@ -208,20 +208,22 @@ func (src *DNSConfig) Clone() *DNSConfig { dst.Nameservers = append(src.Nameservers[:0:0], src.Nameservers...) dst.CertDomains = append(src.CertDomains[:0:0], src.CertDomains...) dst.ExtraRecords = append(src.ExtraRecords[:0:0], src.ExtraRecords...) + dst.ExitNodeFilteredSet = append(src.ExitNodeFilteredSet[:0:0], src.ExitNodeFilteredSet...) return dst } // A compilation failure here means this code must be regenerated, with the command at the top of this file. var _DNSConfigCloneNeedsRegeneration = DNSConfig(struct { - Resolvers []dnstype.Resolver - Routes map[string][]dnstype.Resolver - FallbackResolvers []dnstype.Resolver - Domains []string - Proxied bool - Nameservers []netaddr.IP - PerDomain bool - CertDomains []string - ExtraRecords []DNSRecord + Resolvers []dnstype.Resolver + Routes map[string][]dnstype.Resolver + FallbackResolvers []dnstype.Resolver + Domains []string + Proxied bool + Nameservers []netaddr.IP + PerDomain bool + CertDomains []string + ExtraRecords []DNSRecord + ExitNodeFilteredSet []string }{}) // Clone makes a deep copy of RegisterResponse.