From 0aff3656e2964ef4d3b1d85a1c533ce1ae09fdf2 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 13:46:50 +0000 Subject: [PATCH] policy/v2: bound via route steering by the peer's routes Include and Exclude held one entry per overlapping dst, and the election walked them per dst. Keep one entry per route; a peer without routes returns at once. --- hscontrol/policy/v2/policy.go | 74 ++++++++++++++++++------------ hscontrol/policy/v2/policy_test.go | 69 ++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 30 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index cb5b23984..339436a41 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -1440,6 +1440,11 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via return result } + // A peer without routes has nothing to steer. + if len(peer.SubnetRoutes()) == 0 && !peer.IsExitNode() { + return result + } + // Clip so the appends below allocate: callers share pm.pol under RLock, // and writing into its spare capacity races between them. grants := slices.Clip(pm.pol.Grants) @@ -1504,11 +1509,13 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via var matchedPrefixes []netip.Prefix - for _, dstPrefix := range resolvedDstPrefixes[i] { - for _, route := range peerSubnetRoutes { - if dstPrefix.Overlaps(route) { - matchedPrefixes = append(matchedPrefixes, route) - } + // Emit each route once, however many dst prefixes overlap it: + // the passes below walk Include and Exclude per grant, so their + // size must be bounded by the peer's routes, not by the grant's + // destinations. + for _, route := range peerSubnetRoutes { + if slices.ContainsFunc(resolvedDstPrefixes[i], route.Overlaps) { + matchedPrefixes = append(matchedPrefixes, route) } } @@ -1536,10 +1543,16 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via } } - if peerHasVia { - result.Include = append(result.Include, matchedPrefixes...) - } else { - result.Exclude = append(result.Exclude, matchedPrefixes...) + // Several grants, or repeated announcements, can match the same + // route; keep one entry per route. + for _, p := range matchedPrefixes { + if peerHasVia { + if !slices.Contains(result.Include, p) { + result.Include = append(result.Include, p) + } + } else if !slices.Contains(result.Exclude, p) { + result.Exclude = append(result.Exclude, p) + } } } @@ -1579,33 +1592,34 @@ func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.Via // Elect per matched route, not per dst — a peer can only // be primary for a prefix it actually advertises, and one - // dst may cover multiple distinct routes. - for _, dstPrefix := range resolvedDstPrefixes[i] { - for _, included := range slices.Clone(result.Include) { - if !dstPrefix.Overlaps(included) { - continue - } + // dst may cover multiple distinct routes. The election + // does not depend on which dst matched, so each included + // route is looked at once per grant. + for _, included := range slices.Clone(result.Include) { + if !slices.ContainsFunc(resolvedDstPrefixes[i], included.Overlaps) { + continue + } - var viaPrimaryID types.NodeID + var viaPrimaryID types.NodeID - for _, viaTag := range grant.Via { - for _, node := range pm.nodes.All() { - if node.HasTag(string(viaTag)) && - slices.Contains(node.SubnetRoutes(), included) { - if viaPrimaryID == 0 || node.ID() < viaPrimaryID { - viaPrimaryID = node.ID() - } + for _, viaTag := range grant.Via { + for _, node := range pm.nodes.All() { + if node.HasTag(string(viaTag)) && + slices.Contains(node.SubnetRoutes(), included) { + if viaPrimaryID == 0 || node.ID() < viaPrimaryID { + viaPrimaryID = node.ID() } } } + } - if viaPrimaryID != 0 && peer.ID() != viaPrimaryID { - result.Include = slices.DeleteFunc(result.Include, func(p netip.Prefix) bool { - return p == included - }) - if !slices.Contains(result.Exclude, included) { - result.Exclude = append(result.Exclude, included) - } + if viaPrimaryID != 0 && peer.ID() != viaPrimaryID { + result.Include = slices.DeleteFunc( + result.Include, + func(p netip.Prefix) bool { return p == included }, + ) + if !slices.Contains(result.Exclude, included) { + result.Exclude = append(result.Exclude, included) } } } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 615c3df09..c9b93371b 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/hscontrol/policy/v2/policy_test.go @@ -2573,6 +2573,75 @@ func TestViaRoutesForPeer(t *testing.T) { require.Empty(t, result.Exclude) } }) + + // A route is steered once, however many dst prefixes or grants overlap + // it and however often the peer announces it. Otherwise Include grows + // with the policy instead of the peer's routes, and the election pass, + // which walks Include per dst, goes quadratic under the policy lock. + t.Run("route_steered_once_per_peer", func(t *testing.T) { + t.Parallel() + + nodes := types.Nodes{ + { + ID: 1, + Hostname: "viewer", + IPv4: ap("100.64.0.1"), + User: new(users[0]), + UserID: new(users[0].ID), + Hostinfo: &tailcfg.Hostinfo{}, + }, + { + ID: 2, + Hostname: "router", + IPv4: ap("100.64.0.2"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:router"}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{mp("10.0.0.0/24"), mp("10.0.0.0/24")}, + }, + ApprovedRoutes: []netip.Prefix{mp("10.0.0.0/24")}, + }, + { + ID: 3, + Hostname: "other", + IPv4: ap("100.64.0.3"), + User: new(users[0]), + UserID: new(users[0].ID), + Tags: []string{"tag:other"}, + Hostinfo: &tailcfg.Hostinfo{ + RoutableIPs: []netip.Prefix{mp("10.0.0.0/24"), mp("10.0.0.0/24")}, + }, + ApprovedRoutes: []netip.Prefix{mp("10.0.0.0/24")}, + }, + } + + grant := `{ + "src": ["user1@"], + "dst": ["10.0.0.0/24", "10.0.0.0/24", "10.0.0.0/25", "10.0.0.7/32"], + "ip": ["*"], + "via": ["tag:router"] + }` + + pol := `{ + "tagOwners": { + "tag:router": ["user1@"], + "tag:other": ["user1@"] + }, + "grants": [` + grant + `, ` + grant + `] + }` + + pm, err := NewPolicyManager([]byte(pol), users, nodes.ViewSlice()) + require.NoError(t, err) + + result := pm.ViaRoutesForPeer(nodes[0].View(), nodes[1].View()) + require.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.Include) + require.Empty(t, result.Exclude) + + result = pm.ViaRoutesForPeer(nodes[0].View(), nodes[2].View()) + require.Empty(t, result.Include) + require.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.Exclude) + }) } // TestBuildPeerMap_AutogroupInternetMakesExitNodeVisible reproduces