net/netmon: move TailscaleInterfaceIndex out of netmon.State (#18428)

fixes tailscale/tailscale#18418

Both Serve and PeerAPI broke when we moved the TailscaleInterfaceName
into State, which is updated asynchronously and may not be
available when we configure the listeners.

This extracts the explicit interface name property from netmon.State
and adds as a static struct with getters that have proper error
handling.

The bug is only found in sandboxed Darwin clients, where we
need to know the Tailscale interface details in order to set up the
listeners correctly (they must bind to our interface explicitly to escape
the network sandboxing that is applied by NECP).

Currently set only sandboxed macOS and Plan9 set this but it will
also be useful on Windows to simplify interface filtering in netns.

Signed-off-by: Jonathan Nobels <[email protected]>
(cherry picked from commit 643e91f2eb8b3e3bc7a12b3e79a2df580684e3d0)

cmd/tailscaled/tailscaled.go

@@ -799,8 +799,9 @@ func tryEngine(logf logger.Logf, sys *tsd.System, name string) (onlyNetstack boo
 
 		if runtime.GOOS == "plan9" {
 			// TODO(bradfitz): why don't we do this on all platforms?
+			// TODO(barnstar): we do it on sandboxed darwin now
 			// We should. Doing it just on plan9 for now conservatively.
-			sys.NetMon.Get().SetTailscaleInterfaceName(devName)
+			netmon.SetTailscaleInterfaceProps(devName, 0)
 		}
 
 		r, err := router.New(logf, dev, sys.NetMon.Get(), sys.HealthTracker.Get(), sys.Bus.Get())

ipn/ipnlocal/local.go

@@ -565,7 +565,7 @@ func NewLocalBackend(logf logger.Logf, logID logid.PublicID, sys *tsd.System, lo
 
 	// Call our linkChange code once with the current state.
 	// Following changes are triggered via the eventbus.
-	cd, err := netmon.NewChangeDelta(nil, b.interfaceState, false, netMon.TailscaleInterfaceName(), false)
+	cd, err := netmon.NewChangeDelta(nil, b.interfaceState, false, false)
 	if err != nil {
 		b.logf("[unexpected] setting initial netmon state failed: %v", err)
 	} else {
@@ -5321,7 +5321,11 @@ func (b *LocalBackend) initPeerAPIListenerLocked() {
 		var err error
 		skipListen := i > 0 && isNetstack
 		if !skipListen {
-			ln, err = ps.listen(a.Addr(), b.interfaceState.TailscaleInterfaceIndex)
+			// We don't care about the error here.  Not all platforms set this.
+			// If ps.listen needs it, it will check for zero values and error out.
+			tsIfIndex, _ := netmon.TailscaleInterfaceIndex()
+
+			ln, err = ps.listen(a.Addr(), tsIfIndex)
 			if err != nil {
 				if peerAPIListenAsync {
 					b.logf("[v1] possibly transient peerapi listen(%q) error, will try again on linkChange: %v", a.Addr(), err)
@@ -5329,6 +5333,11 @@ func (b *LocalBackend) initPeerAPIListenerLocked() {
 					// ("peerAPIListeners too low").
 					continue
 				}
+				// Sandboxed macOS specifically requires the interface index to be non-zero.
+				if version.IsSandboxedMacOS() && tsIfIndex == 0 {
+					b.logf("[v1] peerapi listen(%q) error: interface index is 0 on darwin; try restarting tailscaled", a.Addr())
+					continue
+				}
 				b.logf("[unexpected] peerapi listen(%q) error: %v", a.Addr(), err)
 				continue
 			}

ipn/ipnlocal/peerapi.go

@@ -41,6 +41,8 @@ import (
 	"tailscale.com/wgengine/filter"
 )
 
+// initListenConfig, if non-nil, is called during peerAPIListener setup.  It is used only
+// on iOS and macOS to set socket options to bind the listener to the Tailscale interface.
 var initListenConfig func(config *net.ListenConfig, addr netip.Addr, tunIfIndex int) error
 
 // peerDNSQueryHandler is implemented by tsdns.Resolver.
@@ -69,6 +71,13 @@ func (s *peerAPIServer) listen(ip netip.Addr, tunIfIndex int) (ln net.Listener,
 		// On iOS/macOS, this sets the lc.Control hook to
 		// setsockopt the interface index to bind to, to get
 		// out of the network sandbox.
+
+		// A zero tunIfIndex is invalid for peerapi.  A zero value will not get us
+		// out of the network sandbox.  Caller should log and retry.
+		if tunIfIndex == 0 {
+			return nil, fmt.Errorf("peerapi: cannot listen on %s with tunIfIndex 0", ipStr)
+		}
+
 		if err := initListenConfig(&lc, ip, tunIfIndex); err != nil {
 			return nil, err
 		}

ipn/ipnlocal/serve.go

@@ -36,6 +36,7 @@ import (
 	"github.com/pires/go-proxyproto"
 	"go4.org/mem"
 	"tailscale.com/ipn"
+	"tailscale.com/net/netmon"
 	"tailscale.com/net/netutil"
 	"tailscale.com/syncs"
 	"tailscale.com/tailcfg"
@@ -166,16 +167,24 @@ func (s *localListener) Run() {
 
 		var lc net.ListenConfig
 		if initListenConfig != nil {
+			ifIndex, err := netmon.TailscaleInterfaceIndex()
+			if err != nil {
+				s.logf("localListener failed to get Tailscale interface index %v, backing off: %v", s.ap, err)
+				s.bo.BackOff(s.ctx, err)
+				continue
+			}
+
 			// On macOS, this sets the lc.Control hook to
 			// setsockopt the interface index to bind to. This is
-			// required by the network sandbox to allow binding to
-			// a specific interface. Without this hook, the system
-			// chooses a default interface to bind to.
-			if err := initListenConfig(&lc, ip, s.b.interfaceState.TailscaleInterfaceIndex); err != nil {
+			// required by the network sandbox which will not automatically
+			// bind to the tailscale interface to prevent routing loops.
+			// Explicit binding allows us to bypass that restriction.
+			if err := initListenConfig(&lc, ip, ifIndex); err != nil {
 				s.logf("localListener failed to init listen config %v, backing off: %v", s.ap, err)
 				s.bo.BackOff(s.ctx, err)
 				continue
 			}
+
 			// On macOS (AppStore or macsys) and if we're binding to a privileged port,
 			if version.IsSandboxedMacOS() && s.ap.Port() < 1024 {
 				// On macOS, we need to bind to ""/all-interfaces due to

net/netmon/interfaces.go

@@ -0,0 +1,103 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+package netmon
+
+import (
+	"errors"
+	"net"
+
+	"tailscale.com/syncs"
+)
+
+type ifProps struct {
+	mu    syncs.Mutex
+	name  string // interface name, if known/set
+	index int    // interface index, if known/set
+}
+
+// tsIfProps tracks the properties (name and index) of the tailscale interface.
+// There is only one tailscale interface per tailscaled instance.
+var tsIfProps ifProps
+
+func (p *ifProps) tsIfName() string {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	return p.name
+}
+
+func (p *ifProps) tsIfIndex() int {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	return p.index
+}
+
+func (p *ifProps) set(ifName string, ifIndex int) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.name = ifName
+	p.index = ifIndex
+}
+
+// TODO (barnstar): This doesn't need the Monitor receiver anymore but we're
+// keeping it for API compatibility to avoid a breaking change.  This can be
+// removed when the various clients have switched to SetTailscaleInterfaceProps
+func (m *Monitor) SetTailscaleInterfaceName(ifName string) {
+	SetTailscaleInterfaceProps(ifName, 0)
+}
+
+// SetTailscaleInterfaceProps sets the name of the Tailscale interface and
+// its index for use by various listeners/dialers.  If the index is zero,
+// an attempt will be made to look it up by name.  This makes no attempt
+// to validate that the interface exists at the time of calling.
+//
+// If this method is called, it is the responsibility of the caller to
+// update the interface name and index if they change.
+//
+// This should be called as early as possible during tailscaled startup.
+func SetTailscaleInterfaceProps(ifName string, ifIndex int) {
+	if ifIndex != 0 {
+		tsIfProps.set(ifName, ifIndex)
+		return
+	}
+
+	ifaces, err := net.Interfaces()
+	if err != nil {
+		return
+	}
+
+	for _, iface := range ifaces {
+		if iface.Name == ifName {
+			ifIndex = iface.Index
+			break
+		}
+	}
+
+	tsIfProps.set(ifName, ifIndex)
+}
+
+// TailscaleInterfaceName returns the name of the Tailscale interface.
+// For example, "tailscale0", "tun0", "utun3", etc or an error if unset.
+//
+// Callers must handle errors, as the Tailscale interface
+// name may not be set in some environments.
+func TailscaleInterfaceName() (string, error) {
+	name := tsIfProps.tsIfName()
+	if name == "" {
+		return "", errors.New("Tailscale interface name not set")
+	}
+	return name, nil
+}
+
+// TailscaleInterfaceIndex returns the index of the Tailscale interface or
+// an error if unset.
+//
+// Callers must handle errors, as the Tailscale interface
+// index may not be set in some environments.
+func TailscaleInterfaceIndex() (int, error) {
+	index := tsIfProps.tsIfIndex()
+	if index == 0 {
+		return 0, errors.New("Tailscale interface index not set")
+	}
+	return index, nil
+}

net/netmon/loghelper_test.go

@@ -64,7 +64,7 @@ func syncTestLinkChangeLogLimiter(t *testing.T) {
 	// InjectEvent doesn't work because it's not a major event, so we
 	// instead inject the event ourselves.
 	injector := eventbustest.NewInjector(t, bus)
-	cd, err := NewChangeDelta(nil, &State{}, true, "tailscale0", true)
+	cd, err := NewChangeDelta(nil, &State{}, true, true)
 	if err != nil {
 		t.Fatal(err)
 	}

net/netmon/netmon.go

@@ -78,8 +78,7 @@ type Monitor struct {
 	goroutines sync.WaitGroup
 	wallTimer  *time.Timer // nil until Started; re-armed AfterFunc per tick
 	lastWall   time.Time
-	timeJumped bool   // whether we need to send a changed=true after a big time jump
-	tsIfName   string // tailscale interface name, if known/set ("tailscale0", "utun3", ...)
+	timeJumped bool // whether we need to send a changed=true after a big time jump
 }
 
 // ChangeFunc is a callback function registered with Monitor that's called when the
@@ -103,10 +102,6 @@ type ChangeDelta struct {
 	// come out of sleep.
 	TimeJumped bool
 
-	// The tailscale interface name, e.g. "tailscale0", "utun3", etc.  Not all
-	// platforms know this or set it.  Copied from netmon.Monitor.tsIfName.
-	TailscaleIfaceName string
-
 	DefaultRouteInterface string
 
 	// Computed Fields
@@ -134,12 +129,11 @@ func (cd *ChangeDelta) CurrentState() *State {
 // NewChangeDelta builds a ChangeDelta and eagerly computes the cached fields.
 // forceViability, if true, forces DefaultInterfaceMaybeViable to be true regardless of the
 // actual state of the default interface.  This is useful in testing.
-func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string, forceViability bool) (*ChangeDelta, error) {
+func NewChangeDelta(old, new *State, timeJumped bool, forceViability bool) (*ChangeDelta, error) {
 	cd := ChangeDelta{
-		old:                old,
-		new:                new,
-		TimeJumped:         timeJumped,
-		TailscaleIfaceName: tsIfName,
+		old:        old,
+		new:        new,
+		TimeJumped: timeJumped,
 	}
 
 	if cd.new == nil {
@@ -162,8 +156,10 @@ func NewChangeDelta(old, new *State, timeJumped bool, tsIfName string, forceViab
 	cd.DefaultRouteInterface = new.DefaultRouteInterface
 	defIf := new.Interface[cd.DefaultRouteInterface]
 
+	tsIfName, err := TailscaleInterfaceName()
+
 	// The default interface is not viable if it is down or it is the Tailscale interface itself.
-	if !forceViability && (!defIf.IsUp() || cd.DefaultRouteInterface == tsIfName) {
+	if !forceViability && (!defIf.IsUp() || (err == nil && cd.DefaultRouteInterface == tsIfName)) {
 		cd.DefaultInterfaceMaybeViable = false
 	} else {
 		cd.DefaultInterfaceMaybeViable = true
@@ -223,10 +219,11 @@ func (cd *ChangeDelta) isInterestingInterfaceChange() bool {
 	}
 
 	// Compare interfaces in both directions.  Old to new and new to old.
+	tsIfName, ifNameErr := TailscaleInterfaceName()
 
 	for iname, oldInterface := range cd.old.Interface {
-		if iname == cd.TailscaleIfaceName {
-			// Ignore changes in the Tailscale interface itself.
+		if ifNameErr == nil && iname == tsIfName {
+			// Ignore changes in the Tailscale interface itself
 			continue
 		}
 		oldIps := filterRoutableIPs(cd.old.InterfaceIPs[iname])
@@ -259,7 +256,8 @@ func (cd *ChangeDelta) isInterestingInterfaceChange() bool {
 	}
 
 	for iname, newInterface := range cd.new.Interface {
-		if iname == cd.TailscaleIfaceName {
+		if ifNameErr == nil && iname == tsIfName {
+			// Ignore changes in the Tailscale interface itself
 			continue
 		}
 		newIps := filterRoutableIPs(cd.new.InterfaceIPs[iname])
@@ -360,24 +358,7 @@ func (m *Monitor) InterfaceState() *State {
 }
 
 func (m *Monitor) interfaceStateUncached() (*State, error) {
-	return getState(m.tsIfName)
-}
-
-// SetTailscaleInterfaceName sets the name of the Tailscale interface. For
-// example, "tailscale0", "tun0", "utun3", etc.
-//
-// This must be called only early in tailscaled startup before the monitor is
-// used.
-func (m *Monitor) SetTailscaleInterfaceName(ifName string) {
-	m.mu.Lock()
-	defer m.mu.Unlock()
-	m.tsIfName = ifName
-}
-
-func (m *Monitor) TailscaleInterfaceName() string {
-	m.mu.Lock()
-	defer m.mu.Unlock()
-	return m.tsIfName
+	return getState(tsIfProps.tsIfName())
 }
 
 // GatewayAndSelfIP returns the current network's default gateway, and
@@ -598,7 +579,7 @@ func (m *Monitor) handlePotentialChange(newState *State, forceCallbacks bool) {
 		return
 	}
 
-	delta, err := NewChangeDelta(oldState, newState, timeJumped, m.tsIfName, false)
+	delta, err := NewChangeDelta(oldState, newState, timeJumped, false)
 	if err != nil {
 		m.logf("[unexpected] error creating ChangeDelta: %v", err)
 		return

net/netmon/netmon_test.go

@@ -159,7 +159,7 @@ func TestMonitorMode(t *testing.T) {
 
 // tests (*ChangeDelta).RebindRequired
 func TestRebindRequired(t *testing.T) {
-	// s1 cannot be nil by definition
+	// s1 must not be nil by definition
 	tests := []struct {
 		name     string
 		s1, s2   *State
@@ -478,9 +478,11 @@ func TestRebindRequired(t *testing.T) {
 	withIsInterestingInterface(t, func(ni Interface, pfxs []netip.Prefix) bool {
 		return !strings.HasPrefix(ni.Name, "boring")
 	})
+	saveAndRestoreTailscaleIfaceProps(t)
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+
 			// Populate dummy interfaces where missing.
 			for _, s := range []*State{tt.s1, tt.s2} {
 				if s == nil {
@@ -495,7 +497,8 @@ func TestRebindRequired(t *testing.T) {
 				}
 			}
 
-			cd, err := NewChangeDelta(tt.s1, tt.s2, false, tt.tsIfName, true)
+			SetTailscaleInterfaceProps(tt.tsIfName, 1)
+			cd, err := NewChangeDelta(tt.s1, tt.s2, false, true)
 			if err != nil {
 				t.Fatalf("NewChangeDelta error: %v", err)
 			}
@@ -507,6 +510,15 @@ func TestRebindRequired(t *testing.T) {
 	}
 }
 
+func saveAndRestoreTailscaleIfaceProps(t *testing.T) {
+	t.Helper()
+	index, _ := TailscaleInterfaceIndex()
+	name, _ := TailscaleInterfaceName()
+	t.Cleanup(func() {
+		SetTailscaleInterfaceProps(name, index)
+	})
+}
+
 func withIsInterestingInterface(t *testing.T, fn func(Interface, []netip.Prefix) bool) {
 	t.Helper()
 	old := IsInterestingInterface

net/netmon/state.go

@@ -287,9 +287,6 @@ type State struct {
 
 	// PAC is the URL to the Proxy Autoconfig URL, if applicable.
 	PAC string
-
-	// TailscaleInterfaceIndex is the index of the Tailscale interface
-	TailscaleInterfaceIndex int
 }
 
 func (s *State) String() string {
@@ -473,15 +470,22 @@ func hasTailscaleIP(pfxs []netip.Prefix) bool {
 }
 
 func isTailscaleInterface(name string, ips []netip.Prefix) bool {
+	// Sandboxed macOS and Plan9 (and anything else that explicitly calls SetTailscaleInterfaceProps).
+	tsIfName, err := TailscaleInterfaceName()
+	if err == nil {
+		// If we've been told the Tailscale interface name, use that.
+		return name == tsIfName
+	}
+
+	// The sandboxed app should (as of 1.92) set the tun interface name via SetTailscaleInterfaceProps
+	// early in the startup process.  The non-sandboxed app does not.
+	// TODO (barnstar):  If Wireguard created the tun device on darwin, it should know the name and it should
+	// be explicitly set instead checking addresses here.
 	if runtime.GOOS == "darwin" && strings.HasPrefix(name, "utun") && hasTailscaleIP(ips) {
-		// On macOS in the sandboxed app (at least as of
-		// 2021-02-25), we often see two utun devices
-		// (e.g. utun4 and utun7) with the same IPv4 and IPv6
-		// addresses. Just remove all utun devices with
-		// Tailscale IPs until we know what's happening with
-		// macOS NetworkExtensions and utun devices.
 		return true
 	}
+
+	// Windows, Linux...
 	return name == "Tailscale" || // as it is on Windows
 		strings.HasPrefix(name, "tailscale") // TODO: use --tun flag value, etc; see TODO in method doc
 }
@@ -505,18 +509,15 @@ func getState(optTSInterfaceName string) (*State, error) {
 		s.Interface[ni.Name] = ni
 		s.InterfaceIPs[ni.Name] = append(s.InterfaceIPs[ni.Name], pfxs...)
 
-		// Skip uninteresting interfaces.
+		// Skip uninteresting interfaces
 		if IsInterestingInterface != nil && !IsInterestingInterface(ni, pfxs) {
 			return
 		}
 
-		if isTailscaleInterface(ni.Name, pfxs) {
-			s.TailscaleInterfaceIndex = ni.Index
-		}
-
 		if !ifUp || isTSInterfaceName || isTailscaleInterface(ni.Name, pfxs) {
 			return
 		}
+
 		for _, pfx := range pfxs {
 			if pfx.Addr().IsLoopback() {
 				continue
@@ -803,8 +804,7 @@ func (m *Monitor) HasCGNATInterface() (bool, error) {
 	hasCGNATInterface := false
 	cgnatRange := tsaddr.CGNATRange()
 	err := ForeachInterface(func(i Interface, pfxs []netip.Prefix) {
-		isTSInterfaceName := m.tsIfName != "" && i.Name == m.tsIfName
-		if hasCGNATInterface || !i.IsUp() || isTSInterfaceName || isTailscaleInterface(i.Name, pfxs) {
+		if hasCGNATInterface || !i.IsUp() || isTailscaleInterface(i.Name, pfxs) {
 			return
 		}
 		for _, pfx := range pfxs {