ipn: delete domainsForProxying, require explicit DNS search domains (mapver 9) (#1078)

Previously the client had heuristics to calculate which DNS search domains
to set, based on the peers' names. Unfortunately that prevented us from
doing some things we wanted to do server-side related to node sharing.

So, bump MapRequest.Version to 9 to signal that the client only uses the
explicitly configured DNS search domains and doesn't augment it with its own
list.

Updates tailscale/corp#1026

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

control/controlclient/direct.go

@@ -547,7 +547,7 @@ func (c *Direct) PollNetMap(ctx context.Context, maxPolls int, cb func(*NetworkM
 	}
 
 	request := tailcfg.MapRequest{
-		Version:    8,
+		Version:    tailcfg.CurrentMapRequestVersion,
 		KeepAlive:  c.keepAlive,
 		NodeKey:    tailcfg.NodeKey(persist.PrivateNodeKey.Public()),
 		DiscoKey:   c.discoPubKey,

ipn/local.go

@@ -639,7 +639,7 @@ func (b *LocalBackend) updateDNSMap(netMap *controlclient.NetworkMap) {
 	}
 	set(netMap.Name, netMap.Addresses)
 
-	dnsMap := tsdns.NewMap(nameToIP, domainsForProxying(netMap))
+	dnsMap := tsdns.NewMap(nameToIP, magicDNSRootDomains(netMap))
 	// map diff will be logged in tsdns.Resolver.SetMap.
 	b.e.SetDNSMap(dnsMap)
 }
@@ -1209,20 +1209,14 @@ func (b *LocalBackend) authReconfig() {
 
 	// If CorpDNS is false, rcfg.DNS remains the zero value.
 	if uc.CorpDNS {
-		domains := nm.DNS.Domains
 		proxied := nm.DNS.Proxied
-		if proxied {
-			if len(nm.DNS.Nameservers) == 0 {
+		if proxied && len(nm.DNS.Nameservers) == 0 {
 			b.logf("[unexpected] dns proxied but no nameservers")
 			proxied = false
-			} else {
-				// Domains for proxying should come first to avoid leaking queries.
-				domains = append(domainsForProxying(nm), domains...)
-			}
 		}
 		rcfg.DNS = dns.Config{
 			Nameservers: nm.DNS.Nameservers,
-			Domains:     domains,
+			Domains:     nm.DNS.Domains,
 			PerDomain:   nm.DNS.PerDomain,
 			Proxied:     proxied,
 		}
@@ -1235,32 +1229,31 @@ func (b *LocalBackend) authReconfig() {
 	b.logf("[v1] authReconfig: ra=%v dns=%v 0x%02x: %v", uc.RouteAll, uc.CorpDNS, flags, err)
 }
 
-// domainsForProxying produces a list of search domains for proxied DNS.
-func domainsForProxying(nm *controlclient.NetworkMap) []string {
-	var domains []string
-	if idx := strings.IndexByte(nm.Name, '.'); idx != -1 {
-		domains = append(domains, nm.Name[idx+1:])
+// magicDNSRootDomains returns the subset of nm.DNS.Domains that are the search domains for MagicDNS.
+// Each entry has a trailing period.
+func magicDNSRootDomains(nm *controlclient.NetworkMap) []string {
+	searchPathUsedAsDNSSuffix := func(suffix string) bool {
+		if tsdns.NameHasSuffix(nm.Name, suffix) {
+			return true
 		}
-	for _, peer := range nm.Peers {
-		idx := strings.IndexByte(peer.Name, '.')
-		if idx == -1 {
-			continue
-		}
-		domain := peer.Name[idx+1:]
-		seen := false
-		// In theory this makes the function O(n^2) worst case,
-		// but in practice we expect domains to contain very few elements
-		// (only one until invitations are introduced).
-		for _, seenDomain := range domains {
-			if domain == seenDomain {
-				seen = true
+		for _, p := range nm.Peers {
+			if tsdns.NameHasSuffix(p.Name, suffix) {
+				return true
 			}
 		}
-		if !seen {
-			domains = append(domains, domain)
+		return false
+	}
+
+	var ret []string
+	for _, d := range nm.DNS.Domains {
+		if searchPathUsedAsDNSSuffix(d) {
+			if !strings.HasSuffix(d, ".") {
+				d += "."
+			}
+			ret = append(ret, d)
 		}
 	}
-	return domains
+	return ret
 }
 
 // routerConfig produces a router.Config from a wireguard config and IPN prefs.

tailcfg/tailcfg.go

@@ -22,6 +22,18 @@
 	"tailscale.com/types/structs"
 )
 
+// CurrentMapRequestVersion is the current MapRequest.Version value.
+//
+// History of versions:
+//     3: implicit compression, keep-alives
+//     4: opt-in keep-alives via KeepAlive field, opt-in compression via Compress
+//     5: 2020-10-19, implies IncludeIPv6, delta Peers/UserProfiles, supports MagicDNS
+//     6: 2020-12-07: means MapResponse.PacketFilter nil means unchanged
+//     7: 2020-12-15: FilterRule.SrcIPs accepts CIDRs+ranges, doesn't warn about 0.0.0.0/::
+//     8: 2020-12-19: client can receive IPv6 addresses and routes if beta enabled server-side
+//     9: 2020-12-30: client doesn't auto-add implicit search domains from peers; only DNSConfig.Domains
+const CurrentMapRequestVersion = 9
+
 type ID int64
 
 type UserID ID
@@ -471,14 +483,9 @@ type MapRequest struct {
 	// we want to signal to the control server that we're capable of something
 	// different.
 	//
-	// History of versions:
-	//     3: implicit compression, keep-alives
-	//     4: opt-in keep-alives via KeepAlive field, opt-in compression via Compress
-	//     5: 2020-10-19, implies IncludeIPv6, delta Peers/UserProfiles, supports MagicDNS
-	//     6: 2020-12-07: means MapResponse.PacketFilter nil means unchanged
-	//     7: 2020-12-15: FilterRule.SrcIPs accepts CIDRs+ranges, doesn't warn about 0.0.0.0/::
-	//     8: 2020-12-19: client can receive IPv6 addresses and routes if beta enabled server-side
+	// For current values and history, see CurrentMapRequestVersion above.
 	Version int
+
 	Compress    string // "zstd" or "" (no compression)
 	KeepAlive   bool   // whether server should send keep-alives back to us
 	NodeKey     NodeKey
@@ -627,10 +634,19 @@ type MapResponse struct {
 	//
 	// TODO(dmytro): should be sent in DNSConfig.Nameservers once clients have updated.
 	DNS []netaddr.IP `json:",omitempty"`
-	// SearchPaths are the same as DNSConfig.Domains.
-	//
-	// TODO(dmytro): should be sent in DNSConfig.Domains once clients have updated.
+
+	// SearchPaths is the old way to specify DNS search
+	// domains. Clients should use these values if set, but the
+	// server will omit this field for clients with
+	// MapRequest.Version >= 9. Clients should prefer to use
+	// DNSConfig.Domains instead.
 	SearchPaths []string `json:",omitempty"`
+
+	// DNSConfig contains the DNS settings for the client to use.
+	//
+	// TODO(bradfitz): make this a pointer and conditionally sent
+	// only if changed, like DERPMap, PacketFilter, etc. It's
+	// small, though.
 	DNSConfig DNSConfig `json:",omitempty"`
 
 	// Domain is the name of the network that this node is

wgengine/tsdns/map.go

@@ -26,6 +26,10 @@ type Map struct {
 }
 
 // NewMap returns a new Map with name to address mapping given by nameToIP.
+//
+// rootDomains are the domains whose subdomains should always be
+// resolved locally to prevent leakage of sensitive names. They should
+// end in a period ("user-foo.tailscale.net.").
 func NewMap(initNameToIP map[string]netaddr.IP, rootDomains []string) *Map {
 	// TODO(dmytro): we have to allocate names and ipToName, but nameToIP can be avoided.
 	// It is here because control sends us names not in canonical form. Change this.

wgengine/tsdns/tsdns.go

@@ -194,8 +194,8 @@ func (r *Resolver) Resolve(domain string, tp dns.Type) (netaddr.IP, dns.RCode, e
 	}
 
 	anyHasSuffix := false
-	for _, rootDomain := range dnsMap.rootDomains {
-		if strings.HasSuffix(domain, rootDomain) {
+	for _, suffix := range dnsMap.rootDomains {
+		if NameHasSuffix(domain, suffix) {
 			anyHasSuffix = true
 			break
 		}
@@ -611,3 +611,12 @@ func (r *Resolver) respond(query []byte) ([]byte, error) {
 
 	return marshalResponse(resp)
 }
+
+// NameHasSuffix reports whether the provided DNS name ends with the
+// component(s) in suffix, ignoring any trailing dots.
+func NameHasSuffix(name, suffix string) bool {
+	name = strings.TrimSuffix(name, ".")
+	suffix = strings.TrimSuffix(suffix, ".")
+	nameBase := strings.TrimSuffix(name, suffix)
+	return len(nameBase) < len(name) && strings.HasSuffix(nameBase, ".")
+}

wgengine/tsdns/tsdns_test.go

@@ -758,3 +758,24 @@ func TestMarshalResponseFormatError(t *testing.T) {
 	}
 	t.Logf("response: %q", v)
 }
+
+func TestNameHasSuffix(t *testing.T) {
+	tests := []struct {
+		name, suffix string
+		want         bool
+	}{
+		{"foo.com", "com", true},
+		{"foo.com.", "com", true},
+		{"foo.com.", "com.", true},
+
+		{"", "", false},
+		{"foo.com.", "", false},
+		{"foo.com.", "o.com", false},
+	}
+	for _, tt := range tests {
+		got := NameHasSuffix(tt.name, tt.suffix)
+		if got != tt.want {
+			t.Errorf("NameHasSuffix(%q, %q) = %v; want %v", tt.name, tt.suffix, got, tt.want)
+		}
+	}
+}