tailcfg, ipn/ipnlocal, net/dns: forward exit node DNS on Unix to system DNS

Updates #1713

Change-Id: I4c073fec0992d9e01a9a4ce97087d5af0efdc68d
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

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)
+		}
+	}
+
+}

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
+}

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
 	}

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
+// If resolvers is non-empty, it's used explicitly (notably, for exit
-// upstreams are available.
+// node DNS proxy queries), otherwise f.resolvers is used.
-func (f *forwarder) forwardWithDestChan(ctx context.Context, query packet, responseChan chan<- packet, backupResolvers ...resolverAndDelay) error {
+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
+		resolvers = f.resolvers(domain)
-	}
+		if len(resolvers) == 0 {
-	if len(resolvers) == 0 {
+			metricDNSFwdErrorNoUpstream.Add(1)
-		metricDNSFwdErrorNoUpstream.Add(1)
+			return errNoUpstreams
-		return errNoUpstreams
+		}
 	}
 
 	fq := &forwardQuery{

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)
+	resp := parseExitNodeQuery(q)
-	if err == errNoUpstreams {
+	if resp == nil {
-		// Handle to the system resolver.
+		return nil, errors.New("bad query")
-		switch runtime.GOOS {
+	}
-		case "linux":
+	name := resp.Question.Name.String()
-			// Assume for now that we don't have an upstream because
+	if !allowName(name) {
-			// they're using systemd-resolved and we're in Split DNS mode
+		metricDNSExitProxyErrorName.Add(1)
-			// where we don't know the base config.
+		resp.Header.RCode = dns.RCodeRefused
-			//
+		return marshalResponse(resp)
-			// 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.
+	switch runtime.GOOS {
-			err = r.forwarder.forwardWithDestChan(ctx, packet{q, from}, ch, resolverAndDelay{
+	default:
-				name: dnstype.Resolver{
+		return nil, errors.New("unsupported exit node OS")
-					Addr: "127.0.0.1:53",
+	case "windows":
-				},
+		// TODO: use DnsQueryEx and write to ch.
-			})
+		// See https://docs.microsoft.com/en-us/windows/win32/api/windns/nf-windns-dnsqueryex.
-		default:
+		return nil, errors.New("TODO: windows exit node suport")
-			// TODO(bradfitz): if we're on an exit node
+	case "darwin":
-			// on, say, Windows, we need to parse the DNS
+		// /etc/resolv.conf is a lie and only says one upstream DNS
-			// packet in q and call OS-native APIs for
+		// but for now that's probably good enough. Later we'll
-			// each question. But we'll want to strip out
+		// want to blend in everything from scutil --dns.
-			// questions for MagicDNS names probably, so
+		fallthrough
-			// they don't loop back into
+	case "linux", "freebsd", "openbsd", "illumos":
-			// 100.100.100.100. We don't want to resolve
+		nameserver, err := stubResolverForOS()
-			// MagicDNS names across Tailnets once we
+		if err != nil {
-			// permit sharing exit nodes.
+			r.logf("stubResolverForOS: %v", err)
-			//
+			metricDNSExitProxyErrorResolvConf.Add(1)
-			// For now, just return an error.
+			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")
+	metricDNSExitProxyQuery           = clientmetric.NewCounter("dns_exit_node_query")
-	metricDNSQueryForPeerError = clientmetric.NewCounter("dns_query_peerapi_error")
+	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")

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.

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
+	Resolvers           []dnstype.Resolver
-	Routes            map[string][]dnstype.Resolver
+	Routes              map[string][]dnstype.Resolver
-	FallbackResolvers []dnstype.Resolver
+	FallbackResolvers   []dnstype.Resolver
-	Domains           []string
+	Domains             []string
-	Proxied           bool
+	Proxied             bool
-	Nameservers       []netaddr.IP
+	Nameservers         []netaddr.IP
-	PerDomain         bool
+	PerDomain           bool
-	CertDomains       []string
+	CertDomains         []string
-	ExtraRecords      []DNSRecord
+	ExtraRecords        []DNSRecord
+	ExitNodeFilteredSet []string
 }{})
 
 // Clone makes a deep copy of RegisterResponse.