From 60f0b20d76a6d932408a0251c0c0e4ded7ade17a Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 13:17:51 +0000 Subject: [PATCH] policy/v2: refresh clients when a user change drops the last via grant SetUsers read needsPerNodeFilter after the recompile cleared it, so the change was reported as empty and clients kept the via rules. --- hscontrol/policy/v2/policy.go | 7 ++++- hscontrol/policy/v2/policy_test.go | 39 +++++++++++++++++++++++++ hscontrol/servertest/via_compat_test.go | 9 ------ 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 518247aad..cb5b23984 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 } diff --git a/hscontrol/policy/v2/policy_test.go b/hscontrol/policy/v2/policy_test.go index 0f01bb16e..615c3df09 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 { 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