diff --git a/net/dns/direct.go b/net/dns/direct.go index f42a83803..3344579ea 100644 --- a/net/dns/direct.go +++ b/net/dns/direct.go @@ -18,6 +18,7 @@ "strings" "inet.af/netaddr" + "tailscale.com/types/logger" "tailscale.com/util/dnsname" ) @@ -133,18 +134,36 @@ func isResolvedRunning() bool { // The caller must call Down before program shutdown // or as cleanup if the program terminates unexpectedly. type directManager struct { - fs wholeFileFS + logf logger.Logf + fs wholeFileFS + // renameBroken is set if fs.Rename to or from /etc/resolv.conf + // fails. This can happen in some container runtimes, where + // /etc/resolv.conf is bind-mounted from outside the container, + // and therefore /etc and /etc/resolv.conf are different + // filesystems as far as rename(2) is concerned. + // + // In those situations, we fall back to emulating rename with file + // copies and truncations, which is not as good (opens up a race + // where a reader can see an empty or partial /etc/resolv.conf), + // but is better than having non-functioning DNS. + renameBroken bool } -func newDirectManager() directManager { - return directManager{fs: directFS{}} +func newDirectManager(logf logger.Logf) *directManager { + return &directManager{ + logf: logf, + fs: directFS{}, + } } -func newDirectManagerOnFS(fs wholeFileFS) directManager { - return directManager{fs: fs} +func newDirectManagerOnFS(logf logger.Logf, fs wholeFileFS) *directManager { + return &directManager{ + logf: logf, + fs: fs, + } } -func (m directManager) readResolvFile(path string) (OSConfig, error) { +func (m *directManager) readResolvFile(path string) (OSConfig, error) { b, err := m.fs.ReadFile(path) if err != nil { return OSConfig{}, err @@ -154,7 +173,7 @@ func (m directManager) readResolvFile(path string) (OSConfig, error) { // ownedByTailscale reports whether /etc/resolv.conf seems to be a // tailscale-managed file. -func (m directManager) ownedByTailscale() (bool, error) { +func (m *directManager) ownedByTailscale() (bool, error) { isRegular, err := m.fs.Stat(resolvConf) if err != nil { if os.IsNotExist(err) { @@ -177,7 +196,7 @@ func (m directManager) ownedByTailscale() (bool, error) { // backupConfig creates or updates a backup of /etc/resolv.conf, if // resolv.conf does not currently contain a Tailscale-managed config. -func (m directManager) backupConfig() error { +func (m *directManager) backupConfig() error { if _, err := m.fs.Stat(resolvConf); err != nil { if os.IsNotExist(err) { // No resolv.conf, nothing to back up. Also get rid of any @@ -196,10 +215,10 @@ func (m directManager) backupConfig() error { return nil } - return m.fs.Rename(resolvConf, backupConf) + return m.rename(resolvConf, backupConf) } -func (m directManager) restoreBackup() (restored bool, err error) { +func (m *directManager) restoreBackup() (restored bool, err error) { if _, err := m.fs.Stat(backupConf); err != nil { if os.IsNotExist(err) { // No backup, nothing we can do. @@ -225,14 +244,48 @@ func (m directManager) restoreBackup() (restored bool, err error) { } // We own resolv.conf, and a backup exists. - if err := m.fs.Rename(backupConf, resolvConf); err != nil { + if err := m.rename(backupConf, resolvConf); err != nil { return false, err } return true, nil } -func (m directManager) SetDNS(config OSConfig) (err error) { +// rename tries to rename old to new using m.fs.Rename, and falls back +// to hand-copying bytes and truncating old if that fails. +// +// This is a workaround to /etc/resolv.conf being a bind-mounted file +// some container environments, which cannot be moved elsewhere in +// /etc (because that would be a cross-filesystem move) or deleted +// (because that would break the bind in surprising ways). +func (m *directManager) rename(old, new string) error { + if !m.renameBroken { + err := m.fs.Rename(old, new) + if err == nil { + return nil + } + m.logf("rename of %q to %q failed (%v), falling back to copy+delete", old, new, err) + m.renameBroken = true + } + + bs, err := m.fs.ReadFile(old) + if err != nil { + return fmt.Errorf("reading %q to rename: %v", old, err) + } + if err := m.fs.WriteFile(new, bs, 0644); err != nil { + return fmt.Errorf("writing to %q in rename of %q: %v", new, old, err) + } + + if err := m.fs.Remove(old); err != nil { + err2 := m.fs.Truncate(old) + if err2 != nil { + return fmt.Errorf("remove of %q failed (%v) and so did truncate: %v", old, err, err2) + } + } + return nil +} + +func (m *directManager) SetDNS(config OSConfig) (err error) { var changed bool if config.IsZero() { changed, err = m.restoreBackup() @@ -247,7 +300,7 @@ func (m directManager) SetDNS(config OSConfig) (err error) { buf := new(bytes.Buffer) writeResolvConf(buf, config.Nameservers, config.SearchDomains) - if err := atomicWriteFile(m.fs, resolvConf, buf.Bytes(), 0644); err != nil { + if err := m.atomicWriteFile(m.fs, resolvConf, buf.Bytes(), 0644); err != nil { return err } } @@ -275,11 +328,11 @@ func (m directManager) SetDNS(config OSConfig) (err error) { return nil } -func (m directManager) SupportsSplitDNS() bool { +func (m *directManager) SupportsSplitDNS() bool { return false } -func (m directManager) GetBaseConfig() (OSConfig, error) { +func (m *directManager) GetBaseConfig() (OSConfig, error) { owned, err := m.ownedByTailscale() if err != nil { return OSConfig{}, err @@ -292,7 +345,7 @@ func (m directManager) GetBaseConfig() (OSConfig, error) { return m.readResolvFile(fileToRead) } -func (m directManager) Close() error { +func (m *directManager) Close() error { // We used to keep a file for the tailscale config and symlinked // to it, but then we stopped because /etc/resolv.conf being a // symlink to surprising places breaks snaps and other sandboxing @@ -324,7 +377,7 @@ func (m directManager) Close() error { } // We own resolv.conf, and a backup exists. - if err := m.fs.Rename(backupConf, resolvConf); err != nil { + if err := m.rename(backupConf, resolvConf); err != nil { return err } @@ -335,7 +388,7 @@ func (m directManager) Close() error { return nil } -func atomicWriteFile(fs wholeFileFS, filename string, data []byte, perm os.FileMode) error { +func (m *directManager) atomicWriteFile(fs wholeFileFS, filename string, data []byte, perm os.FileMode) error { var randBytes [12]byte if _, err := rand.Read(randBytes[:]); err != nil { return fmt.Errorf("atomicWriteFile: %w", err) @@ -347,7 +400,7 @@ func atomicWriteFile(fs wholeFileFS, filename string, data []byte, perm os.FileM if err := fs.WriteFile(tmpName, data, perm); err != nil { return fmt.Errorf("atomicWriteFile: %w", err) } - return fs.Rename(tmpName, filename) + return m.rename(tmpName, filename) } // wholeFileFS is a high-level file system abstraction designed just for use @@ -359,6 +412,7 @@ type wholeFileFS interface { Rename(oldName, newName string) error Remove(name string) error ReadFile(name string) ([]byte, error) + Truncate(name string) error WriteFile(name string, contents []byte, perm os.FileMode) error } @@ -391,6 +445,10 @@ func (fs directFS) ReadFile(name string) ([]byte, error) { return ioutil.ReadFile(fs.path(name)) } +func (fs directFS) Truncate(name string) error { + return os.Truncate(fs.path(name), 0) +} + func (fs directFS) WriteFile(name string, contents []byte, perm os.FileMode) error { return ioutil.WriteFile(fs.path(name), contents, perm) } diff --git a/net/dns/direct_test.go b/net/dns/direct_test.go index dc090f8fc..07147b669 100644 --- a/net/dns/direct_test.go +++ b/net/dns/direct_test.go @@ -5,31 +5,65 @@ package dns import ( - "io/ioutil" + "errors" + "fmt" + "io/fs" "os" "path/filepath" + "strings" + "syscall" "testing" "inet.af/netaddr" "tailscale.com/util/dnsname" ) -func TestSetDNS(t *testing.T) { - const orig = "nameserver 9.9.9.9 # orig" +func TestDirectManager(t *testing.T) { tmp := t.TempDir() - resolvPath := filepath.Join(tmp, "etc", "resolv.conf") - backupPath := filepath.Join(tmp, "etc", "resolv.pre-tailscale-backup.conf") - - if err := os.MkdirAll(filepath.Dir(resolvPath), 0777); err != nil { + if err := os.MkdirAll(filepath.Join(tmp, "etc"), 0700); err != nil { t.Fatal(err) } - if err := ioutil.WriteFile(resolvPath, []byte(orig), 0644); err != nil { + testDirect(t, directFS{prefix: tmp}) +} + +type boundResolvConfFS struct { + directFS +} + +func (fs boundResolvConfFS) Rename(old, new string) error { + if old == "/etc/resolv.conf" || new == "/etc/resolv.conf" { + return errors.New("cannot move to/from /etc/resolv.conf") + } + return fs.directFS.Rename(old, new) +} + +func (fs boundResolvConfFS) Remove(name string) error { + if name == "/etc/resolv.conf" { + return errors.New("cannot remove /etc/resolv.conf") + } + return fs.directFS.Remove(name) +} + +func TestDirectBrokenRename(t *testing.T) { + tmp := t.TempDir() + if err := os.MkdirAll(filepath.Join(tmp, "etc"), 0700); err != nil { + t.Fatal(err) + } + testDirect(t, boundResolvConfFS{directFS{prefix: tmp}}) +} + +func testDirect(t *testing.T, fs wholeFileFS) { + const orig = "nameserver 9.9.9.9 # orig" + resolvPath := "/etc/resolv.conf" + backupPath := "/etc/resolv.pre-tailscale-backup.conf" + + if err := fs.WriteFile(resolvPath, []byte(orig), 0644); err != nil { t.Fatal(err) } readFile := func(t *testing.T, path string) string { t.Helper() - b, err := ioutil.ReadFile(path) + b, err := fs.ReadFile(path) if err != nil { t.Fatal(err) } @@ -39,12 +73,12 @@ func TestSetDNS(t *testing.T) { if got := readFile(t, resolvPath); got != orig { t.Fatalf("resolv.conf:\n%s, want:\n%s", got, orig) } - if _, err := os.Stat(backupPath); !os.IsNotExist(err) { + if _, err := fs.Stat(backupPath); !os.IsNotExist(err) { t.Fatalf("resolv.conf backup: want it to be gone but: %v", err) } } - m := directManager{fs: directFS{prefix: tmp}} + m := directManager{logf: t.Logf, fs: fs} if err := m.SetDNS(OSConfig{ Nameservers: []netaddr.IP{netaddr.MustParseIP("8.8.8.8"), netaddr.MustParseIP("8.8.4.4")}, SearchDomains: []dnsname.FQDN{"ts.net.", "ts-dns.test."}, @@ -81,3 +115,26 @@ func TestSetDNS(t *testing.T) { } assertBaseState(t) } + +type brokenRemoveFS struct { + directFS +} + +func (b brokenRemoveFS) Rename(old, new string) error { + return errors.New("nyaaah I'm a silly container!") +} + +func (b brokenRemoveFS) Remove(name string) error { + if strings.Contains(name, "/etc/resolv.conf") { + return fmt.Errorf("Faking remove failure: %q", &fs.PathError{Err: syscall.EBUSY}) + } + return b.directFS.Remove(name) +} + +func TestDirectBrokenRemove(t *testing.T) { + tmp := t.TempDir() + if err := os.MkdirAll(filepath.Join(tmp, "etc"), 0700); err != nil { + t.Fatal(err) + } + testDirect(t, brokenRemoveFS{directFS{prefix: tmp}}) +} diff --git a/net/dns/manager_freebsd.go b/net/dns/manager_freebsd.go index 20bfbb87c..cdcab5bab 100644 --- a/net/dns/manager_freebsd.go +++ b/net/dns/manager_freebsd.go @@ -15,7 +15,7 @@ func NewOSConfigurator(logf logger.Logf, _ string) (OSConfigurator, error) { bs, err := ioutil.ReadFile("/etc/resolv.conf") if os.IsNotExist(err) { - return newDirectManager(), nil + return newDirectManager(logf), nil } if err != nil { return nil, fmt.Errorf("reading /etc/resolv.conf: %w", err) @@ -25,16 +25,16 @@ func NewOSConfigurator(logf logger.Logf, _ string) (OSConfigurator, error) { case "resolvconf": switch resolvconfStyle() { case "": - return newDirectManager(), nil + return newDirectManager(logf), nil case "debian": return newDebianResolvconfManager(logf) case "openresolv": return newOpenresolvManager() default: logf("[unexpected] got unknown flavor of resolvconf %q, falling back to direct manager", resolvconfStyle()) - return newDirectManager(), nil + return newDirectManager(logf), nil } default: - return newDirectManager(), nil + return newDirectManager(logf), nil } } diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go index fd701a83b..6748a8881 100644 --- a/net/dns/manager_linux.go +++ b/net/dns/manager_linux.go @@ -40,7 +40,7 @@ func NewOSConfigurator(logf logger.Logf, interfaceName string) (ret OSConfigurat } switch mode { case "direct": - return newDirectManagerOnFS(env.fs), nil + return newDirectManagerOnFS(logf, env.fs), nil case "systemd-resolved": return newResolvedManager(logf, interfaceName) case "network-manager": @@ -51,7 +51,7 @@ func NewOSConfigurator(logf logger.Logf, interfaceName string) (ret OSConfigurat return newOpenresolvManager() default: logf("[unexpected] detected unknown DNS mode %q, using direct manager as last resort", mode) - return newDirectManagerOnFS(env.fs), nil + return newDirectManagerOnFS(logf, env.fs), nil } } diff --git a/net/dns/manager_linux_test.go b/net/dns/manager_linux_test.go index d634de9a3..564a0ace9 100644 --- a/net/dns/manager_linux_test.go +++ b/net/dns/manager_linux_test.go @@ -186,8 +186,20 @@ func (m memFS) ReadFile(name string) ([]byte, error) { panic("TODO") } -func (fs memFS) WriteFile(name string, contents []byte, perm os.FileMode) error { - fs[name] = string(contents) +func (m memFS) Truncate(name string) error { + v, ok := m[name] + if !ok { + return fs.ErrNotExist + } + if s, ok := v.(string); ok { + m[name] = s[:0] + } + + return nil +} + +func (m memFS) WriteFile(name string, contents []byte, perm os.FileMode) error { + m[name] = string(contents) return nil } diff --git a/net/dns/manager_openbsd.go b/net/dns/manager_openbsd.go index 9d655d305..2889bbf0b 100644 --- a/net/dns/manager_openbsd.go +++ b/net/dns/manager_openbsd.go @@ -6,6 +6,6 @@ import "tailscale.com/types/logger" -func NewOSConfigurator(logger.Logf, string) (OSConfigurator, error) { - return newDirectManager(), nil +func NewOSConfigurator(logf logger.Logf, _ string) (OSConfigurator, error) { + return newDirectManager(logf), nil } diff --git a/net/dns/resolved.go b/net/dns/resolved.go index f4f108a7b..aa4448a40 100644 --- a/net/dns/resolved.go +++ b/net/dns/resolved.go @@ -68,7 +68,7 @@ func isResolvedActive() bool { return false } - config, err := newDirectManager().readResolvFile(resolvConf) + config, err := newDirectManager(logger.Discard).readResolvFile(resolvConf) if err != nil { return false } diff --git a/net/dns/wsl_windows.go b/net/dns/wsl_windows.go index e4e5bcf5f..9706af04a 100644 --- a/net/dns/wsl_windows.go +++ b/net/dns/wsl_windows.go @@ -99,9 +99,9 @@ func (wm *wslManager) SetDNS(cfg OSConfig) error { } else if len(distros) == 0 { return nil } - managers := make(map[string]directManager) + managers := make(map[string]*directManager) for _, distro := range distros { - managers[distro] = newDirectManagerOnFS(wslFS{ + managers[distro] = newDirectManagerOnFS(wm.logf, wslFS{ user: "root", distro: distro, }) @@ -141,7 +141,7 @@ func (wm *wslManager) SetDNS(cfg OSConfig) error { // setWSLConf attempts to disable generateResolvConf in each WSL2 linux. // If any are changed, it reports true. -func (wm *wslManager) setWSLConf(managers map[string]directManager) (changed bool) { +func (wm *wslManager) setWSLConf(managers map[string]*directManager) (changed bool) { for distro, m := range managers { b, err := m.fs.ReadFile(wslConf) if err != nil && !os.IsNotExist(err) { @@ -189,6 +189,8 @@ func (fs wslFS) Rename(oldName, newName string) error { } func (fs wslFS) Remove(name string) error { return wslRun(fs.cmd("rm", "--", name)) } +func (fs wslFS) Truncate(name string) error { return fs.WriteFile(name, nil, 0644) } + func (fs wslFS) ReadFile(name string) ([]byte, error) { b, err := wslCombinedOutput(fs.cmd("cat", "--", name)) if ee, _ := err.(*exec.ExitError); ee != nil && ee.ExitCode() == 1 {