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