From c5d572f371cb237d32a2b7eaf41423c3fdbaa196 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 15 Nov 2021 10:33:27 -0800 Subject: [PATCH] net/dns: correctly handle NetworkManager-managed DNS that points to resolved. Fixes #3304 Signed-off-by: David Anderson --- net/dns/manager_linux.go | 68 ++++++++++++++++++++++++++++------- net/dns/manager_linux_test.go | 48 ++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 14 deletions(-) diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go index 6748a8881..f2f6a1a7b 100644 --- a/net/dns/manager_linux.go +++ b/net/dns/manager_linux.go @@ -180,20 +180,55 @@ func dnsMode(logf logger.Logf, env newOSConfigEnv) (ret string, err error) { return "direct", nil } case "NetworkManager": - // You'd think we would use newNMManager somewhere in - // here. However, as explained in - // https://github.com/tailscale/tailscale/issues/1699 , using - // NetworkManager for DNS configuration carries with it the - // cost of losing IPv6 configuration on the Tailscale network - // interface. So, when we can avoid it, we bypass - // NetworkManager by replacing resolv.conf directly. - // - // If you ever try to put NMManager back here, keep in mind - // that versions >=1.26.6 will ignore DNS configuration - // anyway, so you still need a fallback path that uses - // directManager. dbg("rc", "nm") - return "direct", nil + // Sometimes, NetworkManager owns the configuration but points + // it at systemd-resolved. + if err := resolvedIsActuallyResolver(bs); err != nil { + dbg("resolved", "not-in-use") + // You'd think we would use newNMManager here. However, as + // explained in + // https://github.com/tailscale/tailscale/issues/1699 , + // using NetworkManager for DNS configuration carries with + // it the cost of losing IPv6 configuration on the + // Tailscale network interface. So, when we can avoid it, + // we bypass NetworkManager by replacing resolv.conf + // directly. + // + // If you ever try to put NMManager back here, keep in mind + // that versions >=1.26.6 will ignore DNS configuration + // anyway, so you still need a fallback path that uses + // directManager. + return "direct", nil + } + dbg("nm-resolved", "yes") + + if err := env.dbusPing("org.freedesktop.resolve1", "/org/freedesktop/resolve1"); err != nil { + dbg("resolved", "no") + return "direct", nil + } + + // See large comment above for reasons we'd use NM rather than + // resolved. systemd-resolved is actually in charge of DNS + // configuration, but in some cases we might need to configure + // it via NetworkManager. All the logic below is probing for + // that case: is NetworkManager running? If so, is it one of + // the versions that requires direct interaction with it? + if err := env.dbusPing("org.freedesktop.NetworkManager", "/org/freedesktop/NetworkManager/DnsManager"); err != nil { + dbg("nm", "no") + return "systemd-resolved", nil + } + safe, err := env.nmVersionBetween("1.26.0", "1.26.5") + if err != nil { + // Failed to figure out NM's version, can't make a correct + // decision. + return "", fmt.Errorf("checking NetworkManager version: %v", err) + } + if safe { + dbg("nm-safe", "yes") + return "network-manager", nil + } + dbg("nm-safe", "no") + return "systemd-resolved", nil default: dbg("rc", "unknown") return "direct", nil @@ -244,6 +279,13 @@ func nmIsUsingResolved() error { return nil } +// resolvedIsActuallyResolver reports whether the given resolv.conf +// bytes describe a configuration where systemd-resolved (127.0.0.53) +// is the only configured nameserver. +// +// Returns an error if the configuration is something other than +// exclusively systemd-resolved, or nil if the config is only +// systemd-resolved. func resolvedIsActuallyResolver(bs []byte) error { cfg, err := readResolv(bytes.NewBuffer(bs)) if err != nil { diff --git a/net/dns/manager_linux_test.go b/net/dns/manager_linux_test.go index 985b0a923..7000f2cec 100644 --- a/net/dns/manager_linux_test.go +++ b/net/dns/manager_linux_test.go @@ -34,7 +34,7 @@ func TestLinuxDNSMode(t *testing.T) { resolvDotConf( "# Managed by NetworkManager", "nameserver 10.0.0.1")), - wantLog: "dns: [rc=nm ret=direct]", + wantLog: "dns: [rc=nm resolved=not-in-use ret=direct]", want: "direct", }, { @@ -172,6 +172,52 @@ func TestLinuxDNSMode(t *testing.T) { wantLog: "dns: [rc=resolved resolved=no ret=direct]", want: "direct", }, + { + // regression test for https://github.com/tailscale/tailscale/issues/3304 + name: "networkmanager_but_pointing_at_systemd-resolved", + env: env(resolvDotConf( + "# Generated by NetworkManager", + "nameserver 127.0.0.53", + "options edns0 trust-ad"), + resolvedRunning(), + nmRunning("1.32.12", true)), + wantLog: "dns: [rc=nm nm-resolved=yes nm-safe=no ret=systemd-resolved]", + want: "systemd-resolved", + }, + { + // regression test for https://github.com/tailscale/tailscale/issues/3304 + name: "networkmanager_but_pointing_at_systemd-resolved_but_no_resolved", + env: env(resolvDotConf( + "# Generated by NetworkManager", + "nameserver 127.0.0.53", + "options edns0 trust-ad"), + nmRunning("1.32.12", true)), + wantLog: "dns: [rc=nm nm-resolved=yes resolved=no ret=direct]", + want: "direct", + }, + { + // regression test for https://github.com/tailscale/tailscale/issues/3304 + name: "networkmanager_but_pointing_at_systemd-resolved_and_safe_nm", + env: env(resolvDotConf( + "# Generated by NetworkManager", + "nameserver 127.0.0.53", + "options edns0 trust-ad"), + resolvedRunning(), + nmRunning("1.26.3", true)), + wantLog: "dns: [rc=nm nm-resolved=yes nm-safe=yes ret=network-manager]", + want: "network-manager", + }, + { + // regression test for https://github.com/tailscale/tailscale/issues/3304 + name: "networkmanager_but_pointing_at_systemd-resolved_and_no_networkmanager", + env: env(resolvDotConf( + "# Generated by NetworkManager", + "nameserver 127.0.0.53", + "options edns0 trust-ad"), + resolvedRunning()), + wantLog: "dns: [rc=nm nm-resolved=yes nm=no ret=systemd-resolved]", + want: "systemd-resolved", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {