From c7ce4e07e5c43f172d2655c4fe5c6d85731bb70b Mon Sep 17 00:00:00 2001 From: James Tucker Date: Tue, 19 Sep 2023 15:55:34 -0700 Subject: [PATCH] ipn/ipnlocal: allow Split-DNS and default resolvers with WireGuard nodes The initial implementation directly mirrored the behavior of Tailscale exit nodes, where the WireGuard exit node DNS took precedence over other configuration. This adjusted implementation treats the WireGuard DNS resolvers as a lower precedence default resolver than the tailnet default resolver, and allows split DNS configuration as well. This also adds test coverage to the existing DNS selection behavior with respect to default resolvers and split DNS routes for Tailscale exit nodes above cap 25. There may be some refinement to do in the logic in those cases, as split DNS may not be working as we intend, though that would be a pre-existing and separate issue. Updates #9377 Signed-off-by: James Tucker --- ipn/ipnlocal/local.go | 25 +++-- ipn/ipnlocal/local_test.go | 181 ++++++++++++++++++++++++++++++++++--- 2 files changed, 177 insertions(+), 29 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 345799752..77ca3dce1 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -3308,9 +3308,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+) @@ -3320,14 +3318,17 @@ func dnsConfigForNetmap(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg. return dcfg } - // If we're using an exit node and that exit node is IsWireGuardOnly with - // ExitNodeDNSResolver set, then add that as the default. - if resolvers, ok := wireguardExitNodeDNSResolvers(nm, peers, prefs.ExitNodeID()); ok { - addDefault(resolvers) - return dcfg + // 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) + } } - addDefault(nm.DNS.Resolvers) for suffix, resolvers := range nm.DNS.Routes { fqdn, err := dnsname.ToFQDN(suffix) if err != nil { @@ -3342,11 +3343,7 @@ func dnsConfigForNetmap(nm *netmap.NetworkMap, peers map[tailcfg.NodeID]tailcfg. // // While we're already populating it, might as well size the // slice appropriately. - 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 diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index a55b6f638..0957d0863 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -28,6 +28,7 @@ import ( "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" @@ -922,41 +923,191 @@ func TestWireguardExitNodeDNSResolvers(t *testing.T) { nm := &netmap.NetworkMap{} gotResolvers, gotOK := wireguardExitNodeDNSResolvers(nm, peers, tc.id) - if gotOK != tc.wantOK || !resolversEqual(gotResolvers, tc.wantResolvers) { + 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 TestDNSConfigForNetmapForWireguardExitNode(t *testing.T) { - resolvers := []*dnstype.Resolver{{Addr: "dns.example.com"}} - nm := &netmap.NetworkMap{} - peers := map[tailcfg.NodeID]tailcfg.NodeView{ - 1: (&tailcfg.Node{ +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: "1", + StableID: "wg", IsWireGuardOnly: true, - ExitNodeDNSResolvers: resolvers, + 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(), } - prefs := &ipn.Prefs{ - ExitNodeID: "1", - CorpDNS: true, + 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 } - got := dnsConfigForNetmap(nm, peers, prefs.View(), t.Logf, "") - if !resolversEqual(got.DefaultResolvers, resolvers) { - t.Errorf("got %v, want %v", got.DefaultResolvers, resolvers) + 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(a, b []*dnstype.Resolver) bool { +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 } }