From 6006bc92b5d1fd6a71f776826fc8e200ebc9b755 Mon Sep 17 00:00:00 2001 From: Jonathan Nobels Date: Fri, 15 Aug 2025 16:04:23 -0400 Subject: [PATCH] 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 --- net/netmon/defaultroute_darwin.go | 106 +++++++++++++++++---------- net/netmon/interfaces_darwin_test.go | 22 ++++++ net/netns/netns_darwin.go | 41 +++++++++-- 3 files changed, 124 insertions(+), 45 deletions(-) diff --git a/net/netmon/defaultroute_darwin.go b/net/netmon/defaultroute_darwin.go index 4efe2f1aa..57f7e22b7 100644 --- a/net/netmon/defaultroute_darwin.go +++ b/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 + } - // Start by getting all available interfaces. - interfaces, err := netInterfaces() + // Fallback to the BSD logic + idx, err := DefaultRouteInterfaceIndex() if err != nil { - log.Printf("defaultroute_darwin: could not get interfaces: %v", err) - return d, ErrNoGatewayIndexFound + return d, err } - - getInterfaceByName := func(name string) *Interface { - for _, ifc := range interfaces { - if ifc.Name != name { - continue - } - - if !ifc.IsUp() { - log.Printf("defaultroute_darwin: %s is down", name) - return nil - } - - addrs, _ := ifc.Addrs() - if len(addrs) == 0 { - log.Printf("defaultroute_darwin: %s has no addresses", name) - return nil - } - return &ifc - } - return nil + 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. + // that exists, is up, and has an address and is not the tunnel itself. if swiftIfName := lastKnownDefaultRouteIfName.Load(); swiftIfName != "" { - ifc := getInterfaceByName(swiftIfName) - if ifc != nil { + // Start by getting all available interfaces. + interfaces, err := netInterfaces() + if err != nil { + log.Printf("defaultroute_darwin: could not get interfaces: %v", err) + return d, err + } + + 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 +} - // 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 +func getInterfaceByName(name string, interfaces []Interface) (*Interface, error) { + for _, ifc := range interfaces { + if ifc.Name != name { + continue + } + + if !ifc.IsUp() { + return nil, fmt.Errorf("defaultroute_darwin: %s is down", name) + } + + addrs, _ := ifc.Addrs() + if len(addrs) == 0 { + return nil, fmt.Errorf("defaultroute_darwin: %s has no addresses", name) + } + return &ifc, nil } - d.InterfaceName = iface.Name - d.InterfaceIndex = idx - return d, nil + return nil, errors.New("no interfaces found") } diff --git a/net/netmon/interfaces_darwin_test.go b/net/netmon/interfaces_darwin_test.go index d756d1334..c3d40a6f0 100644 --- a/net/netmon/interfaces_darwin_test.go +++ b/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) + } +} diff --git a/net/netns/netns_darwin.go b/net/netns/netns_darwin.go index ac5e89d76..f2ed16601 100644 --- a/net/netns/netns_darwin.go +++ b/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 + } + + 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 -1, 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 }