From 11c1917387d251580667764261a708c3dd7fef34 Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Mon, 12 Oct 2026 17:57:14 -0400 Subject: [PATCH] Warn when an unsafe_route is already routed through another interface on darwin (#1925) --- overlay/tun_darwin.go | 82 +++++++++++++++++++++++++++++--- overlay/tun_darwin_route_test.go | 75 +++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 7 deletions(-) create mode 100644 overlay/tun_darwin_route_test.go diff --git a/overlay/tun_darwin.go b/overlay/tun_darwin.go index b95a93cf..a0834590 100644 --- a/overlay/tun_darwin.go +++ b/overlay/tun_darwin.go @@ -6,8 +6,10 @@ import ( "errors" "fmt" "log/slog" + "math/bits" "net/netip" "os" + "sync" "sync/atomic" "syscall" "unsafe" @@ -28,7 +30,7 @@ type tun struct { DefaultMTU int Routes atomic.Pointer[[]Route] routeTree atomic.Pointer[bart.Table[routing.Gateways]] - linkAddr *netroute.LinkAddr + linkAddr atomic.Pointer[netroute.LinkAddr] // hostOwned means the fd arrived from the OS, which has already configured addressing, mtu // and routes for it. NEPacketTunnelProvider on darwin does this. hostOwned bool @@ -243,7 +245,7 @@ func (t *tun) Activate() error { if linkAddr == nil { return fmt.Errorf("unable to discover link_addr for tun interface") } - t.linkAddr = linkAddr + t.linkAddr.Store(linkAddr) for _, network := range t.vpnNetworks { if network.Addr().Is4() { @@ -303,7 +305,7 @@ func (t *tun) activate4(network netip.Prefix) error { return fmt.Errorf("failed to set tun v4 address: %s", err) } - err = addRoute(network, t.linkAddr) + err = addRoute(network, t.linkAddr.Load()) if err != nil { return err } @@ -394,6 +396,56 @@ func (t *tun) RoutesFor(ip netip.Addr) routing.Gateways { return routing.Gateways{} } +func fetchRoutes() ([]netroute.Message, error) { + rib, err := netroute.FetchRIB(unix.AF_UNSPEC, unix.NET_RT_DUMP, 0) + if err != nil { + return nil, err + } + return netroute.ParseRIB(unix.NET_RT_DUMP, rib) +} + +// routeInterfaceIn returns the index of the interface prefix is routed through, or 0 if msgs has no route for exactly +// that prefix. Routes scoped to an interface are skipped, they don't stop us adding ours. +func routeInterfaceIn(msgs []netroute.Message, prefix netip.Prefix) int { + for _, msg := range msgs { + m, ok := msg.(*netroute.RouteMessage) + if !ok || len(m.Addrs) <= unix.RTAX_NETMASK || m.Flags&unix.RTF_IFSCOPE != 0 { + continue + } + if p, ok := routePrefix(m.Addrs[unix.RTAX_DST], m.Addrs[unix.RTAX_NETMASK]); ok && p == prefix.Masked() { + return m.Index + } + } + return 0 +} + +// routePrefix returns the prefix a route covers. Host routes have no netmask. +func routePrefix(dst, mask netroute.Addr) (netip.Prefix, bool) { + switch d := dst.(type) { + case *netroute.Inet4Addr: + bits := 32 + if m, ok := mask.(*netroute.Inet4Addr); ok { + bits = maskBits(m.IP[:]) + } + return netip.PrefixFrom(netip.AddrFrom4(d.IP), bits), true + case *netroute.Inet6Addr: + bits := 128 + if m, ok := mask.(*netroute.Inet6Addr); ok { + bits = maskBits(m.IP[:]) + } + return netip.PrefixFrom(netip.AddrFrom16(d.IP), bits), true + } + return netip.Prefix{}, false +} + +func maskBits(mask []byte) int { + n := 0 + for _, b := range mask { + n += bits.OnesCount8(b) + } + return n +} + // Get the LinkAddr for the interface of the given name // Is there an easier way to fetch this when we create the interface? // Maybe SIOCGIFINDEX? but this doesn't appear to exist in the darwin headers. @@ -428,7 +480,14 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } + // Activate hasn't run yet, it will install the routes + linkAddr := t.linkAddr.Load() + if linkAddr == nil { + return nil + } + routes := *t.Routes.Load() + systemRoutes := sync.OnceValues(fetchRoutes) for _, r := range routes { if len(r.Via) == 0 || !r.Install { @@ -436,10 +495,16 @@ func (t *tun) addRoutes(logErrors bool) error { continue } - err := addRoute(r.Cidr, t.linkAddr) + err := addRoute(r.Cidr, linkAddr) if err != nil { if errors.Is(err, unix.EEXIST) { - t.l.Warn("unable to add unsafe_route, identical route already exists", "route", r.Cidr) + // Every route we added already exists on a reload, only warn about someone else's + msgs, err := systemRoutes() + if err != nil { + t.l.Warn("unable to add unsafe_route, a route already exists but its interface could not be determined", "route", r.Cidr, "error", err) + } else if routeInterfaceIn(msgs, r.Cidr) != linkAddr.Index { + t.l.Warn("unable to add unsafe_route, the destination is already routed through another interface", "route", r.Cidr) + } } else { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { @@ -450,6 +515,8 @@ func (t *tun) addRoutes(logErrors bool) error { } } else { t.l.Info("Added route", "route", r) + // A cidr listed twice would otherwise find our own route missing from an older fetch + systemRoutes = sync.OnceValues(fetchRoutes) } } @@ -457,7 +524,8 @@ func (t *tun) addRoutes(logErrors bool) error { } func (t *tun) removeRoutes(routes []Route) error { - if t.hostOwned { + linkAddr := t.linkAddr.Load() + if t.hostOwned || linkAddr == nil { return nil } @@ -466,7 +534,7 @@ func (t *tun) removeRoutes(routes []Route) error { continue } - err := delRoute(r.Cidr, t.linkAddr) + err := delRoute(r.Cidr, linkAddr) if err != nil { t.l.Error("Failed to remove route", "error", err, "route", r) } else { diff --git a/overlay/tun_darwin_route_test.go b/overlay/tun_darwin_route_test.go new file mode 100644 index 00000000..9df527bd --- /dev/null +++ b/overlay/tun_darwin_route_test.go @@ -0,0 +1,75 @@ +//go:build darwin && !ios && !e2e_testing + +package overlay + +import ( + "net" + "net/netip" + "os/exec" + "strings" + "testing" + + "github.com/slackhq/nebula/routing" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + netroute "golang.org/x/net/route" + "golang.org/x/sys/unix" +) + +// Every mac routes loopback through lo0 +func TestRouteInterfaceIn(t *testing.T) { + msgs, err := fetchRoutes() + require.NoError(t, err) + routeInterface := func(s string) int { return routeInterfaceIn(msgs, netip.MustParsePrefix(s)) } + + lo0, err := net.InterfaceByName("lo0") + require.NoError(t, err) + + assert.Equal(t, lo0.Index, routeInterface("127.0.0.0/8")) + assert.Equal(t, lo0.Index, routeInterface("127.0.0.1/32"), "a host route has no netmask") + assert.Equal(t, lo0.Index, routeInterface("::1/128")) + assert.Equal(t, 0, routeInterface("127.0.0.0/16"), "only an exact prefix counts") + + // Most macs also list the default route scoped to their other interfaces + out, err := exec.Command("route", "-n", "get", "default").Output() + require.NoError(t, err) + var name string + for _, line := range strings.Split(string(out), "\n") { + if v, ok := strings.CutPrefix(strings.TrimSpace(line), "interface: "); ok { + name = v + } + } + if name == "" { + t.Skip("no default route") + } + def, err := net.InterfaceByName(name) + require.NoError(t, err) + assert.Equal(t, def.Index, routeInterface("0.0.0.0/0")) +} + +func TestRouteInterfaceIn_SkipsScopedCopies(t *testing.T) { + route := func(index, flags int) netroute.Message { + addrs := make([]netroute.Addr, unix.RTAX_MAX) + addrs[unix.RTAX_DST] = &netroute.Inet4Addr{IP: [4]byte{192, 168, 1, 0}} + addrs[unix.RTAX_NETMASK] = &netroute.Inet4Addr{IP: [4]byte{255, 255, 255, 0}} + return &netroute.RouteMessage{Index: index, Flags: flags, Addrs: addrs} + } + p := netip.MustParsePrefix("192.168.1.0/24") + assert.Equal(t, 7, routeInterfaceIn([]netroute.Message{route(4, unix.RTF_UP|unix.RTF_IFSCOPE), route(7, unix.RTF_UP)}, p)) + assert.Equal(t, 0, routeInterfaceIn([]netroute.Message{route(4, unix.RTF_UP|unix.RTF_IFSCOPE)}, p)) +} + +// A reload can happen before Activate +func TestTunRoutesBeforeActivate(t *testing.T) { + tn := &tun{l: test.NewLogger()} + routes := []Route{{ + Cidr: netip.MustParsePrefix("10.253.1.0/24"), + Via: routing.Gateways{routing.NewGateway(netip.MustParseAddr("10.250.0.2"), 1)}, + Install: true, + }} + tn.Routes.Store(&routes) + + assert.NoError(t, tn.addRoutes(true)) + assert.NoError(t, tn.removeRoutes(routes)) +}