net/{netns, netmon}: use LastKnownDefaultInterface if set and check for utun (#16873)

fixes tailscale/corp#31299

Fixes two issues:
getInterfaceIndex would occasionally race with netmon's state, returning
the cached default interface index after it had be changed by NWNetworkMonitor.
This had the potential to cause connections to bind to the prior default.  The fix
here is to preferentially use the interface index provided by NWNetworkMonitor
preferentially.

When no interfaces are available, macOS will set the tunnel as the default
interface when an exit node is enabled, potentially causing getInterfaceIndex
to return utun's index.  We now guard against this when taking the
defaultIdx path.

Signed-off-by: Jonathan Nobels <jonathan@tailscale.com>

net/netmon/defaultroute_darwin.go

@@ -6,6 +6,8 @@
 package netmon
 
 import (
+	"errors"
+	"fmt"
 	"log"
 	"net"
 
@@ -16,14 +18,26 @@ var (
 	lastKnownDefaultRouteIfName syncs.AtomicValue[string]
 )
 
-// UpdateLastKnownDefaultRouteInterface is called by ipn-go-bridge in the iOS app when
+// UpdateLastKnownDefaultRouteInterface is called by ipn-go-bridge from apple network extensions when
 // our NWPathMonitor instance detects a network path transition.
 func UpdateLastKnownDefaultRouteInterface(ifName string) {
 	if ifName == "" {
 		return
 	}
 	if old := lastKnownDefaultRouteIfName.Swap(ifName); old != ifName {
-		log.Printf("defaultroute_darwin: update from Swift, ifName = %s (was %s)", ifName, old)
+		interfaces, err := netInterfaces()
+		if err != nil {
+			log.Printf("defaultroute_darwin: UpdateLastKnownDefaultRouteInterface could not get interfaces: %v", err)
+			return
+		}
+
+		netif, err := getInterfaceByName(ifName, interfaces)
+		if err != nil {
+			log.Printf("defaultroute_darwin: UpdateLastKnownDefaultRouteInterface could not find interface index for %s: %v", ifName, err)
+			return
+		}
+
+		log.Printf("defaultroute_darwin: updated last known default if from OS, ifName = %s index: %d (was %s)", ifName, netif.Index, old)
 	}
 }
 
@@ -40,57 +54,69 @@ func defaultRoute() (d DefaultRouteDetails, err error) {
 	//
 	// If for any reason the Swift machinery didn't work and we don't get any updates, we will
 	// fallback to the BSD logic.
+	osRoute, osRouteErr := OSDefaultRoute()
+	if osRouteErr == nil {
+		// If we got a valid interface from the OS, use it.
+		d.InterfaceName = osRoute.InterfaceName
+		d.InterfaceIndex = osRoute.InterfaceIndex
+		return d, nil
+	}
 
+	// Fallback to the BSD logic
+	idx, err := DefaultRouteInterfaceIndex()
+	if err != nil {
+		return d, err
+	}
+	iface, err := net.InterfaceByIndex(idx)
+	if err != nil {
+		return d, err
+	}
+	d.InterfaceName = iface.Name
+	d.InterfaceIndex = idx
+	return d, nil
+}
+
+// OSDefaultRoute returns the DefaultRouteDetails for the default interface as provided by the OS
+// via UpdateLastKnownDefaultRouteInterface.  If UpdateLastKnownDefaultRouteInterface has not been called,
+// the interface name is not valid, or we cannot find its index, an error is returned.
+func OSDefaultRoute() (d DefaultRouteDetails, err error) {
+
+	// Did Swift set lastKnownDefaultRouteInterface? If so, we should use it and don't bother
+	// with anything else. However, for sanity, do check whether Swift gave us with an interface
+	// that exists, is up, and has an address and is not the tunnel itself.
+	if swiftIfName := lastKnownDefaultRouteIfName.Load(); swiftIfName != "" {
 		// Start by getting all available interfaces.
 		interfaces, err := netInterfaces()
 		if err != nil {
 			log.Printf("defaultroute_darwin: could not get interfaces: %v", err)
-		return d, ErrNoGatewayIndexFound
+			return d, err
 		}
 
-	getInterfaceByName := func(name string) *Interface {
+		if ifc, err := getInterfaceByName(swiftIfName, interfaces); err == nil {
+			d.InterfaceName = ifc.Name
+			d.InterfaceIndex = ifc.Index
+			return d, nil
+		}
+	}
+	err = errors.New("no os provided default route interface found")
+	return d, err
+}
+
+func getInterfaceByName(name string, interfaces []Interface) (*Interface, error) {
 	for _, ifc := range interfaces {
 		if ifc.Name != name {
 			continue
 		}
 
 		if !ifc.IsUp() {
-				log.Printf("defaultroute_darwin: %s is down", name)
-				return nil
+			return nil, fmt.Errorf("defaultroute_darwin: %s is down", name)
 		}
 
 		addrs, _ := ifc.Addrs()
 		if len(addrs) == 0 {
-				log.Printf("defaultroute_darwin: %s has no addresses", name)
-				return nil
-			}
-			return &ifc
-		}
-		return nil
-	}
-
-	// Did Swift set lastKnownDefaultRouteInterface? If so, we should use it and don't bother
-	// with anything else. However, for sanity, do check whether Swift gave us with an interface
-	// that exists, is up, and has an address.
-	if swiftIfName := lastKnownDefaultRouteIfName.Load(); swiftIfName != "" {
-		ifc := getInterfaceByName(swiftIfName)
-		if ifc != nil {
-			d.InterfaceName = ifc.Name
-			d.InterfaceIndex = ifc.Index
-			return d, nil
+			return nil, fmt.Errorf("defaultroute_darwin: %s has no addresses", name)
 		}
+		return &ifc, nil
 	}
-
-	// Fallback to the BSD logic
-	idx, err := DefaultRouteInterfaceIndex()
-	if err != nil {
-		return d, err
-	}
-	iface, err := net.InterfaceByIndex(idx)
-	if err != nil {
-		return d, err
-	}
-	d.InterfaceName = iface.Name
-	d.InterfaceIndex = idx
-	return d, nil
+	return nil, errors.New("no interfaces found")
 }

net/netmon/interfaces_darwin_test.go

@@ -112,3 +112,25 @@ func TestFetchRoutingTable(t *testing.T) {
 		}
 	}
 }
+
+func TestUpdateLastKnownDefaultRouteInterface(t *testing.T) {
+	// Pick some interface on the machine
+	interfaces, err := netInterfaces()
+	if err != nil || len(interfaces) == 0 {
+		t.Fatalf("netInterfaces() error: %v", err)
+	}
+
+	// Set it as our last known default route interface
+	ifName := interfaces[0].Name
+	UpdateLastKnownDefaultRouteInterface(ifName)
+
+	// And make sure we can get it back
+	route, err := OSDefaultRoute()
+	if err != nil {
+		t.Fatalf("OSDefaultRoute() error: %v", err)
+	}
+	want, got := ifName, route.InterfaceName
+	if want != got {
+		t.Errorf("OSDefaultRoute() = %q, want %q", got, want)
+	}
+}

net/netns/netns_darwin.go

@@ -78,10 +78,38 @@ func getInterfaceIndex(logf logger.Logf, netMon *netmon.Monitor, address string)
 			return -1, errInterfaceStateInvalid
 		}
 
-		if iface, ok := state.Interface[state.DefaultRouteInterface]; ok {
-			return iface.Index, nil
+		// Netmon's cached view of the default inteface
+		cachedIdx, ok := state.Interface[state.DefaultRouteInterface]
+		// OSes view (if available) of the default interface
+		osIf, osIferr := netmon.OSDefaultRoute()
+
+		idx := -1
+		errOut := errInterfaceStateInvalid
+		// Preferentially choose the OS's view of the default if index.  Due to the way darwin sets the delegated
+		// interface on tunnel creation only, it is possible for netmon to have a stale view of the default and
+		// netmon's view is often temporarily wrong during network transitions, or for us to not have the
+		// the the oses view of the defaultIf yet.
+		if osIferr == nil {
+			idx = osIf.InterfaceIndex
+			errOut = nil
+		} else if ok {
+			idx = cachedIdx.Index
+			errOut = nil
 		}
-		return -1, errInterfaceStateInvalid
+
+		if osIferr == nil && ok && (osIf.InterfaceIndex != cachedIdx.Index) {
+			logf("netns: [unexpected] os default if %q (%d) != netmon cached if %q (%d)", osIf.InterfaceName, osIf.InterfaceIndex, cachedIdx.Name, cachedIdx.Index)
+		}
+
+		// Sanity check to make sure we didn't pick the tailscale interface
+		if tsif, err2 := tailscaleInterface(); tsif != nil && err2 == nil && errOut == nil {
+			if tsif.Index == idx {
+				idx = -1
+				errOut = errInterfaceStateInvalid
+			}
+		}
+
+		return idx, errOut
 	}
 
 	useRoute := bindToInterfaceByRoute.Load() || bindToInterfaceByRouteEnv()
@@ -100,7 +128,7 @@ func getInterfaceIndex(logf logger.Logf, netMon *netmon.Monitor, address string)
 
 	idx, err := interfaceIndexFor(addr, true /* canRecurse */)
 	if err != nil {
-		logf("netns: error in interfaceIndexFor: %v", err)
+		logf("netns: error getting interface index for %q: %v", address, err)
 		return defaultIdx()
 	}
 
@@ -108,10 +136,13 @@ func getInterfaceIndex(logf logger.Logf, netMon *netmon.Monitor, address string)
 	// if so, we fall back to binding from the default.
 	tsif, err2 := tailscaleInterface()
 	if err2 == nil && tsif != nil && tsif.Index == idx {
-		logf("[unexpected] netns: interfaceIndexFor returned Tailscale interface")
+		// note: with an exit node enabled, this is almost always true.  defaultIdx() is the
+		// right thing to do here.
 		return defaultIdx()
 	}
 
+	logf("netns: completed success interfaceIndexFor(%s) = %d", address, idx)
+
 	return idx, err
 }