From 1b76d9071a1c0ba4e2c746a2c669bfc46466fe84 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 13:17:51 +0000 Subject: [PATCH] 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 | 23 ++++ hscontrol/policy/v2/policy.go | 91 ++++++++----- hscontrol/policy/v2/policy_test.go | 174 ++++++++++++++++++++++++ hscontrol/servertest/via_compat_test.go | 9 -- 4 files changed, 256 insertions(+), 41 deletions(-) diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index 37441c22b..4daa314a0 100644 --- a/hscontrol/policy/v2/filter_test.go +++ b/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) +} diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 518247aad..ede844fbe 100644 --- a/hscontrol/policy/v2/policy.go +++ b/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) } } } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 0f01bb16e..3f7485ed7 100644 --- a/hscontrol/policy/v2/policy_test.go +++ b/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 diff --git a/hscontrol/servertest/via_compat_test.go b/hscontrol/servertest/via_compat_test.go index f698d4d92..127eb4bb6 100644 --- a/hscontrol/servertest/via_compat_test.go +++ b/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 /rename//. - "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