From 854d5d36a163ddf69089759d6d7ccdf75b030e0e Mon Sep 17 00:00:00 2001 From: David Anderson Date: Mon, 12 Apr 2021 15:51:37 -0700 Subject: [PATCH] net/dns: return error from NewOSManager, use it to initialize NM. Signed-off-by: David Anderson --- cmd/tailscaled/tailscaled.go | 6 ++- cmd/tailscaled/tailscaled_windows.go | 8 +++- net/dns/debian_resolvconf.go | 4 +- net/dns/direct.go | 4 +- net/dns/manager.go | 6 ++- net/dns/manager_default.go | 2 +- net/dns/manager_freebsd.go | 2 +- net/dns/manager_linux.go | 2 +- net/dns/manager_openbsd.go | 2 +- net/dns/manager_windows.go | 4 +- net/dns/nm.go | 57 ++++++++++++++-------------- net/dns/noop.go | 4 +- net/dns/openresolv.go | 4 +- net/dns/resolvconf.go | 2 +- net/dns/resolved.go | 4 +- wgengine/userspace.go | 6 ++- 16 files changed, 67 insertions(+), 50 deletions(-) diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go index e55bcedcb..c36d13992 100644 --- a/cmd/tailscaled/tailscaled.go +++ b/cmd/tailscaled/tailscaled.go @@ -365,7 +365,11 @@ func tryEngine(logf logger.Logf, linkMon *monitor.Mon, name string) (e wgengine. dev.Close() return nil, false, err } - conf.DNS = dns.NewOSConfigurator(logf, devName) + d, err := dns.NewOSConfigurator(logf, devName) + if err != nil { + return nil, false, err + } + conf.DNS = d conf.Router = r if wrapNetstack { conf.Router = netstack.NewSubnetRouterWrapper(conf.Router) diff --git a/cmd/tailscaled/tailscaled_windows.go b/cmd/tailscaled/tailscaled_windows.go index 7f7a0b981..c19137fe2 100644 --- a/cmd/tailscaled/tailscaled_windows.go +++ b/cmd/tailscaled/tailscaled_windows.go @@ -175,10 +175,16 @@ func startIPNServer(ctx context.Context, logid string) error { if wrapNetstack { r = netstack.NewSubnetRouterWrapper(r) } + d, err := dns.NewOSConfigurator(logf, devName) + if err != nil { + r.Close() + dev.Close() + return nil, err + } eng, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{ Tun: dev, Router: r, - DNS: dns.NewOSConfigurator(logf, devName), + DNS: d, ListenPort: 41641, }) if err != nil { diff --git a/net/dns/debian_resolvconf.go b/net/dns/debian_resolvconf.go index f0c440547..9dbf23e94 100644 --- a/net/dns/debian_resolvconf.go +++ b/net/dns/debian_resolvconf.go @@ -53,7 +53,7 @@ type resolvconfManager struct { scriptInstalled bool // libc update script has been installed } -func newDebianResolvconfManager(logf logger.Logf) *resolvconfManager { +func newDebianResolvconfManager(logf logger.Logf) (*resolvconfManager, error) { ret := &resolvconfManager{ logf: logf, listRecordsPath: "/lib/resolvconf/list-records", @@ -86,7 +86,7 @@ func newDebianResolvconfManager(logf logger.Logf) *resolvconfManager { ret.interfacesDir = "/etc/resolvconf/run/interfaces" } - return ret + return ret, nil } func (m *resolvconfManager) SetDNS(config OSConfig) error { diff --git a/net/dns/direct.go b/net/dns/direct.go index 6da43bc4a..23a5153b0 100644 --- a/net/dns/direct.go +++ b/net/dns/direct.go @@ -120,8 +120,8 @@ func isResolvedRunning() bool { // or as cleanup if the program terminates unexpectedly. type directManager struct{} -func newDirectManager() directManager { - return directManager{} +func newDirectManager() (directManager, error) { + return directManager{}, nil } // ownedByTailscale reports whether /etc/resolv.conf seems to be a diff --git a/net/dns/manager.go b/net/dns/manager.go index a026c4b40..abe526aa4 100644 --- a/net/dns/manager.go +++ b/net/dns/manager.go @@ -240,7 +240,11 @@ func (m *Manager) Down() error { // in case the Tailscale daemon terminated without closing the router. // No other state needs to be instantiated before this runs. func Cleanup(logf logger.Logf, interfaceName string) { - oscfg := NewOSConfigurator(logf, interfaceName) + oscfg, err := NewOSConfigurator(logf, interfaceName) + if err != nil { + logf("creating dns cleanup: %v", err) + return + } dns := NewManager(logf, oscfg, nil) if err := dns.Down(); err != nil { logf("dns down: %v", err) diff --git a/net/dns/manager_default.go b/net/dns/manager_default.go index f7e7e3e5e..4bc53ffd6 100644 --- a/net/dns/manager_default.go +++ b/net/dns/manager_default.go @@ -8,7 +8,7 @@ package dns import "tailscale.com/types/logger" -func NewOSConfigurator(logger.Logf, string) OSConfigurator { +func NewOSConfigurator(logger.Logf, string) (OSConfigurator, error) { // TODO(dmytro): on darwin, we should use a macOS-specific method such as scutil. // This is currently not implemented. Editing /etc/resolv.conf does not work, // as most applications use the system resolver, which disregards it. diff --git a/net/dns/manager_freebsd.go b/net/dns/manager_freebsd.go index 1485c6728..fe839c084 100644 --- a/net/dns/manager_freebsd.go +++ b/net/dns/manager_freebsd.go @@ -6,7 +6,7 @@ package dns import "tailscale.com/types/logger" -func NewOSConfigurator(logf logger.Logf, _ string) OSConfigurator { +func NewOSConfigurator(logf logger.Logf, _ string) (OSConfigurator, error) { switch { case isResolvconfActive(): return newResolvconfManager(logf) diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go index 3143334cc..15fd6c860 100644 --- a/net/dns/manager_linux.go +++ b/net/dns/manager_linux.go @@ -6,7 +6,7 @@ package dns import "tailscale.com/types/logger" -func NewOSConfigurator(logf logger.Logf, interfaceName string) OSConfigurator { +func NewOSConfigurator(logf logger.Logf, interfaceName string) (OSConfigurator, error) { switch { case isResolvedActive(): return newResolvedManager(logf) diff --git a/net/dns/manager_openbsd.go b/net/dns/manager_openbsd.go index 0abddd2f7..ed0a08d45 100644 --- a/net/dns/manager_openbsd.go +++ b/net/dns/manager_openbsd.go @@ -6,6 +6,6 @@ package dns import "tailscale.com/types/logger" -func NewOSConfigurator(logger.Logf, string) OSConfigurator { +func NewOSConfigurator(logger.Logf, string) (OSConfigurator, error) { return newDirectManager() } diff --git a/net/dns/manager_windows.go b/net/dns/manager_windows.go index 4b1cb3659..b8842aa3a 100644 --- a/net/dns/manager_windows.go +++ b/net/dns/manager_windows.go @@ -39,7 +39,7 @@ type windowsManager struct { nrptWorks bool } -func NewOSConfigurator(logf logger.Logf, interfaceName string) OSConfigurator { +func NewOSConfigurator(logf logger.Logf, interfaceName string) (OSConfigurator, error) { ret := windowsManager{ logf: logf, guid: interfaceName, @@ -56,7 +56,7 @@ func NewOSConfigurator(logf logger.Logf, interfaceName string) OSConfigurator { ret.delKey(nrptBase) } - return ret + return ret, nil } // keyOpenTimeout is how long we wait for a registry key to diff --git a/net/dns/nm.go b/net/dns/nm.go index a0f52938f..02eb150c8 100644 --- a/net/dns/nm.go +++ b/net/dns/nm.go @@ -71,42 +71,26 @@ func isNMActive() bool { // nmManager uses the NetworkManager DBus API. type nmManager struct { interfaceName string - canSplit bool + manager dbus.BusObject + dnsManager dbus.BusObject } -func nmCanSplitDNS() bool { +func newNMManager(interfaceName string) (*nmManager, error) { conn, err := dbus.SystemBus() if err != nil { - return false + return nil, err } - var mode string - nm := conn.Object("org.freedesktop.NetworkManager", dbus.ObjectPath("/org/freedesktop/NetworkManager/DnsManager")) - v, err := nm.GetProperty("org.freedesktop.NetworkManager.DnsManager.Mode") - if err != nil { - return false - } - mode, ok := v.Value().(string) - if !ok { - return false - } - - // Per NM's documentation, it only does split-DNS when it's - // programming dnsmasq or systemd-resolved. All other modes are - // primary-only. - return mode == "dnsmasq" || mode == "systemd-resolved" -} - -func newNMManager(interfaceName string) nmManager { - return nmManager{ + return &nmManager{ interfaceName: interfaceName, - canSplit: nmCanSplitDNS(), - } + manager: conn.Object("org.freedesktop.NetworkManager", dbus.ObjectPath("/org/freedesktop/NetworkManager")), + dnsManager: conn.Object("org.freedesktop.NetworkManager", dbus.ObjectPath("/org/freedesktop/NetworkManager/DnsManager")), + }, nil } type nmConnectionSettings map[string]map[string]dbus.Variant -func (m nmManager) SetDNS(config OSConfig) error { +func (m *nmManager) SetDNS(config OSConfig) error { ctx, cancel := context.WithTimeout(context.Background(), reconfigTimeout) defer cancel() @@ -127,7 +111,7 @@ func (m nmManager) SetDNS(config OSConfig) error { return err } -func (m nmManager) trySet(ctx context.Context, config OSConfig) error { +func (m *nmManager) trySet(ctx context.Context, config OSConfig) error { conn, err := dbus.SystemBus() if err != nil { return fmt.Errorf("connecting to system bus: %w", err) @@ -262,9 +246,24 @@ func (m nmManager) trySet(ctx context.Context, config OSConfig) error { return nil } -func (m nmManager) SupportsSplitDNS() bool { return m.canSplit } +func (m *nmManager) SupportsSplitDNS() bool { + var mode string + v, err := m.dnsManager.GetProperty("org.freedesktop.NetworkManager.DnsManager.Mode") + if err != nil { + return false + } + mode, ok := v.Value().(string) + if !ok { + return false + } -func (m nmManager) GetBaseConfig() (OSConfig, error) { + // Per NM's documentation, it only does split-DNS when it's + // programming dnsmasq or systemd-resolved. All other modes are + // primary-only. + return mode == "dnsmasq" || mode == "systemd-resolved" +} + +func (m *nmManager) GetBaseConfig() (OSConfig, error) { conn, err := dbus.SystemBus() if err != nil { return OSConfig{}, err @@ -362,7 +361,7 @@ func (m nmManager) GetBaseConfig() (OSConfig, error) { return ret, nil } -func (m nmManager) Close() error { +func (m *nmManager) Close() error { // No need to do anything on close, NetworkManager will delete our // settings when the tailscale interface goes away. return nil diff --git a/net/dns/noop.go b/net/dns/noop.go index f9d79e6ce..f08ecc036 100644 --- a/net/dns/noop.go +++ b/net/dns/noop.go @@ -13,6 +13,6 @@ func (m noopManager) GetBaseConfig() (OSConfig, error) { return OSConfig{}, ErrGetBaseConfigNotSupported } -func NewNoopManager() noopManager { - return noopManager{} +func NewNoopManager() (noopManager, error) { + return noopManager{}, nil } diff --git a/net/dns/openresolv.go b/net/dns/openresolv.go index 183e8d240..552615d07 100644 --- a/net/dns/openresolv.go +++ b/net/dns/openresolv.go @@ -15,8 +15,8 @@ import ( // implementation of the `resolvconf` program. type openresolvManager struct{} -func newOpenresolvManager() openresolvManager { - return openresolvManager{} +func newOpenresolvManager() (openresolvManager, error) { + return openresolvManager{}, nil } func (m openresolvManager) SetDNS(config OSConfig) error { diff --git a/net/dns/resolvconf.go b/net/dns/resolvconf.go index 73e7d947f..c583a4f34 100644 --- a/net/dns/resolvconf.go +++ b/net/dns/resolvconf.go @@ -42,7 +42,7 @@ func isResolvconfActive() bool { return false } -func newResolvconfManager(logf logger.Logf) OSConfigurator { +func newResolvconfManager(logf logger.Logf) (OSConfigurator, error) { _, err := exec.Command("resolvconf", "--version").CombinedOutput() if err != nil { if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 99 { diff --git a/net/dns/resolved.go b/net/dns/resolved.go index 523cf0d70..998ea657a 100644 --- a/net/dns/resolved.go +++ b/net/dns/resolved.go @@ -87,10 +87,10 @@ type resolvedManager struct { logf logger.Logf } -func newResolvedManager(logf logger.Logf) resolvedManager { +func newResolvedManager(logf logger.Logf) (resolvedManager, error) { return resolvedManager{ logf: logf, - } + }, nil } // Up implements managerImpl. diff --git a/wgengine/userspace.go b/wgengine/userspace.go index fcec5b738..c39532d4d 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -219,7 +219,11 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) } if conf.DNS == nil { logf("[v1] using fake (no-op) DNS configurator") - conf.DNS = dns.NewNoopManager() + d, err := dns.NewNoopManager() + if err != nil { + return nil, err + } + conf.DNS = d } tsTUNDev := tstun.Wrap(logf, conf.Tun)