From d636407f14c97169bde96ac2cb9ac3b0f817975c Mon Sep 17 00:00:00 2001 From: Andrea Gottardo Date: Tue, 4 Jun 2024 09:41:13 -0700 Subject: [PATCH] net/dns: don't set MatchDomains on Apple platforms when no upstream nameservers available (#12334) This PR addresses a DNS issue on macOS as discussed this morning. Signed-off-by: Andrea Gottardo --- net/dns/manager.go | 25 ++-- net/dns/manager_tcp_test.go | 4 +- net/dns/manager_test.go | 250 +++++++++++++++++++++++++++++++++++- wgengine/userspace.go | 2 +- 4 files changed, 264 insertions(+), 17 deletions(-) diff --git a/net/dns/manager.go b/net/dns/manager.go index 14e297408..cdbc676eb 100644 --- a/net/dns/manager.go +++ b/net/dns/manager.go @@ -52,10 +52,11 @@ type Manager struct { resolver *resolver.Resolver os OSConfigurator + goos string // if empty, gets set to runtime.GOOS } // NewManagers created a new manager from the given config. -func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker, dialer *tsdial.Dialer, linkSel resolver.ForwardLinkSelector, knobs *controlknobs.Knobs) *Manager { +func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker, dialer *tsdial.Dialer, linkSel resolver.ForwardLinkSelector, knobs *controlknobs.Knobs, goos string) *Manager { if dialer == nil { panic("nil Dialer") } @@ -63,11 +64,15 @@ func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker, panic("Dialer has nil NetMon") } logf = logger.WithPrefix(logf, "dns: ") + if goos == "" { + goos = runtime.GOOS + } m := &Manager{ logf: logf, resolver: resolver.New(logf, linkSel, dialer, knobs), os: oscfg, health: health, + goos: goos, } m.ctx, m.ctxCancel = context.WithCancel(context.Background()) m.logf("using %T", m.os) @@ -166,7 +171,7 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig // Similarly, the OS always gets search paths. ocfg.SearchDomains = cfg.SearchDomains - if runtime.GOOS == "windows" { + if m.goos == "windows" { ocfg.Hosts = compileHostEntries(cfg) } @@ -219,8 +224,9 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig // // This bool is used in a couple of places below to implement this // workaround. - isWindows := runtime.GOOS == "windows" - if len(cfg.singleResolverSet()) > 0 && m.os.SupportsSplitDNS() && !isWindows { + isWindows := m.goos == "windows" + isApple := (m.goos == "darwin" || m.goos == "ios") + if len(cfg.singleResolverSet()) > 0 && m.os.SupportsSplitDNS() && !isWindows && !isApple { // Split DNS configuration requested, where all split domains // go to the same resolvers. We can let the OS do it. ocfg.Nameservers = toIPsOnly(cfg.singleResolverSet()) @@ -240,8 +246,6 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig // selectively answer ExtraRecords, and ignore other DNS traffic. As a // workaround, we read the existing default resolver configuration and use // that as the forwarder for all DNS traffic that quad-100 doesn't handle. - const isApple = runtime.GOOS == "darwin" || runtime.GOOS == "ios" - if isApple || !m.os.SupportsSplitDNS() { // If the OS can't do native split-dns, read out the underlying // resolver config and blend it into our config. @@ -257,9 +261,8 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig } } - if baseCfg == nil || isApple && len(baseCfg.Nameservers) == 0 { - // If there was no base config, or if we're on Apple and the base - // config is empty, then we need to fallback to SplitDNS mode. + if baseCfg == nil { + // If there was no base config, then we need to fallback to SplitDNS mode. ocfg.MatchDomains = cfg.matchDomains() } else { // On iOS only (for now), check if all route names point to resources inside the tailnet. @@ -269,7 +272,7 @@ func (m *Manager) compileConfig(cfg Config) (rcfg resolver.Config, ocfg OSConfig // we have any Routes outside the tailnet. Otherwise when app connectors are enabled, // a query for 'work-laptop' might lead to search domain expansion, resolving // as 'work-laptop.aws.com' for example. - if runtime.GOOS == "ios" && rcfg.RoutesRequireNoCustomResolvers() { + if m.goos == "ios" && rcfg.RoutesRequireNoCustomResolvers() { for r := range rcfg.Routes { ocfg.MatchDomains = append(ocfg.MatchDomains, r) } @@ -476,7 +479,7 @@ func CleanUp(logf logger.Logf, netMon *netmon.Monitor, interfaceName string) { } d := &tsdial.Dialer{Logf: logf} d.SetNetMon(netMon) - dns := NewManager(logf, oscfg, nil, d, nil, nil) + dns := NewManager(logf, oscfg, nil, d, nil, nil, runtime.GOOS) if err := dns.Down(); err != nil { logf("dns down: %v", err) } diff --git a/net/dns/manager_tcp_test.go b/net/dns/manager_tcp_test.go index 8c61035d2..2982c6129 100644 --- a/net/dns/manager_tcp_test.go +++ b/net/dns/manager_tcp_test.go @@ -88,7 +88,7 @@ func TestDNSOverTCP(t *testing.T) { SearchDomains: fqdns("coffee.shop"), }, } - m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil) + m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil, "") m.resolver.TestOnlySetHook(f.SetResolver) m.Set(Config{ Hosts: hosts( @@ -173,7 +173,7 @@ func TestDNSOverTCP_TooLarge(t *testing.T) { SearchDomains: fqdns("coffee.shop"), }, } - m := NewManager(log, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil) + m := NewManager(log, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil, "") m.resolver.TestOnlySetHook(f.SetResolver) m.Set(Config{ Hosts: hosts("andrew.ts.com.", "1.2.3.4"), diff --git a/net/dns/manager_test.go b/net/dns/manager_test.go index 1e7adc943..4caf33335 100644 --- a/net/dns/manager_test.go +++ b/net/dns/manager_test.go @@ -172,6 +172,7 @@ func TestManager(t *testing.T) { bs OSConfig os OSConfig rs resolver.Config + goos string // empty means "linux" }{ { name: "empty", @@ -424,7 +425,7 @@ func TestManager(t *testing.T) { }, }, { - name: "routes-multi-split", + name: "routes-multi-split-linux", in: Config{ Routes: upstreams( "corp.com", "2.2.2.2", @@ -442,6 +443,57 @@ func TestManager(t *testing.T) { "corp.com.", "2.2.2.2", "bigco.net.", "3.3.3.3"), }, + goos: "linux", + }, + { + // The `routes-multi-split-linux` test case above on Darwin should NOT result in a split + // DNS configuration. + // Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains + // without those domains also being SearchDomains. + name: "routes-multi-does-not-split-on-darwin", + in: Config{ + Routes: upstreams( + "corp.com", "2.2.2.2", + "bigco.net", "3.3.3.3"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + split: false, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "corp.com.", "2.2.2.2", + "bigco.net.", "3.3.3.3"), + }, + goos: "darwin", + }, + { + // The `routes-multi-split-linux` test case above on iOS should NOT result in a split + // DNS configuration. + // Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains + // without those domains also being SearchDomains. + name: "routes-multi-does-not-split-on-ios", + in: Config{ + Routes: upstreams( + "corp.com", "2.2.2.2", + "bigco.net", "3.3.3.3"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + split: false, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "corp.com.", "2.2.2.2", + "bigco.net.", "3.3.3.3"), + }, + goos: "ios", }, { name: "magic", @@ -489,6 +541,59 @@ func TestManager(t *testing.T) { "bradfitz.ts.com.", "2.3.4.5"), LocalDomains: fqdns("ts.com."), }, + goos: "linux", + }, + { + // The `magic-split` test case above on Darwin should NOT result in a split DNS configuration. + // Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains + // without those domains also being SearchDomains. + name: "magic-split-does-not-split-on-darwin", + in: Config{ + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + Routes: upstreams("ts.com", ""), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + split: false, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + rs: resolver.Config{ + Routes: upstreams(".", ""), + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + LocalDomains: fqdns("ts.com."), + }, + goos: "darwin", + }, + { + // The `magic-split` test case above on iOS should NOT result in a split DNS configuration. + // Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains + // without those domains also being SearchDomains. + name: "magic-split-does-not-split-on-ios", + in: Config{ + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + Routes: upstreams("ts.com", ""), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + split: false, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + rs: resolver.Config{ + Routes: upstreams(".", ""), + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + LocalDomains: fqdns("ts.com."), + }, + goos: "ios", }, { name: "routes-magic", @@ -518,7 +623,7 @@ func TestManager(t *testing.T) { }, }, { - name: "routes-magic-split", + name: "routes-magic-split-linux", in: Config{ Routes: upstreams( "corp.com", "2.2.2.2", @@ -541,6 +646,71 @@ func TestManager(t *testing.T) { "bradfitz.ts.com.", "2.3.4.5"), LocalDomains: fqdns("ts.com."), }, + goos: "linux", + }, + { + // The `routes-magic-split-linux` test case above on Darwin should NOT result in a + // split DNS configuration. + // Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains + // without those domains also being SearchDomains. + name: "routes-magic-does-not-split-on-darwin", + in: Config{ + Routes: upstreams( + "corp.com", "2.2.2.2", + "ts.com", ""), + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + split: true, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "corp.com.", "2.2.2.2", + ), + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + LocalDomains: fqdns("ts.com."), + }, + goos: "darwin", + }, + { + // The `routes-magic-split-linux` test case above on Darwin should NOT result in a + // split DNS configuration. + // Check that MatchDomains is empty. Due to Apple limitations, we cannot set MatchDomains + // without those domains also being SearchDomains. + name: "routes-magic-does-not-split-on-ios", + in: Config{ + Routes: upstreams( + "corp.com", "2.2.2.2", + "ts.com", ""), + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + split: true, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("tailscale.com", "universe.tf"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "corp.com.", "2.2.2.2", + ), + Hosts: hosts( + "dave.ts.com.", "1.2.3.4", + "bradfitz.ts.com.", "2.3.4.5"), + LocalDomains: fqdns("ts.com."), + }, + goos: "ios", }, { name: "exit-node-forward", @@ -598,6 +768,76 @@ func TestManager(t *testing.T) { Routes: upstreams(".", "https://dns.nextdns.io/c3a884"), }, }, + { + // on iOS exclusively, tests the split DNS behavior for battery life optimization added in + // https://github.com/tailscale/tailscale/pull/10576 + name: "ios-use-split-dns-when-no-custom-resolvers", + in: Config{ + Routes: upstreams("ts.net", "199.247.155.52", "optimistic-display.ts.net", ""), + SearchDomains: fqdns("optimistic-display.ts.net"), + }, + split: true, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("optimistic-display.ts.net"), + MatchDomains: fqdns("ts.net"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "ts.net", "199.247.155.52", + ), + LocalDomains: fqdns("optimistic-display.ts.net."), + }, + goos: "ios", + }, + { + // if using app connectors, the battery life optimization above should not be applied + name: "ios-dont-use-split-dns-when-app-connector-resolver-needed", + in: Config{ + Routes: upstreams( + "ts.net", "199.247.155.52", + "optimistic-display.ts.net", "", + "github.com", "https://dnsresolver.bigcorp.com/2f143"), + SearchDomains: fqdns("optimistic-display.ts.net"), + }, + split: true, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("optimistic-display.ts.net"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "github.com", "https://dnsresolver.bigcorp.com/2f143", + "ts.net", "199.247.155.52", + ), + LocalDomains: fqdns("optimistic-display.ts.net."), + }, + goos: "ios", + }, + { + // on darwin, verify that with the same config as in ios-use-split-dns-when-no-custom-resolvers, + // MatchDomains are NOT set. + name: "darwin-dont-use-split-dns-when-no-custom-resolvers", + in: Config{ + Routes: upstreams("ts.net", "199.247.155.52", "optimistic-display.ts.net", ""), + SearchDomains: fqdns("optimistic-display.ts.net"), + }, + split: true, + os: OSConfig{ + Nameservers: mustIPs("100.100.100.100"), + SearchDomains: fqdns("optimistic-display.ts.net"), + }, + rs: resolver.Config{ + Routes: upstreams( + ".", "", + "ts.net", "199.247.155.52", + ), + LocalDomains: fqdns("optimistic-display.ts.net."), + }, + goos: "darwin", + }, } trIP := cmp.Transformer("ipStr", func(ip netip.Addr) string { return ip.String() }) @@ -614,7 +854,11 @@ func TestManager(t *testing.T) { SplitDNS: test.split, BaseConfig: test.bs, } - m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil) + goos := test.goos + if goos == "" { + goos = "linux" + } + m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil, goos) m.resolver.TestOnlySetHook(f.SetResolver) if err := m.Set(test.in); err != nil { diff --git a/wgengine/userspace.go b/wgengine/userspace.go index aaa45ddda..b65395fd8 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -348,7 +348,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) tunName, _ := conf.Tun.Name() conf.Dialer.SetTUNName(tunName) conf.Dialer.SetNetMon(e.netMon) - e.dns = dns.NewManager(logf, conf.DNS, e.health, conf.Dialer, fwdDNSLinkSelector{e, tunName}, conf.ControlKnobs) + e.dns = dns.NewManager(logf, conf.DNS, e.health, conf.Dialer, fwdDNSLinkSelector{e, tunName}, conf.ControlKnobs, runtime.GOOS) // TODO: there's probably a better place for this sockstats.SetNetMon(e.netMon)