net/dns: fix openresolv DNS config when no other snippets exist (#20996)

openresolv exits 2 when no config snippets are registered; treating that
as a failure aborted the entire DNS reconfiguration. Filtering out our own
snippet could also leave "resolvconf -l" with no arguments, which lists
every snippet including ours and would make quad-100 its own upstream.

Fixes #20825

Signed-off-by: Brendan Creane <bcreane@gmail.com>

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 {

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)
+			}
+		})
+	}
+}