diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c34dfc30a..134444ed8 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -6155,10 +6155,23 @@ func (b *LocalBackend) authReconfigLocked() { } rcfg := b.routerConfigLocked(cfg, prefs, nm) - // Add these extra Allowed IPs after router configuration, because the expected - // extension (features/conn25), does not want these routes installed on the OS. + // Push each peer's extra WireGuard-only allowed IPs (the conn25 + // extension's Transit IPs) into the route manager, which feeds + // them to WireGuard via the outbound peer lookup and the per-peer + // allowed source prefixes (including for lazily created peers) + // while keeping them out of the OS route set, because the + // expected extension (features/conn25) does not want these routes + // installed on the OS. This runs after routerConfigLocked above + // for the same reason: rcfg is derived from cfg.Peers, which must + // not yet include the extras. // See also [Hooks.ExtraWireGuardAllowedIPs]. if extraAllowedIPsFn, ok := b.extHost.hooks.ExtraWireGuardAllowedIPs.GetOk(); ok { + for k := range cn.updateRouteManagerExtras(extraAllowedIPsFn) { + b.e.SyncDevicePeer(k) + } + // Also append the extras to cfg.Peers so the full SyncPeers + // in Reconfig below doesn't strip them from active peers. + // This loop goes away when cfg.Peers does. for i := range cfg.Peers { extras := extraAllowedIPsFn(cfg.Peers[i].PublicKey) cfg.Peers[i].AllowedIPs = extras.AppendTo(cfg.Peers[i].AllowedIPs) diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index d7ef60f59..6c13d5211 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/ipn/ipnlocal/node_backend.go @@ -938,6 +938,28 @@ func (nb *nodeBackend) updateRouteManagerPrefs(p routePrefs) (changedAllowedIPs return res.AllowedIPs } +// updateRouteManagerExtras pushes each peer's extra WireGuard-only +// allowed IPs into the route manager, obtained by calling fn (the +// [ipnext.Hooks.ExtraWireGuardAllowedIPs] hook) with each peer's +// public key. +// +// It returns the peers whose allowed source prefixes changed as a +// result, as described by [routemanager.Result.AllowedIPs]. +func (nb *nodeBackend) updateRouteManagerExtras(fn func(key.NodePublic) views.Slice[netip.Prefix]) (changedAllowedIPs map[key.NodePublic][]netip.Prefix) { + nb.mu.Lock() + defer nb.mu.Unlock() + var extras map[tailcfg.NodeID][]netip.Prefix + for id, p := range nb.peers { + if pfxs := fn(p.Key()); pfxs.Len() > 0 { + mak.Set(&extras, id, pfxs.AsSlice()) + } + } + rt := nb.routeMgr.Begin() + rt.SetExtraAllowedIPs(extras) + res := rt.Commit() + return res.AllowedIPs +} + // osRoutes returns the sorted set of prefixes that the route manager // wants programmed into the OS routing table. func (nb *nodeBackend) osRoutes() []netip.Prefix { diff --git a/ipn/ipnlocal/node_backend_test.go b/ipn/ipnlocal/node_backend_test.go index 04a7b8969..9b09ed930 100644 --- a/ipn/ipnlocal/node_backend_test.go +++ b/ipn/ipnlocal/node_backend_test.go @@ -17,6 +17,7 @@ "tailscale.com/tstest" "tailscale.com/types/key" "tailscale.com/types/netmap" + "tailscale.com/types/views" "tailscale.com/util/eventbus" "tailscale.com/util/mak" "tailscale.com/util/set" @@ -505,3 +506,56 @@ func TestNodeBackendDiscoChangedDelta(t *testing.T) { t.Errorf("upsert(after TSMP entry GC) discoChanged = %v; want %v", got, nk2) } } + +func TestNodeBackendRouteManagerExtras(t *testing.T) { + nb := newNodeBackend(t.Context(), tstest.WhileTestRunningLogger(t), eventbus.New()) + + n := &tailcfg.Node{ + ID: 1, + Key: key.NewNode().Public(), + HomeDERP: 1, + Addresses: []netip.Prefix{ + netip.MustParsePrefix("100.64.0.1/32"), + }, + } + n.AllowedIPs = n.Addresses + p1 := n.View() + nb.SetNetMap(&netmap.NetworkMap{Peers: []tailcfg.NodeView{p1}}) + + transit := netip.MustParsePrefix("fe80::1234/128") + extrasFor := func(k key.NodePublic) views.Slice[netip.Prefix] { + if k == p1.Key() { + return views.SliceOf([]netip.Prefix{transit}) + } + return views.Slice[netip.Prefix]{} + } + + // Installing extras reports the peer's allowed prefixes as + // changed and adds the transit IP to the outbound table. + changed := nb.updateRouteManagerExtras(extrasFor) + if len(changed) != 1 || !slices.Contains(changed[p1.Key()], transit) { + t.Errorf("updateRouteManagerExtras changed = %v; want %v including %v", changed, p1.Key(), transit) + } + if pr, ok := nb.routeMgr.Outbound().Lookup(transit.Addr()); !ok || pr.Key != p1.Key() { + t.Errorf("Outbound lookup %v = %v, %v; want %v", transit.Addr(), pr, ok, p1.Key()) + } + if nb.routeMgr.OSRoutes().Get(transit) { + t.Errorf("OSRoutes contains %v; extras must not reach the OS route set", transit) + } + + // An unchanged hook result is a no-op. + if changed := nb.updateRouteManagerExtras(extrasFor); changed != nil { + t.Errorf("unchanged extras reported changes: %v", changed) + } + + // A hook that no longer returns extras removes them. + changed = nb.updateRouteManagerExtras(func(key.NodePublic) views.Slice[netip.Prefix] { + return views.Slice[netip.Prefix]{} + }) + if len(changed) != 1 { + t.Errorf("clearing extras changed = %v; want just %v", changed, p1.Key()) + } + if _, ok := nb.routeMgr.Outbound().Lookup(transit.Addr()); ok { + t.Errorf("Outbound still routes %v after extras cleared", transit.Addr()) + } +}