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

@@ -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"
 )
 

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 (

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()))
+}

hscontrol/state/state.go

@@ -120,6 +120,7 @@ var nodeUpdateColumns = []string{
 	"Expiry",
 	"LastSeen",
 	"ApprovedRoutes",
+	"AutoApprovedRoutes",
 	"UpdatedAt",
 }
 
@@ -1156,11 +1157,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
 	})
 }
 
@@ -1454,11 +1455,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())).
@@ -1467,7 +1472,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).
@@ -1483,8 +1491,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.
@@ -3631,10 +3674,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.
@@ -3721,40 +3763,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)
 
@@ -3780,11 +3789,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

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")