diff --git a/net/dns/openresolv.go b/net/dns/openresolv.go index 2a4ed174e..4239f50e0 100644 --- a/net/dns/openresolv.go +++ b/net/dns/openresolv.go @@ -7,10 +7,14 @@ import ( "bytes" + "errors" "fmt" + "net/netip" "os/exec" + "slices" "strings" + "tailscale.com/net/tsaddr" "tailscale.com/types/logger" ) @@ -39,6 +43,38 @@ func (m openresolvManager) logCmdErr(cmd *exec.Cmd, err error) { m.logf("error running command %s stderr=%q exitCode=%d: %v", commandStr, exerr.Stderr, exerr.ExitCode(), err) } +// openresolvNoSnippetsExitCode is the exit status resolvconf returns when a +// requested config snippet does not exist. Asking for all snippets when none +// are registered returns this status too, because openresolv treats the empty +// result as a missing snippet rather than as an empty list. +const openresolvNoSnippetsExitCode = 2 + +// readSnippets runs resolvconf with the given arguments and returns its stdout. +// An exit status of openresolvNoSnippetsExitCode is not an error: it returns no +// output and a nil error. Other failures are logged and returned. +// +// Callers must pass either "-i" with no arguments or "-l" with explicit snippet +// names. Only those forms produce empty stdout alongside +// openresolvNoSnippetsExitCode; "-i" with snippet names prints the ones that do +// exist and still exits 2, so its output would be silently dropped. +// +// Stderr is excluded from the returned bytes so that diagnostics like "No +// resolv.conf for key foo" are never parsed as snippet names or resolv.conf +// lines. logCmdErr still logs stderr when a command fails. +func (m openresolvManager) readSnippets(args ...string) ([]byte, error) { + cmd := exec.Command("resolvconf", args...) + out, err := cmd.Output() + if err != nil { + if ee, ok := errors.AsType[*exec.ExitError](err); ok && ee.ExitCode() == openresolvNoSnippetsExitCode { + m.logf("[v1] resolvconf %q found no matching config snippets", args) + return nil, nil + } + m.logCmdErr(cmd, err) + return nil, err + } + return out, nil +} + func (m openresolvManager) deleteTailscaleConfig() error { cmd := exec.Command("resolvconf", "-f", "-d", "tailscale") out, err := cmd.CombinedOutput() @@ -75,18 +111,21 @@ func (m openresolvManager) GetBaseConfig() (OSConfig, error) { // List the names of all config snippets openresolv is aware // of. Snippets get listed in priority order (most to least), // which we'll exploit later. - bs, err := exec.Command("resolvconf", "-i").CombinedOutput() + bs, err := m.readSnippets("-i") if err != nil { return OSConfig{}, err } - // Remove the "tailscale" snippet from the list. - args := []string{"-l"} - for f := range strings.SplitSeq(strings.TrimSpace(string(bs)), " ") { - if f == "tailscale" { - continue - } - args = append(args, f) + others := slices.DeleteFunc(strings.Fields(string(bs)), func(f string) bool { + return f == "tailscale" + }) + if len(others) == 0 { + // There are no other snippets, so there is no base config to read. + // Returning early is required, not merely an optimization: a + // "resolvconf -l" with no snippet names lists every snippet, + // including Tailscale's own, which would make quad-100 its own + // upstream. See tailscale/tailscale#20825. + return OSConfig{}, nil } // List all resolvconf snippets except our own, and parse that as @@ -100,14 +139,30 @@ func (m openresolvManager) GetBaseConfig() (OSConfig, error) { // practice, openresolv uses are generally quite limited, and boil // down to 1-2 DHCP leases, for which the correct outcome is a // blended config like the one we produce here. - var buf bytes.Buffer - cmd := exec.Command("resolvconf", args...) - cmd.Stdout = &buf - if err := cmd.Run(); err != nil { - m.logCmdErr(cmd, err) + out, err := m.readSnippets(append([]string{"-l"}, others...)...) + if err != nil { return OSConfig{}, err } - return readResolv(&buf) + cfg, err := readResolv(bytes.NewReader(out)) + if err != nil { + return OSConfig{}, err + } + + // Forwarding to the Tailscale service IPs would make quad-100 send + // queries to itself, in an infinite loop, so drop them if another + // snippet names them. See tailscale/tailscale#7816. + var removed bool + cfg.Nameservers = slices.DeleteFunc(cfg.Nameservers, func(ip netip.Addr) bool { + if ip == tsaddr.TailscaleServiceIP() || ip == tsaddr.TailscaleServiceIPv6() { + removed = true + return true + } + return false + }) + if removed { + m.logf("[v1] dropped Tailscale service IP from openresolv base config") + } + return cfg, nil } func (m openresolvManager) Close() error { diff --git a/net/dns/openresolv_test.go b/net/dns/openresolv_test.go new file mode 100644 index 000000000..f6a03286d --- /dev/null +++ b/net/dns/openresolv_test.go @@ -0,0 +1,194 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +//go:build (linux && !android) || freebsd || openbsd + +package dns + +import ( + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "tailscale.com/util/must" +) + +// fakeResolvconf is the canned behavior of the two read-only resolvconf +// subcommands openresolvManager uses: "-i" to list the names of the registered +// config snippets, and "-l" to dump their contents. +type fakeResolvconf struct { + listOut string // stdout of "resolvconf -i" + listCode int // exit status of "resolvconf -i" + dumpOut string // stdout of "resolvconf -l ..." + dumpCode int // exit status of "resolvconf -l ..." +} + +// install puts f at the front of $PATH as "resolvconf", so that the exec.Command +// calls in openresolv.go find it, and returns the path of the file it appends +// its arguments to, one invocation per line. +// +// The canned stdout reaches the script through files rather than being +// substituted into it, so that no test data has to survive shell quoting. +func (f fakeResolvconf) install(t *testing.T) (argvLog string) { + t.Helper() + + dir := t.TempDir() + writeFile := func(name, content string) string { + path := filepath.Join(dir, name) + must.Do(os.WriteFile(path, []byte(content), 0644)) + return path + } + argvLog = filepath.Join(dir, "argv") + listOut := writeFile("list-out", f.listOut) + dumpOut := writeFile("dump-out", f.dumpOut) + + script := fmt.Sprintf(`#!/bin/sh +printf '%%s\n' "$*" >>%s +case "$1" in +-i) cat %s + # Diagnostics on stderr must never be parsed as snippet names. + echo 'No resolv.conf for key bogus' >&2 + exit %d ;; +-l) cat %s + exit %d ;; +esac +echo "fake resolvconf: unexpected args: $*" >&2 +exit 99 +`, argvLog, listOut, f.listCode, dumpOut, f.dumpCode) + must.Do(os.WriteFile(filepath.Join(dir, "resolvconf"), []byte(script), 0755)) + + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + return argvLog +} + +func TestOpenresolvGetBaseConfig(t *testing.T) { + // The dump openresolv prints for a single snippet belonging to eth0. + const eth0Dump = "# resolv.conf from eth0\nnameserver 192.168.1.1\nsearch lan\n" + + tests := []struct { + name string + resolvconf fakeResolvconf + + wantNameservers []string + wantSearch []string + wantErr bool + // wantArgv is every resolvconf invocation we expect, in order. + wantArgv []string + }{ + { + // The bug in tailscale/tailscale#20825: openresolv exits 2 + // when its key directory exists but is empty. That means + // "no snippets", so we must report an empty base config + // rather than failing the whole DNS reconfiguration. + name: "no_snippets_at_all", + resolvconf: fakeResolvconf{listCode: 2}, + wantArgv: []string{"-i"}, + }, + { + // The other half of #20825: we're the only registered + // snippet, so "resolvconf -l" must not be run at all, lest + // openresolv hand our own config back as our upstream. + name: "only_tailscale_registered", + resolvconf: fakeResolvconf{listOut: "tailscale\n"}, + wantArgv: []string{"-i"}, + }, + { + // openresolv exits 0 with no output when it has no key + // directory yet. Same conclusion, different exit status. + name: "empty_listing_exit_zero", + wantArgv: []string{"-i"}, + }, + { + name: "tailscale_among_others", + resolvconf: fakeResolvconf{ + listOut: "eth0 tailscale wlan0\n", + dumpOut: eth0Dump, + }, + wantNameservers: []string{"192.168.1.1"}, + wantSearch: []string{"lan."}, + // Priority order must be preserved, and our own snippet dropped. + wantArgv: []string{"-i", "-l eth0 wlan0"}, + }, + { + name: "no_tailscale_snippet_yet", + resolvconf: fakeResolvconf{ + listOut: "eth0\n", + dumpOut: eth0Dump, + }, + wantNameservers: []string{"192.168.1.1"}, + wantSearch: []string{"lan."}, + wantArgv: []string{"-i", "-l eth0"}, + }, + { + // Any status other than 2 is a real failure and must be + // reported, so the caller can flag DNS as unhealthy. + name: "listing_fails", + resolvconf: fakeResolvconf{listCode: 1}, + wantErr: true, + wantArgv: []string{"-i"}, + }, + { + // A snippet can be deregistered between the two calls. The + // dump then prints nothing and exits 2; that's an empty base + // config, not a reason to abandon the reconfiguration. + name: "snippet_vanished_before_dump", + resolvconf: fakeResolvconf{ + listOut: "eth0\n", + dumpCode: 2, + }, + wantArgv: []string{"-i", "-l eth0"}, + }, + { + // Defense in depth for tailscale/tailscale#7816: quad-100 + // must never become our own upstream, even if some other + // snippet names it. + name: "quad-100_in_another_snippet", + resolvconf: fakeResolvconf{ + listOut: "eth0\n", + dumpOut: "nameserver 100.100.100.100\nnameserver 192.168.1.1\nnameserver fd7a:115c:a1e0::53\nsearch lan\n", + }, + wantNameservers: []string{"192.168.1.1"}, + wantSearch: []string{"lan."}, + wantArgv: []string{"-i", "-l eth0"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + argvLog := tt.resolvconf.install(t) + m := openresolvManager{t.Logf} + + cfg, err := m.GetBaseConfig() + if gotErr := err != nil; gotErr != tt.wantErr { + t.Fatalf("GetBaseConfig() error = %v, want error = %v", err, tt.wantErr) + } + + var gotNameservers []string + for _, ns := range cfg.Nameservers { + gotNameservers = append(gotNameservers, ns.String()) + } + if !slices.Equal(gotNameservers, tt.wantNameservers) { + t.Errorf("nameservers = %q, want %q", gotNameservers, tt.wantNameservers) + } + + var gotSearch []string + for _, d := range cfg.SearchDomains { + gotSearch = append(gotSearch, d.WithTrailingDot()) + } + if !slices.Equal(gotSearch, tt.wantSearch) { + t.Errorf("search domains = %q, want %q", gotSearch, tt.wantSearch) + } + + var gotArgv []string + if b, err := os.ReadFile(argvLog); err == nil { + gotArgv = strings.Split(strings.TrimRight(string(b), "\n"), "\n") + } + if !slices.Equal(gotArgv, tt.wantArgv) { + t.Errorf("resolvconf invocations = %q, want %q", gotArgv, tt.wantArgv) + } + }) + } +}