diff --git a/client/internal/dns/file_parser_unix.go b/client/internal/dns/file_parser_unix.go index 50ba74c0c..2ef05e3fd 100644 --- a/client/internal/dns/file_parser_unix.go +++ b/client/internal/dns/file_parser_unix.go @@ -35,11 +35,7 @@ func parseBackupResolvConf() (*resolvConf, error) { } func parseResolvConfFile(resolvConfFile string) (*resolvConf, error) { - rconf := &resolvConf{ - searchDomains: make([]string, 0), - nameServers: make([]netip.Addr, 0), - others: make([]string, 0), - } + rconf := newResolvConf() file, err := os.Open(resolvConfFile) if err != nil { @@ -56,56 +52,74 @@ func parseResolvConfFile(resolvConfFile string) (*resolvConf, error) { return rconf, fmt.Errorf("failed to read %s file: %w", resolvConfFile, err) } + return parseResolvConf(cur) +} + +// parseResolvConf parses resolv.conf content. The returned config is never nil, so callers can +// fall back to its empty lists on error. +func parseResolvConf(cur []byte) (*resolvConf, error) { + rconf := newResolvConf() + if len(cur) == 0 { return rconf, fmt.Errorf("file is empty") } for _, line := range strings.Split(string(cur), "\n") { - line = strings.TrimSpace(line) - - if strings.HasPrefix(line, "#") { - continue - } - - if strings.HasPrefix(line, "domain") { - continue - } - - if strings.HasPrefix(line, "options") && strings.Contains(line, "rotate") { - line = strings.ReplaceAll(line, "rotate", "") - splitLines := strings.Fields(line) - if len(splitLines) == 1 { - continue - } - line = strings.Join(splitLines, " ") - } - - if strings.HasPrefix(line, "search") { - splitLines := strings.Fields(line) - if len(splitLines) < 2 { - continue - } - - rconf.searchDomains = splitLines[1:] - continue - } - - if strings.HasPrefix(line, "nameserver") { - splitLines := strings.Fields(line) - if len(splitLines) != 2 { - continue - } - if addr, err := netip.ParseAddr(splitLines[1]); err == nil { - rconf.nameServers = append(rconf.nameServers, addr.Unmap()) - } else { - log.Warnf("invalid nameserver address in resolv.conf: %s, skipping", splitLines[1]) - } - continue - } - - if line != "" { - rconf.others = append(rconf.others, line) - } + parseResolvConfLine(rconf, strings.TrimSpace(line)) } return rconf, nil } + +func parseResolvConfLine(rconf *resolvConf, line string) { + if strings.HasPrefix(line, "#") { + return + } + + if strings.HasPrefix(line, "domain") { + return + } + + if strings.HasPrefix(line, "options") && strings.Contains(line, "rotate") { + line = strings.ReplaceAll(line, "rotate", "") + splitLines := strings.Fields(line) + if len(splitLines) == 1 { + return + } + line = strings.Join(splitLines, " ") + } + + if strings.HasPrefix(line, "search") { + splitLines := strings.Fields(line) + if len(splitLines) < 2 { + return + } + + rconf.searchDomains = splitLines[1:] + return + } + + if strings.HasPrefix(line, "nameserver") { + splitLines := strings.Fields(line) + if len(splitLines) != 2 { + return + } + if addr, err := netip.ParseAddr(splitLines[1]); err == nil { + rconf.nameServers = append(rconf.nameServers, addr.Unmap()) + } else { + log.Warnf("invalid nameserver address in resolv.conf: %s, skipping", splitLines[1]) + } + return + } + + if line != "" { + rconf.others = append(rconf.others, line) + } +} + +func newResolvConf() *resolvConf { + return &resolvConf{ + searchDomains: make([]string, 0), + nameServers: make([]netip.Addr, 0), + others: make([]string, 0), + } +} diff --git a/client/internal/dns/file_repair_unix.go b/client/internal/dns/file_repair_unix.go index 0846dbf38..57255fb56 100644 --- a/client/internal/dns/file_repair_unix.go +++ b/client/internal/dns/file_repair_unix.go @@ -3,7 +3,9 @@ package dns import ( + "bytes" "net/netip" + "os" "path" "path/filepath" "sync" @@ -23,7 +25,8 @@ var ( } ) -type repairConfFn func([]string, netip.Addr, *resolvConf, *statemanager.Manager) error +// repairConfFn writes the NetBird-managed resolv.conf and returns the exact content it wrote. +type repairConfFn func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) type repair struct { operationFile string @@ -43,7 +46,9 @@ func newRepair(operationFile string, updateFn repairConfFn) *repair { } } -func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP netip.Addr, stateManager *statemanager.Manager) { +// watchFileChanges keeps resolv.conf pointing at NetBird. written is the content NetBird last +// wrote to the file, used to recognize the echo of its own writes. +func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP netip.Addr, written []byte, stateManager *statemanager.Manager) { if f.inotify != nil { return } @@ -54,54 +59,80 @@ func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP netip log.Errorf("failed to start inotify watcher for resolv.conf: %s", err) return } + + if err := inotify.Add(f.watchDir); err != nil { + log.Errorf("failed to add inotify watch for resolv.conf: %s", err) + if err := inotify.Close(); err != nil { + log.Debugf("failed to close resolv.conf inotify: %v", err) + } + return + } f.inotify = inotify f.inotifyWg.Add(1) go func() { defer f.inotifyWg.Done() - for event := range f.inotify.Events { - if !f.isEventRelevant(event) { - continue - } - log.Tracef("%s changed, check if it is broken", f.operationFile) - - rConf, err := parseResolvConfFile(f.operationFile) - if err != nil { - log.Warnf("failed to parse resolv conf: %s", err) - continue - } - - log.Debugf("check resolv.conf parameters: %s", rConf) - if !isNbParamsMissing(nbSearchDomains, nbNameserverIP, rConf) { - log.Tracef("resolv.conf still correct, skip the update") - continue - } - log.Info("broken params in resolv.conf, repairing it...") - - err = f.inotify.Remove(f.watchDir) - if err != nil { - log.Errorf("failed to rm inotify watch for resolv.conf: %s", err) - } - - err = f.updateFn(nbSearchDomains, nbNameserverIP, rConf, stateManager) - if err != nil { - log.Errorf("failed to repair resolv.conf: %v", err) - } - - err = f.inotify.Add(f.watchDir) - if err != nil { - log.Errorf("failed to re-add inotify watch for resolv.conf: %s", err) - return + // A change that landed before the watch was armed produced no event. + lastWritten := f.checkAndRepair(nbSearchDomains, nbNameserverIP, written, stateManager) + for { + select { + case event, ok := <-inotify.Events: + if !ok { + return + } + if !f.isEventRelevant(event) { + continue + } + case err, ok := <-inotify.Errors: + if !ok { + return + } + // fsnotify stops delivering events until an error is received, and an + // error such as a queue overflow means events were dropped. + log.Warnf("resolv.conf inotify error, checking the file: %v", err) } + lastWritten = f.checkAndRepair(nbSearchDomains, nbNameserverIP, lastWritten, stateManager) } }() +} - err = f.inotify.Add(f.watchDir) +// checkAndRepair rewrites resolv.conf when NetBird's parameters are missing from it and returns +// the content NetBird last wrote. The watch stays armed while repairing, so a third-party write +// racing the repair still raises an event; the echo of NetBird's own write is skipped by content. +func (f *repair) checkAndRepair(nbSearchDomains []string, nbNameserverIP netip.Addr, lastWritten []byte, stateManager *statemanager.Manager) []byte { + content, err := os.ReadFile(f.operationFile) if err != nil { - log.Errorf("failed to add inotify watch for resolv.conf: %s", err) - return + log.Warnf("failed to read resolv.conf: %s", err) + return lastWritten } + + if bytes.Equal(content, lastWritten) { + log.Tracef("%s still holds the NetBird managed content, skip the check", f.operationFile) + return lastWritten + } + + log.Tracef("%s changed, check if it is broken", f.operationFile) + + rConf, err := parseResolvConf(content) + if err != nil { + log.Warnf("failed to parse resolv conf: %s", err) + return lastWritten + } + + log.Debugf("check resolv.conf parameters: %s", rConf) + if !isNbParamsMissing(nbSearchDomains, nbNameserverIP, rConf) { + log.Tracef("resolv.conf still correct, skip the update") + return lastWritten + } + log.Info("broken params in resolv.conf, repairing it...") + + written, err := f.updateFn(nbSearchDomains, nbNameserverIP, rConf, stateManager) + if err != nil { + log.Errorf("failed to repair resolv.conf: %v", err) + return lastWritten + } + return written } func (f *repair) stopWatchFileChanges() { diff --git a/client/internal/dns/file_repair_unix_test.go b/client/internal/dns/file_repair_unix_test.go index f22081307..c771ff641 100644 --- a/client/internal/dns/file_repair_unix_test.go +++ b/client/internal/dns/file_repair_unix_test.go @@ -4,12 +4,19 @@ package dns import ( "context" + "fmt" "net/netip" "os" "path/filepath" + "strings" + "sync/atomic" "testing" "time" + "github.com/fsnotify/fsnotify" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/client/internal/statemanager" "github.com/netbirdio/netbird/util" ) @@ -106,14 +113,14 @@ nameserver 8.8.8.8`, var changed bool ctx, cancel := context.WithTimeout(context.Background(), time.Second) - updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) error { + updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) { changed = true cancel() - return nil + return nil, nil } r := newRepair(operationFile, updateFn) - r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil) + r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil, nil) err = os.WriteFile(operationFile, []byte(tt.touchedConfContent), 0755) if err != nil { @@ -153,14 +160,14 @@ searchdomain netbird.cloud something` var changed bool ctx, cancel := context.WithTimeout(context.Background(), time.Second) - updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) error { + updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) { changed = true cancel() - return nil + return nil, nil } r := newRepair(tmpLink, updateFn) - r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil) + r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), nil, nil) err = os.WriteFile(tmpLink, []byte(modifyContent), 0755) if err != nil { @@ -175,3 +182,149 @@ searchdomain netbird.cloud something` t.Errorf("unexpected result: want: %v, got: %v", true, false) } } + +// systemd-resolved can rewrite resolv.conf twice in a row, e.g. on restart. When the second write +// lands right after NetBird's repair, the watcher must still see it and repair again; otherwise the +// host silently stays on the upstream resolver until the next unrelated change of the file. +func Test_repairCatchesWriteRacingRepair(t *testing.T) { + good := "nameserver 10.0.0.1\nsearch netbird.cloud\n" + upstream := "nameserver 8.8.8.8\nsearch .\n" + operationFile := filepath.Join(t.TempDir(), "resolv.conf") + require.NoError(t, os.WriteFile(operationFile, []byte(good), 0644)) + + var calls atomic.Int32 + updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) { + n := calls.Add(1) + if err := os.WriteFile(operationFile, []byte(good), 0644); err != nil { + return nil, err + } + if n == 1 { + // the third party's second write lands right after the repair + if err := writeFileAtomic(operationFile, upstream); err != nil { + return nil, err + } + } + return []byte(good), nil + } + + r := newRepair(operationFile, updateFn) + r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), []byte(good), nil) + t.Cleanup(r.stopWatchFileChanges) + + require.NoError(t, writeFileAtomic(operationFile, upstream)) + + assert.Eventually(t, func() bool { + content, err := os.ReadFile(operationFile) + return err == nil && string(content) == good + }, 2*time.Second, 10*time.Millisecond, "resolv.conf should be repaired after the racing write") + assert.GreaterOrEqual(t, calls.Load(), int32(2), "the racing write should trigger a second repair") +} + +// A change made between NetBird's write and the arming of the watch produces no event, so the +// watcher must check the file as soon as it starts. +func Test_repairChecksFileWhenArmed(t *testing.T) { + good := "nameserver 10.0.0.1\nsearch netbird.cloud\n" + operationFile := filepath.Join(t.TempDir(), "resolv.conf") + require.NoError(t, os.WriteFile(operationFile, []byte("nameserver 8.8.8.8\n"), 0644)) + + repaired := make(chan struct{}, 1) + updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) { + select { + case repaired <- struct{}{}: + default: + } + return []byte(good), os.WriteFile(operationFile, []byte(good), 0644) + } + + r := newRepair(operationFile, updateFn) + r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), []byte(good), nil) + t.Cleanup(r.stopWatchFileChanges) + + select { + case <-repaired: + case <-time.After(2 * time.Second): + t.Fatal("watcher should repair a file changed before it was armed") + } +} + +// When NetBird has more search domains than resolv.conf can hold, its own write lacks some of them +// and fails the parameter check. The watcher must recognize that write as its own and not repair +// it again in a loop. +func Test_repairNoLoopOnTruncatedSearchDomains(t *testing.T) { + nbDomains := make([]string, 0, fileMaxNumberOfSearchDomains+2) + for i := range fileMaxNumberOfSearchDomains + 2 { + nbDomains = append(nbDomains, fmt.Sprintf("d%d.netbird.cloud", i)) + } + truncated := fmt.Sprintf("nameserver 10.0.0.1\nsearch %s\n", strings.Join(nbDomains[:fileMaxNumberOfSearchDomains], " ")) + + operationFile := filepath.Join(t.TempDir(), "resolv.conf") + require.NoError(t, os.WriteFile(operationFile, []byte(truncated), 0644)) + + var calls atomic.Int32 + updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) { + calls.Add(1) + return []byte(truncated), os.WriteFile(operationFile, []byte(truncated), 0644) + } + + r := newRepair(operationFile, updateFn) + r.watchFileChanges(nbDomains, netip.MustParseAddr("10.0.0.1"), []byte(truncated), nil) + t.Cleanup(r.stopWatchFileChanges) + + require.NoError(t, writeFileAtomic(operationFile, "nameserver 8.8.8.8\n")) + + require.Eventually(t, func() bool { return calls.Load() >= 1 }, 2*time.Second, 10*time.Millisecond, + "the third-party write should be repaired") + time.Sleep(500 * time.Millisecond) + assert.Equal(t, int32(1), calls.Load(), "NetBird's own truncated write should not trigger another repair") +} + +// fsnotify blocks its reader until an error is received, and an error such as a queue overflow +// means events were dropped. The watcher must consume errors and check the file, so it keeps working +// and catches a change whose event was lost. +func Test_repairChecksFileOnWatcherError(t *testing.T) { + good := "nameserver 10.0.0.1\nsearch netbird.cloud\n" + operationFile := filepath.Join(t.TempDir(), "resolv.conf") + require.NoError(t, os.WriteFile(operationFile, []byte(good), 0644)) + + // A hard link in another directory changes the file without an event in the watched one, + // standing in for an event the kernel dropped. + alias := filepath.Join(t.TempDir(), "alias") + require.NoError(t, os.Link(operationFile, alias)) + + repaired := make(chan struct{}, 1) + updateFn := func([]string, netip.Addr, *resolvConf, *statemanager.Manager) ([]byte, error) { + select { + case repaired <- struct{}{}: + default: + } + return []byte(good), os.WriteFile(operationFile, []byte(good), 0644) + } + + r := newRepair(operationFile, updateFn) + r.watchFileChanges([]string{"netbird.cloud"}, netip.MustParseAddr("10.0.0.1"), []byte(good), nil) + t.Cleanup(r.stopWatchFileChanges) + + require.NoError(t, os.WriteFile(alias, []byte("nameserver 8.8.8.8\n"), 0644)) + + select { + case r.inotify.Errors <- fsnotify.ErrEventOverflow: + case <-time.After(2 * time.Second): + t.Fatal("watcher should consume inotify errors") + } + + select { + case <-repaired: + case <-time.After(2 * time.Second): + t.Fatal("watcher should check the file after an inotify error") + } +} + +// writeFileAtomic replaces the file the way systemd-resolved does: write a temporary file in the same +// directory, then rename it over the target. +func writeFileAtomic(target, content string) error { + tmp := target + ".tmp" + if err := os.WriteFile(tmp, []byte(content), 0644); err != nil { + return err + } + return os.Rename(tmp, target) +} diff --git a/client/internal/dns/file_unix.go b/client/internal/dns/file_unix.go index 45e621443..52f221d10 100644 --- a/client/internal/dns/file_unix.go +++ b/client/internal/dns/file_unix.go @@ -61,11 +61,11 @@ func (f *fileConfigurator) applyDNSConfig(config HostDNSConfig, stateManager *st f.repair.stopWatchFileChanges() - err = f.updateConfig(nbSearchDomains, f.nbNameserverIP, resolvConf, stateManager) + written, err := f.updateConfig(nbSearchDomains, f.nbNameserverIP, resolvConf, stateManager) if err != nil { return err } - f.repair.watchFileChanges(nbSearchDomains, f.nbNameserverIP, stateManager) + f.repair.watchFileChanges(nbSearchDomains, f.nbNameserverIP, written, stateManager) return nil } @@ -74,7 +74,7 @@ func (f *fileConfigurator) getOriginalNameservers() []netip.Addr { return f.originalNameservers } -func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP netip.Addr, cfg *resolvConf, stateManager *statemanager.Manager) error { +func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP netip.Addr, cfg *resolvConf, stateManager *statemanager.Manager) ([]byte, error) { searchDomainList := mergeSearchDomains(nbSearchDomains, cfg.searchDomains) buf := prepareResolvConfContent( @@ -90,7 +90,7 @@ func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP if restoreErr != nil { log.Errorf("attempt to restore default file failed with error: %s", err) } - return fmt.Errorf("creating resolver file %s. Error: %w", defaultResolvConfPath, err) + return nil, fmt.Errorf("creating resolver file %s. Error: %w", defaultResolvConfPath, err) } log.Infof("created a NetBird managed %s file with the DNS settings. Added %d search domains. Search list: %s", defaultResolvConfPath, len(searchDomainList), searchDomainList) @@ -100,7 +100,7 @@ func (f *fileConfigurator) updateConfig(nbSearchDomains []string, nbNameserverIP log.Errorf("failed to create unclean shutdown resolv.conf backup: %s", err) } - return nil + return buf.Bytes(), nil } func (f *fileConfigurator) restoreHostDNS() error {