From a2f2a6e21a213f459300e9e53565816a8284e011 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 2 Feb 2024 17:54:33 +0100 Subject: [PATCH] Fix/resolv parser (#1520) fix an issue In case if the original resolv.conf file is empty, then it can cause a nil pointer --- client/internal/dns/file_parser_linux.go | 18 ++-- client/internal/dns/file_parser_linux_test.go | 83 ++++++++++++------- client/internal/dns/file_repair_linux.go | 22 +++-- client/internal/dns/file_repair_linux_test.go | 47 ++++++++++- 4 files changed, 124 insertions(+), 46 deletions(-) diff --git a/client/internal/dns/file_parser_linux.go b/client/internal/dns/file_parser_linux.go index a0115ecd3..95e1ddb54 100644 --- a/client/internal/dns/file_parser_linux.go +++ b/client/internal/dns/file_parser_linux.go @@ -33,9 +33,15 @@ func parseBackupResolvConf() (*resolvConf, error) { } func parseResolvConfFile(resolvConfFile string) (*resolvConf, error) { + rconf := &resolvConf{ + searchDomains: make([]string, 0), + nameServers: make([]string, 0), + others: make([]string, 0), + } + file, err := os.Open(resolvConfFile) if err != nil { - return nil, fmt.Errorf("failed to open %s file: %w", resolvConfFile, err) + return rconf, fmt.Errorf("failed to open %s file: %w", resolvConfFile, err) } defer func() { if err := file.Close(); err != nil { @@ -45,17 +51,11 @@ func parseResolvConfFile(resolvConfFile string) (*resolvConf, error) { cur, err := os.ReadFile(resolvConfFile) if err != nil { - return nil, fmt.Errorf("failed to read %s file: %w", resolvConfFile, err) + return rconf, fmt.Errorf("failed to read %s file: %w", resolvConfFile, err) } if len(cur) == 0 { - return nil, fmt.Errorf("file is empty") - } - - rconf := &resolvConf{ - searchDomains: make([]string, 0), - nameServers: make([]string, 0), - others: make([]string, 0), + return rconf, fmt.Errorf("file is empty") } for _, line := range strings.Split(string(cur), "\n") { diff --git a/client/internal/dns/file_parser_linux_test.go b/client/internal/dns/file_parser_linux_test.go index 5c22c88e6..180ad2f9d 100644 --- a/client/internal/dns/file_parser_linux_test.go +++ b/client/internal/dns/file_parser_linux_test.go @@ -3,8 +3,8 @@ package dns import ( - "fmt" "os" + "path/filepath" "testing" ) @@ -75,40 +75,13 @@ options debug expectedNS: []string{"192.168.2.1", "100.81.99.197"}, expectedOther: []string{"options debug"}, }, - { - input: `# This is /run/systemd/resolve/resolv.conf managed by man:systemd-resolved(8). -# Do not edit. -# -# This file might be symlinked as /etc/resolv.conf. If you're looking at -# /etc/resolv.conf and seeing this text, you have followed the symlink. -# -# This is a dynamic resolv.conf file for connecting local clients directly to -# all known uplink DNS servers. This file lists all configured search domains. -# -# Third party programs should typically not access this file directly, but only -# through the symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a -# different way, replace this symlink by a static file or a different symlink. -# -# See man:systemd-resolved.service(8) for details about the supported modes of -# operation for /etc/resolv.conf. - -nameserver 192.168.2.1 -nameserver 100.81.99.197 -search netbird.cloud -options debug -options edns0 trust-ad -`, - expectedSearch: []string{"netbird.cloud"}, - expectedNS: []string{"192.168.2.1", "100.81.99.197"}, - expectedOther: []string{"options debug", "options edns0 trust-ad"}, - }, } for _, testCase := range testCases { testCase := testCase t.Run("test", func(t *testing.T) { t.Parallel() - tmpResolvConf := fmt.Sprintf("%s/%s", t.TempDir(), "resolv.conf") + tmpResolvConf := filepath.Join(t.TempDir(), "resolv.conf") err := os.WriteFile(tmpResolvConf, []byte(testCase.input), 0644) if err != nil { t.Fatal(err) @@ -147,3 +120,55 @@ func compareLists(search []string, search2 []string) bool { } return true } + +func Test_emptyFile(t *testing.T) { + cfg, err := parseResolvConfFile("/tmp/nothing") + if err == nil { + t.Errorf("expected error, got nil") + } + if len(cfg.others) != 0 || len(cfg.searchDomains) != 0 || len(cfg.nameServers) != 0 { + t.Errorf("expected empty config, got %v", cfg) + } +} + +func Test_symlink(t *testing.T) { + input := `# This is /run/systemd/resolve/resolv.conf managed by man:systemd-resolved(8). +# Do not edit. +# +# This file might be symlinked as /etc/resolv.conf. If you're looking at +# /etc/resolv.conf and seeing this text, you have followed the symlink. +# +# This is a dynamic resolv.conf file for connecting local clients directly to +# all known uplink DNS servers. This file lists all configured search domains. +# +# Third party programs should typically not access this file directly, but only +# through the symlink at /etc/resolv.conf. To manage man:resolv.conf(5) in a +# different way, replace this symlink by a static file or a different symlink. +# +# See man:systemd-resolved.service(8) for details about the supported modes of +# operation for /etc/resolv.conf. + +nameserver 192.168.0.1 +` + + tmpResolvConf := filepath.Join(t.TempDir(), "resolv.conf") + err := os.WriteFile(tmpResolvConf, []byte(input), 0644) + if err != nil { + t.Fatal(err) + } + + tmpLink := filepath.Join(t.TempDir(), "symlink") + err = os.Symlink(tmpResolvConf, tmpLink) + if err != nil { + t.Fatal(err) + } + + cfg, err := parseResolvConfFile(tmpLink) + if err != nil { + t.Fatal(err) + } + + if len(cfg.nameServers) != 1 { + t.Errorf("unexpected resolv.conf content: %v", cfg) + } +} diff --git a/client/internal/dns/file_repair_linux.go b/client/internal/dns/file_repair_linux.go index 7be2a08f6..cbdda5e9e 100644 --- a/client/internal/dns/file_repair_linux.go +++ b/client/internal/dns/file_repair_linux.go @@ -3,8 +3,8 @@ package dns import ( - "fmt" "path" + "path/filepath" "sync" "github.com/fsnotify/fsnotify" @@ -32,9 +32,10 @@ type repair struct { } func newRepair(operationFile string, updateFn repairConfFn) *repair { + targetFile := targetFile(operationFile) return &repair{ - operationFile: operationFile, - watchDir: path.Dir(operationFile), + operationFile: targetFile, + watchDir: path.Dir(targetFile), updateFn: updateFn, } } @@ -44,7 +45,7 @@ func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP strin return } - log.Infof("start to watch resolv.conf") + log.Infof("start to watch resolv.conf: %s", f.operationFile) inotify, err := fsnotify.NewWatcher() if err != nil { log.Errorf("failed to start inotify watcher for resolv.conf: %s", err) @@ -60,7 +61,7 @@ func (f *repair) watchFileChanges(nbSearchDomains []string, nbNameserverIP strin continue } - log.Tracef("resolv.conf changed, check if it is broken") + log.Tracef("%s changed, check if it is broken", f.operationFile) rConf, err := parseResolvConfFile(f.operationFile) if err != nil { @@ -124,8 +125,7 @@ func (f *repair) isEventRelevant(event fsnotify.Event) bool { return false } - operationFileSymlink := fmt.Sprintf("%s~", f.operationFile) - if event.Name == f.operationFile || event.Name == operationFileSymlink { + if event.Name == f.operationFile { return true } return false @@ -149,3 +149,11 @@ func isNbParamsMissing(nbSearchDomains []string, nbNameserverIP string, rConf *r return false } + +func targetFile(filename string) string { + target, err := filepath.EvalSymlinks(filename) + if err != nil { + log.Errorf("evarl err: %s", err) + } + return target +} diff --git a/client/internal/dns/file_repair_linux_test.go b/client/internal/dns/file_repair_linux_test.go index 6c20db716..4e27f46ba 100644 --- a/client/internal/dns/file_repair_linux_test.go +++ b/client/internal/dns/file_repair_linux_test.go @@ -5,6 +5,7 @@ package dns import ( "context" "os" + "path/filepath" "testing" "time" @@ -126,5 +127,49 @@ nameserver 8.8.8.8`, } }) } - +} + +func Test_newRepairSymlink(t *testing.T) { + resolvConfContent := ` +nameserver 10.0.0.1 +nameserver 8.8.8.8 +searchdomain netbird.cloud something` + + modifyContent := `nameserver 8.8.8.8` + + tmpResolvConf := filepath.Join(t.TempDir(), "resolv.conf") + err := os.WriteFile(tmpResolvConf, []byte(resolvConfContent), 0644) + if err != nil { + t.Fatal(err) + } + + tmpLink := filepath.Join(t.TempDir(), "symlink") + err = os.Symlink(tmpResolvConf, tmpLink) + if err != nil { + t.Fatal(err) + } + + var changed bool + ctx, cancel := context.WithTimeout(context.Background(), time.Second) + updateFn := func([]string, string, *resolvConf) error { + changed = true + cancel() + return nil + } + + r := newRepair(tmpLink, updateFn) + r.watchFileChanges([]string{"netbird.cloud"}, "10.0.0.1") + + err = os.WriteFile(tmpLink, []byte(modifyContent), 0755) + if err != nil { + t.Fatalf("failed to write out resolv.conf: %s", err) + } + + <-ctx.Done() + + r.stopWatchFileChanges() + + if changed != true { + t.Errorf("unexpected result: want: %v, got: %v", true, false) + } }