From e481fe49d32ee44c7cb2483d2cb52716dab7fbdb Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Tue, 27 Oct 2026 15:04:40 +0000 Subject: [PATCH] state: drop auto-approved routes when they are withdrawn Only routes auto-approval alone granted go; approvals from before the upgrade stay, as do routes an admin set. --- hscontrol/servertest/mapstream_compat_test.go | 6 - hscontrol/servertest/reauth_compat_test.go | 7 +- hscontrol/state/autoapprove_test.go | 135 ++++++++++++++++++ hscontrol/state/state.go | 127 +++++++++------- integration/route_test.go | 6 +- 5 files changed, 215 insertions(+), 66 deletions(-) diff --git a/hscontrol/servertest/mapstream_compat_test.go b/hscontrol/servertest/mapstream_compat_test.go index 7f5523e31..12d69228e 100644 --- a/hscontrol/servertest/mapstream_compat_test.go +++ b/hscontrol/servertest/mapstream_compat_test.go @@ -127,11 +127,6 @@ var mapStreamSkipReasons = map[string]string{ "policy-generation/G4/thor-1/peer-owners": sameUserPeers, "policy-generation/G4/thor-2/peer-owners": sameUserPeers, - "policy-generation/G4/rt/enabled": withdrawnRouteEnabled, - "policy-generation/G5/rt/enabled": withdrawnRouteEnabled, - "policy-generation/G7/rt/enabled": withdrawnRouteEnabled, - "policy-generation/G8/rt/enabled": withdrawnRouteEnabled, - "user-role/S1-reconnect/own-a/frames": reconnectAnnounced, "user-role/S1-reconnect/own-b/frames": reconnectAnnounced, "user-role/S2-reconnect/own-a/frames": reconnectAnnounced, @@ -150,7 +145,6 @@ const ( peerHostinfoWhole = "PEER_HOSTINFO_WHOLE: headscale sends peers a node's whole reported Hostinfo; SaaS sends only Hostname, OS, Services, the SSH host keys and AppConnector" peerServicesWhole = "PEER_SERVICES_WHOLE: headscale sends peers every service a node reports; SaaS leaves out peerapi-dns-proxy" removalSentToAll = "REMOVAL_SENT_TO_ALL: headscale tells every connected node that a deleted node is gone; SaaS tells only the nodes that had it as a peer" - withdrawnRouteEnabled = "WITHDRAWN_ROUTE_ENABLED: headscale's routes API keeps a route the device stopped advertising in enabledRoutes; SaaS lists only advertised routes as enabled" taggedDeviceUser = "TAGGED_DEVICE_USER: SaaS shows a tagged device's user as the login that made its key; headscale shows tagged-devices" ) diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index f439e1dd4..6a5313fff 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -43,10 +43,6 @@ var reauthSkipReasons = map[string]string{ "owner-routes/r-admin/owner/served": adminRetagNoApproval, "owner-routes/r-admin/owner/self-routes": adminRetagNoApproval, - "owner-routes/r-retag/withdraw/enabled": withdrawKeepsAutoApproval, - "owner-routes/r-expired-retag/withdraw/enabled": withdrawKeepsAutoApproval, - "owner-routes/u-convert/withdraw/enabled": withdrawKeepsAutoApproval, - "identity/w-tagreq-relogin-tags/w-tagreq/owner": keyLoginTagsRefused, "identity/w-tagreq-relogin-tags/w-tagreq/outcome": keyLoginTagsRefused, "identity/w-tagreq-relogin-tags/w-tagreq/node-key": keyLoginTagsRefused, @@ -86,8 +82,7 @@ var reauthSkipReasons = map[string]string{ } const ( - adminRetagNoApproval = "ADMIN_RETAG_NO_APPROVAL: an admin tag change does not auto-approve the routes the new tags may approve; SaaS approves them at once" - withdrawKeepsAutoApproval = "WITHDRAW_KEEPS_AUTO_APPROVAL: headscale keeps an auto-approved route once it is withdrawn; SaaS drops it and keeps only admin-set approvals" + adminRetagNoApproval = "ADMIN_RETAG_NO_APPROVAL: an admin tag change does not auto-approve the routes the new tags may approve; SaaS approves them at once" ) const ( diff --git a/hscontrol/state/autoapprove_test.go b/hscontrol/state/autoapprove_test.go index b0b75f1ce..734004c0f 100644 --- a/hscontrol/state/autoapprove_test.go +++ b/hscontrol/state/autoapprove_test.go @@ -4,6 +4,7 @@ import ( "net/netip" "testing" + "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" @@ -214,3 +215,137 @@ func approvedRoutes(t *testing.T, s *State, id types.NodeID) []netip.Prefix { return nv.ApprovedRoutes().AsSlice() } + +// TestWithdrawnRouteKeepsOnlyAdminApprovals: a route only auto-approval +// granted is dropped once the router stops announcing it and approved again +// when it comes back; a route an admin approved stays, also when +// autoApprovers cover it, and so does every approval made before the upgrade. +func TestWithdrawnRouteKeepsOnlyAdminApprovals(t *testing.T) { + auto := netip.MustParsePrefix("10.0.1.0/24") + admin := netip.MustParsePrefix("10.0.2.0/24") + + for _, tt := range []struct { + name string + setup func(t *testing.T, s *State, id types.NodeID) // after announcing both routes + want []netip.Prefix // approved after the withdrawal + }{ + {name: "auto only", want: nil}, + { + name: "admin overlap", + setup: func(t *testing.T, s *State, id types.NodeID) { + t.Helper() + + _, _, err := s.SetApprovedRoutes(id, []netip.Prefix{auto, admin}) + require.NoError(t, err) + }, + want: []netip.Prefix{auto, admin}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + dbPath, s, id := persistTestSetup(t) + + // Stored, so the restart below keeps it. + _, err := s.db.SetPolicy(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["persist-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`) + require.NoError(t, err) + _, err = s.ReloadPolicy() + require.NoError(t, err) + + announce(t, s, id, auto, admin) + require.ElementsMatch(t, []netip.Prefix{auto, admin}, approvedRoutes(t, s, id)) + + if tt.setup != nil { + tt.setup(t, s, id) + } + + // The marks are stored, so a restart between keeps them. + require.NoError(t, s.Close()) + s = persistTestReopen(t, dbPath) + + announce(t, s, id) + assert.ElementsMatch(t, tt.want, approvedRoutes(t, s, id), "after the withdrawal") + + stored, err := s.db.GetNodeByID(id) + require.NoError(t, err) + assert.ElementsMatch(t, tt.want, stored.ApprovedRoutes.List()) + + announce(t, s, id, auto, admin) + assert.ElementsMatch(t, []netip.Prefix{auto, admin}, approvedRoutes(t, s, id), "announced again") + }) + } + + t.Run("approved before the upgrade", func(t *testing.T) { + dbPath, s, id := persistTestSetup(t) + require.NoError(t, s.Close()) + + // A row as the migration leaves it: approved, with no marks. + database, err := db.NewHeadscaleDatabase(persistTestConfig(dbPath)) + require.NoError(t, err) + require.NoError(t, database.DB.Exec( + `UPDATE nodes SET approved_routes = ?, auto_approved_routes = NULL, host_info = ? WHERE id = ?`, + `["10.0.1.0/24"]`, `{"RoutableIPs":["10.0.1.0/24"]}`, id).Error) + require.NoError(t, database.Close()) + + s = persistTestReopen(t, dbPath) + + _, err = s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["persist-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + announce(t, s, id) + assert.Equal(t, []netip.Prefix{auto}, approvedRoutes(t, s, id)) + }) +} + +// announce sends a map request in which node id announces routes. +func announce(t *testing.T, s *State, id types.NodeID, routes ...netip.Prefix) { + t.Helper() + + nv, ok := s.GetNodeByID(id) + require.True(t, ok) + + _, err := s.UpdateNodeFromMapRequest(id, tailcfg.MapRequest{ + NodeKey: nv.NodeKey(), + DiscoKey: nv.DiscoKey(), + Hostinfo: &tailcfg.Hostinfo{Hostname: nv.Hostname(), RoutableIPs: routes}, + }) + require.NoError(t, err) +} + +// TestRegistrationDropsWithdrawnAutoApprovals: a registration that stops +// announcing an auto-approved route drops its approval too. +func TestRegistrationDropsWithdrawnAutoApprovals(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("withdraw-user") + + _, err := s.SetPolicy([]byte(`{ + "autoApprovers": {"routes": {"10.0.0.0/16": ["withdraw-user@"]}}, + "acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + + kept := netip.MustParsePrefix("10.0.1.0/24") + withdrawn := netip.MustParsePrefix("10.0.2.0/24") + machineKey := key.NewMachine().Public() + req := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: key.NewNode().Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{kept, withdrawn}}, + } + + node, _, err := s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + require.Equal(t, []netip.Prefix{kept, withdrawn}, approvedRoutes(t, s, node.ID())) + + req.Hostinfo = &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{kept}} + _, _, err = s.HandleNodeFromPreAuthKey(req, machineKey) + require.NoError(t, err) + assert.Equal(t, []netip.Prefix{kept}, approvedRoutes(t, s, node.ID())) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index c8ab64de8..105f908e8 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -119,6 +119,7 @@ var nodeUpdateColumns = []string{ "Expiry", "LastSeen", "ApprovedRoutes", + "AutoApprovedRoutes", "UpdatedAt", } @@ -1215,11 +1216,11 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, // It returns a PolicyChange when a primary moved, the policy manager saw a // policy input change, or the node's peers changed; otherwise NodeAdded. func (s *State) SetApprovedRoutes(nodeID types.NodeID, routes []netip.Prefix) (types.NodeView, change.Change, error) { - // TODO(kradalby): In principle we should call the AutoApprove logic here - // because even if the CLI removes an auto-approved route, it will be added - // back automatically. + // Every route an admin sets is the admin's, even one autoApprovers + // cover, so a withdrawal keeps it. return s.updateApprovedRoutes(nodeID, func(node *types.Node) { node.ApprovedRoutes = routes + node.AutoApprovedRoutes = nil }) } @@ -1513,11 +1514,15 @@ func (s *State) SetPolicy(pol []byte) (bool, error) { return changed, nil } -// AutoApproveRoutes checks if a node's routes should be auto-approved. -// AutoApproveRoutes checks if any routes should be auto-approved for a node and updates them. +// AutoApproveRoutes approves the announced routes the policy lets a node +// approve, and drops the auto-approved routes it no longer announces. func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { approved, changed := policy.ApproveRoutesWithPolicy(s.polMan, nv, nv.ApprovedRoutes().AsSlice(), nv.AnnouncedRoutes()) - if changed { + withdrawn := slices.ContainsFunc(nv.AutoApprovedRoutes().AsSlice(), func(route netip.Prefix) bool { + return !slices.Contains(nv.AnnouncedRoutes(), route) + }) + + if changed || withdrawn { log.Debug(). EmbedObject(nv). Strs("routes.announced", util.PrefixesToString(nv.AnnouncedRoutes())). @@ -1526,7 +1531,10 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { Msg("Single node auto-approval detected route changes") // Recompute on the current node so an older view cannot undo an admin write. - _, c, err := s.updateApprovedRoutes(nv.ID(), s.approveAnnouncedRoutes) + _, c, err := s.updateApprovedRoutes(nv.ID(), func(n *types.Node) { + dropWithdrawnAutoApprovals(n) + s.approveAnnouncedRoutes(n) + }) if err != nil { log.Error(). EmbedObject(nv). @@ -1542,8 +1550,43 @@ func (s *State) AutoApproveRoutes(nv types.NodeView) (change.Change, error) { return change.Change{}, nil } -func (s *State) approveAnnouncedRoutes(n *types.Node) { - n.ApprovedRoutes, _ = policy.ApproveRoutesWithPolicy(s.polMan, n.View(), n.ApprovedRoutes, n.AnnouncedRoutes()) +// approveAnnouncedRoutes approves the announced routes the policy lets n +// approve, and records the ones it adds as auto-approved. +func (s *State) approveAnnouncedRoutes(n *types.Node) bool { + approved, changed := policy.ApproveRoutesWithPolicy(s.polMan, n.View(), n.ApprovedRoutes, n.AnnouncedRoutes()) + if !changed { + return false + } + + for _, route := range approved { + if !slices.Contains(n.ApprovedRoutes, route) { + n.AutoApprovedRoutes = append(n.AutoApprovedRoutes, route) + } + } + + n.ApprovedRoutes = approved + + return true +} + +// dropWithdrawnAutoApprovals removes the auto-approved routes n no longer +// announces, as Tailscale SaaS does. Routes an admin approved stay. +func dropWithdrawnAutoApprovals(n *types.Node) bool { + announced := n.AnnouncedRoutes() + withdrawn := func(route netip.Prefix) bool { + return slices.Contains(n.AutoApprovedRoutes, route) && !slices.Contains(announced, route) + } + + if !slices.ContainsFunc(n.ApprovedRoutes, withdrawn) { + return false + } + + n.ApprovedRoutes = slices.DeleteFunc(slices.Clone(n.ApprovedRoutes), withdrawn) + n.AutoApprovedRoutes = slices.DeleteFunc(slices.Clone(n.AutoApprovedRoutes), func(route netip.Prefix) bool { + return !slices.Contains(announced, route) + }) + + return true } // GetPolicy retrieves the current policy from the database. @@ -3713,10 +3756,9 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest Msg("Processing MapRequest for node") var ( - delta mapRequestDelta - routeChange bool - autoApprovedRoutes []netip.Prefix - staleNodeKey bool + delta mapRequestDelta + routeChange bool + staleNodeKey bool ) // Snapshot the primary assignment so we can tell whether the // Hostinfo + auto-approval that follows shifted any prefix. @@ -3803,40 +3845,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest hostinfoToStore := delta.hostinfoChanged || delta.derpChanged - // Approvers run over every announced route, but only when the - // announced set changes: any other report would re-grant a route - // an admin revoked. - if delta.routesChanged { - // Extract announced routes from request - var announcedRoutes []netip.Prefix - if req.Hostinfo != nil { - announcedRoutes = req.Hostinfo.RoutableIPs - } - - // Apply policy-based auto-approval if routes are announced - if len(announcedRoutes) > 0 { - autoApprovedRoutes, routeChange = policy.ApproveRoutesWithPolicy( - s.polMan, - currentNode.View(), - currentNode.ApprovedRoutes, - announcedRoutes, - ) - } - } - - // Log when routes change but approval doesn't - if delta.routesChanged && !routeChange { - if hi := req.Hostinfo; hi != nil { - log.Debug(). - Caller(). - Uint64(zf.NodeID, id.Uint64()). - Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())). - Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(hi.RoutableIPs)). - Strs(zf.ApprovedRoutes, util.PrefixesToString(currentNode.ApprovedRoutes)). - Bool(zf.RouteChanged, routeChange). - Msg("announced routes changed but approved routes did not") - } - } + oldAnnounced := currentNode.AnnouncedRoutes() currentNode.ApplyPeerChange(&delta.peerChange) @@ -3862,11 +3871,25 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest } } - if routeChange { - // Approving in this write keeps one request at one NodeStore - // write, one peer build and one row update. Persisting is - // already due: route approval only runs on a Hostinfo change. - currentNode.ApprovedRoutes = autoApprovedRoutes + // Approval follows the announced set, only when it changes: any + // other report would re-grant a route an admin revoked. Deciding in + // this write keeps one request at one NodeStore write, one peer + // build and one row update; persisting is already due, as the + // Hostinfo changed. + if delta.routesChanged { + oldApproved := currentNode.ApprovedRoutes + dropped := dropWithdrawnAutoApprovals(currentNode) + routeChange = s.approveAnnouncedRoutes(currentNode) || dropped + + log.Debug(). + Caller(). + Uint64(zf.NodeID, id.Uint64()). + Strs(zf.OldAnnouncedRoutes, util.PrefixesToString(oldAnnounced)). + Strs(zf.NewAnnouncedRoutes, util.PrefixesToString(currentNode.AnnouncedRoutes())). + Strs(zf.RoutesApprovedOld, util.PrefixesToString(oldApproved)). + Strs(zf.RoutesApprovedNew, util.PrefixesToString(currentNode.ApprovedRoutes)). + Bool(zf.RouteChanged, routeChange). + Msg("announced routes changed") } // AllApprovedRoutes is announced ∩ approved; a Hostinfo diff --git a/integration/route_test.go b/integration/route_test.go index 85aa85441..6f33e329c 100644 --- a/integration/route_test.go +++ b/integration/route_test.go @@ -3008,7 +3008,9 @@ func TestAutoApproveMultiNetwork(t *testing.T) { nodes, err = headscale.ListNodes() assert.NoError(c, err) requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 1, 1, 1) - requireNodeRouteCountWithCollect(c, nodes[1], 1, 1, 0) + // The withdrawn sub-route was auto-approved, so it loses + // its approval. + requireNodeRouteCountWithCollect(c, nodes[1], 1, 0, 0) requireNodeRouteCountWithCollect(c, nodes[2], 0, 0, 0) }, assertTimeout, 500*time.Millisecond, "route state changes should propagate") @@ -3048,7 +3050,7 @@ func TestAutoApproveMultiNetwork(t *testing.T) { nodes, err = headscale.ListNodes() assert.NoError(c, err) requireNodeRouteCountWithCollect(c, MustFindNode(routerUsernet1.Hostname(), nodes), 1, 1, 1) - requireNodeRouteCountWithCollect(c, nodes[1], 1, 1, 0) + requireNodeRouteCountWithCollect(c, nodes[1], 1, 0, 0) requireNodeRouteCountWithCollect(c, nodes[2], 2, 2, 2) }, assertTimeout, 500*time.Millisecond, "route state changes should propagate")