From 5c2b2fa1f8c4d0a75b24f7cb79e71ed87beb832a Mon Sep 17 00:00:00 2001 From: James Tucker Date: Wed, 20 Sep 2023 13:07:48 -0700 Subject: [PATCH] ipn/ipnlocal: plumb ExitNodeDNSResolvers for IsWireGuardOnly exit nodes Control sends ExitNodeDNSResolvers when configured for IsWireGuardOnly nodes that are to be used as the default resolver with a lower precedence than split DNS, and a lower precedence than "Override local DNS", but otherwise before local DNS is used when the exit node is in use. Neither of the below changes were problematic, but appeared so alongside a number of other client and external changes. See tailscale/corp#14809. Reland ea9dd8fabc64d7f4d054c2c45743728129f1430b. Reland d52ab181c3c55354fb47a224a194d0d2c080301b. Updates #9377 Updates tailscale/corp#14809 Signed-off-by: James Tucker --- ipn/ipnlocal/local.go | 49 +++++-- ipn/ipnlocal/local_test.go | 258 +++++++++++++++++++++++++++++++++++++ tailcfg/tailcfg.go | 3 +- 3 files changed, 301 insertions(+), 9 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d7fad76dd..cab5002a1 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3339,9 +3339,7 @@ func dnsConfigForNetmap(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg. } addDefault := func(resolvers []*dnstype.Resolver) { - for _, r := range resolvers { - dcfg.DefaultResolvers = append(dcfg.DefaultResolvers, r) - } + dcfg.DefaultResolvers = append(dcfg.DefaultResolvers, resolvers...) } // If we're using an exit node and that exit node is new enough (1.19.x+) @@ -3351,7 +3349,17 @@ func dnsConfigForNetmap(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg. return dcfg } - addDefault(nm.DNS.Resolvers) + // If the user has set default resolvers ("override local DNS"), prefer to + // use those resolvers as the default, otherwise if there are WireGuard exit + // node resolvers, use those as the default. + if len(nm.DNS.Resolvers) > 0 { + addDefault(nm.DNS.Resolvers) + } else { + if resolvers, ok := wireguardExitNodeDNSResolvers(nm, peers, prefs.ExitNodeID()); ok { + addDefault(resolvers) + } + } + for suffix, resolvers := range nm.DNS.Routes { fqdn, err := dnsname.ToFQDN(suffix) if err != nil { @@ -3366,11 +3374,10 @@ func dnsConfigForNetmap(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg. // // While we're already populating it, might as well size the // slice appropriately. + // Per #9498 the exact requirements of nil vs empty slice remain + // unclear, this is a haunted graveyard to be resolved. dcfg.Routes[fqdn] = make([]*dnstype.Resolver, 0, len(resolvers)) - - for _, r := range resolvers { - dcfg.Routes[fqdn] = append(dcfg.Routes[fqdn], r) - } + dcfg.Routes[fqdn] = append(dcfg.Routes[fqdn], resolvers...) } // Set FallbackResolvers as the default resolvers in the @@ -4768,6 +4775,32 @@ func exitNodeCanProxyDNS(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg return "", false } +// wireguardExitNodeDNSResolvers returns the DNS resolvers to use for a +// WireGuard-only exit node, if it has resolver addresses. +func wireguardExitNodeDNSResolvers(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg.NodeView, exitNodeID tailcfg.StableNodeID) ([]*dnstype.Resolver, bool) { + if exitNodeID.IsZero() { + return nil, false + } + + for _, p := range peers { + if p.StableID() == exitNodeID { + if p.IsWireGuardOnly() { + resolvers := p.ExitNodeDNSResolvers() + if !resolvers.IsNil() && resolvers.Len() > 0 { + copies := make([]*dnstype.Resolver, resolvers.Len()) + for i := range resolvers.LenIter() { + copies[i] = resolvers.At(i).AsStruct() + } + return copies, true + } + } + return nil, false + } + } + + return nil, false +} + func peerCanProxyDNS(p tailcfg.NodeView) bool { if p.Cap() >= 26 { // Actually added at 25 diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 757f2254b..2bb037a30 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -22,11 +22,13 @@ "tailscale.com/tailcfg" "tailscale.com/tsd" "tailscale.com/tstest" + "tailscale.com/types/dnstype" "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/logid" "tailscale.com/types/netmap" "tailscale.com/types/ptr" + "tailscale.com/util/dnsname" "tailscale.com/util/set" "tailscale.com/wgengine" "tailscale.com/wgengine/filter" @@ -899,3 +901,259 @@ func TestWhoIs(t *testing.T) { }) } } + +func TestWireguardExitNodeDNSResolvers(t *testing.T) { + type tc struct { + name string + id tailcfg.StableNodeID + peers []*tailcfg.Node + wantOK bool + wantResolvers []*dnstype.Resolver + } + + tests := []tc{ + { + name: "no peers", + id: "1", + wantOK: false, + wantResolvers: nil, + }, + { + name: "non wireguard peer", + id: "1", + peers: []*tailcfg.Node{ + { + ID: 1, + StableID: "1", + IsWireGuardOnly: false, + ExitNodeDNSResolvers: []*dnstype.Resolver{{Addr: "dns.example.com"}}, + }, + }, + wantOK: false, + wantResolvers: nil, + }, + { + name: "no matching IDs", + id: "2", + peers: []*tailcfg.Node{ + { + ID: 1, + StableID: "1", + IsWireGuardOnly: true, + ExitNodeDNSResolvers: []*dnstype.Resolver{{Addr: "dns.example.com"}}, + }, + }, + wantOK: false, + wantResolvers: nil, + }, + { + name: "wireguard peer", + id: "1", + peers: []*tailcfg.Node{ + { + ID: 1, + StableID: "1", + IsWireGuardOnly: true, + ExitNodeDNSResolvers: []*dnstype.Resolver{{Addr: "dns.example.com"}}, + }, + }, + wantOK: true, + wantResolvers: []*dnstype.Resolver{{Addr: "dns.example.com"}}, + }, + } + + for _, tc := range tests { + peers := peersMap(nodeViews(tc.peers)) + nm := &netmap.NetworkMap{} + gotResolvers, gotOK := wireguardExitNodeDNSResolvers(nm, peers, tc.id) + + if gotOK != tc.wantOK || !resolversEqual(t, gotResolvers, tc.wantResolvers) { + t.Errorf("case: %s: got %v, %v, want %v, %v", tc.name, gotOK, gotResolvers, tc.wantOK, tc.wantResolvers) + } + } +} + +func TestDNSConfigForNetmapForExitNodeConfigs(t *testing.T) { + type tc struct { + name string + exitNode tailcfg.StableNodeID + peers []tailcfg.NodeView + dnsConfig *tailcfg.DNSConfig + wantDefaultResolvers []*dnstype.Resolver + wantRoutes map[dnsname.FQDN][]*dnstype.Resolver + } + + defaultResolvers := []*dnstype.Resolver{{Addr: "default.example.com"}} + wgResolvers := []*dnstype.Resolver{{Addr: "wg.example.com"}} + peers := []tailcfg.NodeView{ + (&tailcfg.Node{ + ID: 1, + StableID: "wg", + IsWireGuardOnly: true, + ExitNodeDNSResolvers: wgResolvers, + Hostinfo: (&tailcfg.Hostinfo{}).View(), + }).View(), + // regular tailscale exit node with DNS capabilities + (&tailcfg.Node{ + Cap: 26, + ID: 2, + StableID: "ts", + Hostinfo: (&tailcfg.Hostinfo{}).View(), + }).View(), + } + exitDOH := peerAPIBase(&netmap.NetworkMap{Peers: peers}, peers[0]) + "/dns-query" + routes := map[dnsname.FQDN][]*dnstype.Resolver{ + "route.example.com.": {{Addr: "route.example.com"}}, + } + stringifyRoutes := func(routes map[dnsname.FQDN][]*dnstype.Resolver) map[string][]*dnstype.Resolver { + if routes == nil { + return nil + } + m := make(map[string][]*dnstype.Resolver) + for k, v := range routes { + m[string(k)] = v + } + return m + } + + tests := []tc{ + { + name: "noExit/noRoutes/noResolver", + exitNode: "", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{}, + wantDefaultResolvers: nil, + wantRoutes: nil, + }, + { + name: "tsExit/noRoutes/noResolver", + exitNode: "ts", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{}, + wantDefaultResolvers: []*dnstype.Resolver{{Addr: exitDOH}}, + wantRoutes: nil, + }, + { + name: "tsExit/noRoutes/defaultResolver", + exitNode: "ts", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{Resolvers: defaultResolvers}, + wantDefaultResolvers: []*dnstype.Resolver{{Addr: exitDOH}}, + wantRoutes: nil, + }, + + // The following two cases may need to be revisited. For a shared-in + // exit node split-DNS may effectively break, furthermore in the future + // if different nodes observe different DNS configurations, even a + // tailnet local exit node may present a different DNS configuration, + // which may not meet expectations in some use cases. + // In the case where a default resolver is set, the default resolver + // should also perhaps take precedence also. + { + name: "tsExit/routes/noResolver", + exitNode: "ts", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{Routes: stringifyRoutes(routes)}, + wantDefaultResolvers: []*dnstype.Resolver{{Addr: exitDOH}}, + wantRoutes: nil, + }, + { + name: "tsExit/routes/defaultResolver", + exitNode: "ts", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{Routes: stringifyRoutes(routes), Resolvers: defaultResolvers}, + wantDefaultResolvers: []*dnstype.Resolver{{Addr: exitDOH}}, + wantRoutes: nil, + }, + + // WireGuard exit nodes with DNS capabilities provide a "fallback" type + // behavior, they have a lower precedence than a default resolver, but + // otherwise allow split-DNS to operate as normal, and are used when + // there is no default resolver. + { + name: "wgExit/noRoutes/noResolver", + exitNode: "wg", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{}, + wantDefaultResolvers: wgResolvers, + wantRoutes: nil, + }, + { + name: "wgExit/noRoutes/defaultResolver", + exitNode: "wg", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{Resolvers: defaultResolvers}, + wantDefaultResolvers: defaultResolvers, + wantRoutes: nil, + }, + { + name: "wgExit/routes/defaultResolver", + exitNode: "wg", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{Routes: stringifyRoutes(routes), Resolvers: defaultResolvers}, + wantDefaultResolvers: defaultResolvers, + wantRoutes: routes, + }, + { + name: "wgExit/routes/noResolver", + exitNode: "wg", + peers: peers, + dnsConfig: &tailcfg.DNSConfig{Routes: stringifyRoutes(routes)}, + wantDefaultResolvers: wgResolvers, + wantRoutes: routes, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + nm := &netmap.NetworkMap{ + Peers: tc.peers, + DNS: *tc.dnsConfig, + } + + prefs := &ipn.Prefs{ExitNodeID: tc.exitNode, CorpDNS: true} + got := dnsConfigForNetmap(nm, peersMap(tc.peers), prefs.View(), t.Logf, "") + if !resolversEqual(t, got.DefaultResolvers, tc.wantDefaultResolvers) { + t.Errorf("DefaultResolvers: got %#v, want %#v", got.DefaultResolvers, tc.wantDefaultResolvers) + } + if !routesEqual(t, got.Routes, tc.wantRoutes) { + t.Errorf("Routes: got %#v, want %#v", got.Routes, tc.wantRoutes) + } + }) + } +} + +func resolversEqual(t *testing.T, a, b []*dnstype.Resolver) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + t.Errorf("resolversEqual: a == nil || b == nil : %#v != %#v", a, b) + return false + } + if len(a) != len(b) { + t.Errorf("resolversEqual: len(a) != len(b) : %#v != %#v", a, b) + return false + } + for i := range a { + if !a[i].Equal(b[i]) { + t.Errorf("resolversEqual: a != b [%d]: %v != %v", i, *a[i], *b[i]) + return false + } + } + return true +} + +func routesEqual(t *testing.T, a, b map[dnsname.FQDN][]*dnstype.Resolver) bool { + if len(a) != len(b) { + t.Logf("routes: len(a) != len(b): %d != %d", len(a), len(b)) + return false + } + for name := range a { + if !resolversEqual(t, a[name], b[name]) { + t.Logf("routes: a != b [%s]: %v != %v", name, a[name], b[name]) + return false + } + } + return true +} diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 9ead3525b..b3754f2b5 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -115,7 +115,8 @@ // - 73: 2023-09-01: Non-Windows clients expect to receive ClientVersion // - 74: 2023-09-18: Client understands NodeCapMap // - 75: 2023-09-12: Client understands NodeAttrDNSForwarderDisableTCPRetries -const CurrentCapabilityVersion CapabilityVersion = 75 +// - 76: 2023-09-20: Client understands ExitNodeDNSResolvers for IsWireGuardOnly nodes +const CurrentCapabilityVersion CapabilityVersion = 76 type StableID string