policy/v2: keep via steering bounded across route and user changes

Refresh clients when the last via grant disappears. Match each ACL once,
deduplicate routes, and elect once per route the peer actually serves.

hscontrol/policy/v2/filter_test.go

@@ -4260,3 +4260,26 @@ func TestDestinationsToNetPortRange_AutogroupInternet(t *testing.T) {
 		})
 	}
 }
+
+// TestACLSourcesResolvedOnce pins that the grants an ACL is split into, one
+// per destination, share the resolved sources instead of each holding a copy.
+func TestACLSourcesResolvedOnce(t *testing.T) {
+	t.Parallel()
+
+	pol, err := unmarshalPolicy([]byte(`{"acls":[{"action":"accept",
+		"src":["10.0.0.1","10.0.1.1","10.0.2.1"],
+		"dst":["100.64.0.1:80","100.64.0.2:443","100.64.0.3:22"]}]}`))
+	require.NoError(t, err)
+
+	grants := pol.compileGrants(nil, types.Nodes{}.ViewSlice())
+	require.Len(t, grants, 3)
+
+	for _, cg := range grants {
+		require.Equal(t, []string{"10.0.0.1", "10.0.1.1", "10.0.2.1"}, cg.srcIPStrings)
+		require.Same(t, &grants[0].srcIPStrings[0], &cg.srcIPStrings[0])
+	}
+
+	rules := globalFilterRules(grants)
+	require.Len(t, rules, 1)
+	require.Len(t, rules[0].DstPorts, 3)
+}

hscontrol/policy/v2/policy.go

@@ -981,6 +981,10 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) {
 	prev := pm.users
 	pm.users = users
 
+	// A via grant left without sources is not compiled, so the recompile
+	// can clear the flag while clients still hold its rules.
+	hadPerNodeFilter := pm.needsPerNodeFilter
+
 	// SSH policies and autogroup:self sources resolve users by name, and
 	// the self sources are outside the filter hash, so updateLocked can
 	// report no change while per-node results moved.
@@ -1000,7 +1004,8 @@ func (pm *PolicyManager) SetUsers(users []types.User) (bool, bool, error) {
 
 	// SSH rules and per-node filters embed user identity outside the filter
 	// hash, so a user change needs a client refresh even when it did not move.
-	if pm.needsPerNodeFilter || (pm.pol != nil && len(pm.pol.SSHs) > 0) {
+	if hadPerNodeFilter || pm.needsPerNodeFilter ||
+		(pm.pol != nil && len(pm.pol.SSHs) > 0) {
 		policyChanged = true
 	}
 
@@ -1435,11 +1440,24 @@ 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)
+
+	// Ports never steer routes, so an ACL is one grant here, not one per
+	// destination that would match its sources once per destination.
 	for _, acl := range pm.pol.ACLs {
-		grants = append(grants, aclToGrants(acl)...)
+		dsts := make(Aliases, 0, len(acl.Destinations))
+		for _, d := range acl.Destinations {
+			dsts = append(dsts, d.Alias)
+		}
+
+		grants = append(grants, Grant{Sources: acl.Sources, Destinations: dsts})
 	}
 
 	// matchViewer resolves a grant lazily and at most once: non-via
@@ -1499,11 +1517,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)
 			}
 		}
 
@@ -1531,10 +1551,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)
+			}
 		}
 	}
 
@@ -1574,33 +1600,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)
 					}
 				}
 			}

hscontrol/policy/v2/policy_test.go

@@ -558,6 +558,45 @@ func TestSetUsersDropsStaleSelfFilters(t *testing.T) {
 	}
 }
 
+// TestSetUsersReportsLostViaSources: a user change that leaves the only via
+// grant without sources must be reported, or clients keep its rules.
+func TestSetUsersReportsLostViaSources(t *testing.T) {
+	pol := `{
+		"tagOwners": {"tag:router": ["u1@"]},
+		"grants": [{"src": ["u3@"], "dst": ["10.0.0.0/16"], "via": ["tag:router"], "ip": ["*"]}]}`
+
+	u1, u3, x3 := types.User{Name: "u1"}, types.User{Name: "u3"}, types.User{Name: "x3"}
+	u1.ID, u3.ID, x3.ID = 1, 3, 3
+
+	router := node("router", "100.64.0.1", "fd7a:115c:a1e0::1", types.User{})
+	router.User, router.UserID = nil, nil
+	router.Tags = []string{"tag:router"}
+	router.Hostinfo = &tailcfg.Hostinfo{
+		RoutableIPs: []netip.Prefix{netip.MustParsePrefix("10.0.0.0/16")},
+	}
+	router.ApprovedRoutes = router.Hostinfo.RoutableIPs
+
+	nodes := types.Nodes{router, node("u3-a", "100.64.0.3", "fd7a:115c:a1e0::3", u3)}
+	for i, n := range nodes {
+		n.ID = types.NodeID(i + 1) //nolint:gosec // safe conversion in test
+	}
+
+	pm, err := NewPolicyManager([]byte(pol), types.Users{u1, u3}, nodes.ViewSlice())
+	require.NoError(t, err)
+
+	rules, err := pm.FilterForNode(router.View())
+	require.NoError(t, err)
+	require.NotEmpty(t, rules, "the router admits u3 before the rename")
+
+	policyChanged, _, err := pm.SetUsers(types.Users{u1, x3})
+	require.NoError(t, err)
+
+	rules, err = pm.FilterForNode(router.View())
+	require.NoError(t, err)
+	require.Empty(t, rules, "nobody is named u3 any more")
+	require.True(t, policyChanged, "the withdrawn rule must reach clients")
+}
+
 // TestInvalidateGlobalPolicyCache tests the cache invalidation logic for global policies.
 func TestInvalidateGlobalPolicyCache(t *testing.T) {
 	mustIPPtr := func(s string) *netip.Addr {
@@ -1960,6 +1999,72 @@ func TestViaRoutesForPeer(t *testing.T) {
 		require.Empty(t, result.UsePrimary)
 	})
 
+	t.Run("acl_destinations_share_source_match", 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")},
+				},
+				ApprovedRoutes: []netip.Prefix{mp("10.0.0.0/24")},
+			},
+		}
+
+		// Only the last destination of each ACL covers the route, and only
+		// the sources of the second ACL match the viewer.
+		pol := func(secondSrc string) string {
+			return `{
+			"tagOwners": {
+				"tag:router": ["user1@"]
+			},
+			"acls": [{
+				"action": "accept",
+				"src": ["user2@"],
+				"dst": ["192.168.0.0/24:*", "10.0.0.0/24:*"]
+			}, {
+				"action": "accept",
+				"src": ["user2@", "` + secondSrc + `"],
+				"dst": ["192.168.0.0/24:*", "10.0.0.0/24:*"]
+			}],
+			"grants": [{
+				"src": ["user1@"],
+				"dst": ["10.0.0.0/24"],
+				"ip": ["*"],
+				"via": ["tag:router"]
+			}]
+		}`
+		}
+
+		pm, err := NewPolicyManager([]byte(pol("user1@")), 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.Equal(t, []netip.Prefix{mp("10.0.0.0/24")}, result.UsePrimary)
+
+		pm, err = NewPolicyManager([]byte(pol("user2@")), 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.UsePrimary)
+	})
+
 	t.Run("peer_does_not_advertise_destination", func(t *testing.T) {
 		t.Parallel()
 
@@ -2534,6 +2639,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

hscontrol/servertest/via_compat_test.go

@@ -107,13 +107,6 @@ var routesMapCompatTests = []struct {
 var viaCompatSkipReasons = map[string]string{
 	"tagged-self": "TAGGED_SELF_PROFILE: SaaS gives a tagged node a profile of its own, headscale the shared tagged-devices one",
 
-	// TestViaGrantSourceLossCompat, keyed <test_id>/rename/<node>/<check>.
-	"via-grant-v67/rename/squirtle/peers":          viaSourceLossNotSent,
-	"via-grant-v67/rename/squirtle/packet-filter":  viaSourceLossNotSent,
-	"via-grant-v67/rename/blastoise/peers":         viaSourceLossNotSent,
-	"via-grant-v67/rename/blastoise/packet-filter": viaSourceLossNotSent,
-	"via-grant-v67/rename/ivysaur/peers":           viaSourceLossNotSent,
-
 	"routes-f2-subnet-as-acl-source/ivysaur/packet-filter":  filterDstMerged,
 	"routes-f2-subnet-as-acl-source/venusaur/packet-filter": filterDstMerged,
 
@@ -263,8 +256,6 @@ var viaCompatSkipReasons = map[string]string{
 	"routes-m6-subnet-to-subnet-multi-route/m-odin2/peers": sameUserPeers,
 }
 
-const viaSourceLossNotSent = "VIA_SOURCE_LOSS_NOT_SENT: a user change that leaves a via grant without sources does not reach open streams"
-
 const exitRoutesOnOverlap = "EXIT_ROUTES_ON_OVERLAP: headscale offers a peer's exit routes to a viewer whose rules reach only nodes or subnets; SaaS leaves them out"
 
 // Adjacent member addresses merge into one prefix, and the reduced filter