state: auto-approve on registration only when the node key rotates

Registration runs the approval itself, once the policy manager knows the
node's owner, for a new node key or a change to the announced routes.

hscontrol/api/v1/auth.go

@@ -90,19 +90,12 @@ func registerAuth(api huma.API, b Backend) {
 			nil,
 			util.RegisterMethodCLI,
 		)
-		if err != nil {
-			b.Change(nodeChange)
+		b.Change(nodeChange)
 
+		if err != nil {
 			return nil, mapError("registering node", err)
 		}
 
-		routeChange, err := b.State.AutoApproveRoutes(node)
-		b.Change(nodeChange, routeChange)
-
-		if err != nil {
-			return nil, huma.Error500InternalServerError("auto approving routes", err)
-		}
-
 		out := &authRegisterOutput{}
 		out.Body.Node = nodeFromView(node)
 

hscontrol/api/v1/nodes.go

@@ -487,19 +487,11 @@ func registerNodeAdminOps(api huma.API, b Backend) {
 			nil,
 			util.RegisterMethodCLI,
 		)
-		if err != nil {
-			b.Change(nodeChange)
-
-			return nil, mapError("registering node", err)
-		}
-
-		routeChange, err := b.State.AutoApproveRoutes(node)
-
 		// Empty changes are ignored by the change sink.
-		b.Change(nodeChange, routeChange)
+		b.Change(nodeChange)
 
 		if err != nil {
-			return nil, huma.Error500InternalServerError("auto approving routes", err)
+			return nil, mapError("registering node", err)
 		}
 
 		out := &nodeOutput{}

hscontrol/auth.go

@@ -472,27 +472,7 @@ func (h *Headscale) handleRegisterWithAuthKey(
 		return nil, nil //nolint:nilnil // intentional: no node to return when ephemeral deleted
 	}
 
-	// This is a bit of a back and forth, but we have a bit of a chicken and egg
-	// dependency here.
-	// Because the way the policy manager works, we need to have the node
-	// in the database, then add it to the policy manager and then we can
-	// approve the route. This means we get this dance where the node is
-	// first added to the database, then we add it to the policy manager via
-	// nodesChangedHook and then we can auto approve the routes.
-	// As that only approves the struct object, we need to save it again and
-	// ensure we send an update.
-	// This works, but might be another good candidate for doing some sort of
-	// eventbus.
-	// TODO(kradalby): This needs to be ran as part of the batcher maybe?
-	// now since we dont update the node/pol here anymore
-	routesChange, err := h.state.AutoApproveRoutes(node)
-
-	// Send both changes. Empty changes are ignored by Change().
-	h.Change(changed, routesChange)
-
-	if err != nil {
-		return nil, fmt.Errorf("auto approving routes: %w", err)
-	}
+	h.Change(changed)
 
 	resp := &tailcfg.RegisterResponse{
 		MachineAuthorized: true,

hscontrol/oidc.go

@@ -1001,38 +1001,18 @@ func (a *AuthProviderOIDC) handleRegistration(
 	registrationID types.AuthID,
 	expiry *time.Time,
 ) (bool, error) {
-	node, nodeChange, err := a.h.state.HandleNodeFromAuthPath(
+	_, nodeChange, err := a.h.state.HandleNodeFromAuthPath(
 		registrationID,
 		types.UserID(user.ID),
 		expiry,
 		util.RegisterMethodOIDC,
 	)
-	if err != nil {
-		a.h.Change(nodeChange)
+	a.h.Change(nodeChange)
 
+	if err != nil {
 		return false, fmt.Errorf("registering node: %w", err)
 	}
 
-	// This is a bit of a back and forth, but we have a bit of a chicken and egg
-	// dependency here.
-	// Because the way the policy manager works, we need to have the node
-	// in the database, then add it to the policy manager and then we can
-	// approve the route. This means we get this dance where the node is
-	// first added to the database, then we add it to the policy manager via
-	// SaveNode (which automatically updates the policy manager) and then we can auto approve the routes.
-	// As that only approves the struct object, we need to save it again and
-	// ensure we send an update.
-	// This works, but might be another good candidate for doing some sort of
-	// eventbus.
-	routesChange, err := a.h.state.AutoApproveRoutes(node)
-
-	// Send both changes. Empty changes are ignored by Change().
-	a.h.Change(nodeChange, routesChange)
-
-	if err != nil {
-		return false, fmt.Errorf("auto approving routes: %w", err)
-	}
-
 	return !nodeChange.IsEmpty(), nil
 }
 

hscontrol/servertest/lifecycle_compat_test.go

@@ -36,10 +36,6 @@ import (
 // the step of what killed nodes leave behind. A listed check that matches
 // fails, so a fix must delete its entry.
 var lifecycleSkipReasons = map[string]string{
-	"route-approval/b9/enabled":     approvalOnSameKeyReauth,
-	"route-approval/b9/served":      approvalOnSameKeyReauth,
-	"route-approval/b9/self-routes": approvalOnSameKeyReauth,
-
 	"ephemeral-lifecycle/E1-logout/eph-logout/outcome":      logoutAnswerUnauthorized,
 	"ephemeral-lifecycle/E1-logout/eph-logout-user/outcome": logoutAnswerUnauthorized,
 
@@ -58,12 +54,6 @@ var lifecycleSkipReasons = map[string]string{
 	"ephemeral-lifecycle/collection/perm-to-eph/present-observer": ephemeralFollowsKey,
 }
 
-// SaaS runs auto-approval when a router's announced routes change and on a
-// re-auth that rotates its node key, over every announced route.
-const (
-	approvalOnSameKeyReauth = "APPROVAL_ON_SAME_KEY_REAUTH: headscale auto-approves on a re-auth that keeps the node key; SaaS does not"
-)
-
 const (
 	logoutAnswerUnauthorized = "LOGOUT_ANSWER_UNAUTHORIZED: headscale answers the logout that deletes an ephemeral node with MachineAuthorized false; SaaS answers true"
 	deletedKeyListed         = "DELETED_KEY_LISTED: headscale lists a deleted auth key; SaaS leaves it out of both key lists, while a read by ID still answers"

hscontrol/state/autoapprove_test.go

@@ -5,9 +5,11 @@ import (
 	"testing"
 
 	"github.com/juanfont/headscale/hscontrol/types"
+	"github.com/juanfont/headscale/hscontrol/types/change"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"tailscale.com/tailcfg"
+	"tailscale.com/types/key"
 )
 
 // TestRouteApprovalWaitsForAnnouncement: approvers run when the announced
@@ -113,3 +115,102 @@ func TestAutoApproveKeepsCurrentManualRoutes(t *testing.T) {
 		})
 	}
 }
+
+// TestRegistrationApprovesOnlyWithNewNodeKey: a registration runs route
+// auto-approval when it brings a new node key or a change to the announced
+// routes, not on a login that keeps both.
+func TestRegistrationApprovesOnlyWithNewNodeKey(t *testing.T) {
+	s := newRetagTestState(t)
+	user := s.CreateUserForTest("approve-user")
+
+	_, err := s.SetPolicy([]byte(`{
+		"autoApprovers": {"routes": {"10.0.0.0/16": ["approve-user@"]}},
+		"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
+	}`))
+	require.NoError(t, err)
+
+	pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+	require.NoError(t, err)
+
+	route := netip.MustParsePrefix("10.0.1.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{route}},
+	}
+
+	node, _, err := s.HandleNodeFromPreAuthKey(req, machineKey)
+	require.NoError(t, err)
+	require.Equal(t, []netip.Prefix{route}, approvedRoutes(t, s, node.ID()), "a new node")
+
+	_, _, err = s.SetApprovedRoutes(node.ID(), nil)
+	require.NoError(t, err)
+
+	_, _, err = s.HandleNodeFromPreAuthKey(req, machineKey)
+	require.NoError(t, err)
+	assert.Empty(t, approvedRoutes(t, s, node.ID()), "the same node key")
+
+	added := netip.MustParsePrefix("10.0.2.0/24")
+	req.Hostinfo = &tailcfg.Hostinfo{Hostname: "router", RoutableIPs: []netip.Prefix{route, added}}
+	_, _, err = s.HandleNodeFromPreAuthKey(req, machineKey)
+	require.NoError(t, err)
+	assert.Equal(t, []netip.Prefix{route, added}, approvedRoutes(t, s, node.ID()), "a new announced route")
+
+	_, _, err = s.SetApprovedRoutes(node.ID(), nil)
+	require.NoError(t, err)
+
+	req.NodeKey = key.NewNode().Public()
+	_, _, err = s.HandleNodeFromPreAuthKey(req, machineKey)
+	require.NoError(t, err)
+	assert.Equal(t, []netip.Prefix{route, added}, approvedRoutes(t, s, node.ID()), "a new node key")
+}
+
+// TestRegistrationApprovalComparesReplacedNode: registration approval compares
+// with the node the writer replaced, so an announcement and an admin revoke
+// that land after the lookup leave an unchanged re-registration approving
+// nothing.
+func TestRegistrationApprovalComparesReplacedNode(t *testing.T) {
+	s := newRetagTestState(t)
+	user := s.CreateUserForTest("approve-user")
+
+	_, err := s.SetPolicy([]byte(`{
+		"autoApprovers": {"routes": {"10.0.0.0/16": ["approve-user@"]}},
+		"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]
+	}`))
+	require.NoError(t, err)
+
+	pak, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+	require.NoError(t, err)
+
+	c := registerForPAKRereg(t, s, pak)
+	stale, stalePAK := c.lookup(t, pak.Key)
+
+	route := netip.MustParsePrefix("10.0.1.0/24")
+	hi := &tailcfg.Hostinfo{Hostname: "h0", RoutableIPs: []netip.Prefix{route}}
+
+	_, err = s.UpdateNodeFromMapRequest(c.node.ID(), tailcfg.MapRequest{NodeKey: c.regReq.NodeKey, Hostinfo: hi})
+	require.NoError(t, err)
+	require.Equal(t, []netip.Prefix{route}, approvedRoutes(t, s, c.node.ID()), "the announcement approves")
+
+	_, _, err = s.SetApprovedRoutes(c.node.ID(), nil)
+	require.NoError(t, err)
+
+	req := c.regReq
+	req.Hostinfo = hi.Clone()
+	node, replaced, err := s.reregisterNodeWithPAK(stale, stalePAK, req, c.machineKey, hi.Hostname, req.Hostinfo)
+	require.NoError(t, err)
+
+	_, _, err = s.approveRoutesAfterRegistration(replaced, node, change.Change{})
+	require.NoError(t, err)
+	assert.Empty(t, approvedRoutes(t, s, c.node.ID()), "the admin revoke stands")
+}
+
+func approvedRoutes(t *testing.T, s *State, id types.NodeID) []netip.Prefix {
+	t.Helper()
+
+	nv, ok := s.GetNodeByID(id)
+	require.True(t, ok)
+
+	return nv.ApprovedRoutes().AsSlice()
+}

hscontrol/state/state.go

@@ -2158,10 +2158,9 @@ func sameOwner(a, b types.NodeView) bool {
 
 // applyAuthNodeUpdate applies common update logic for re-authenticating or converting
 // an existing node. It updates the node in [NodeStore], processes RequestTags, and
-// persists changes to the database. The bool reports whether peers read the
-// node it replaced and the update the same, apart from its keys; see
-// [onlyKeysChanged].
-func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, bool, error) {
+// persists changes to the database. It also returns the node the NodeStore
+// writer replaced.
+func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, types.NodeView, error) {
 	regData := params.RegData
 	// Log the operation type
 	if params.IsConvertFromTag {
@@ -2202,7 +2201,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 
 	rejectedTags := s.validateRequestTagsForReauth(params.ExistingNode, authUser, requestTags)
 	if len(rejectedTags) > 0 {
-		return types.NodeView{}, false, fmt.Errorf(
+		return types.NodeView{}, types.NodeView{}, fmt.Errorf(
 			"%w %v are invalid or not permitted",
 			ErrRequestedTagsInvalidOrNotPermitted,
 			rejectedTags,
@@ -2217,7 +2216,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 	// the NodeStore NodeKey index (denying the victim service).
 	if existing, ok := s.nodeStore.GetNodeByNodeKey(regData.NodeKey); ok &&
 		existing.MachineKey() != regData.MachineKey {
-		return types.NodeView{}, false, ErrNodeKeyInUse
+		return types.NodeView{}, types.NodeView{}, ErrNodeKeyInUse
 	}
 
 	// Update existing node in [NodeStore] - validation passed, safe to mutate
@@ -2319,11 +2318,9 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 	})
 
 	if !ok {
-		return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID())
+		return types.NodeView{}, types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID())
 	}
 
-	keysOnly := onlyKeysChanged(before, updatedNodeView, time.Now())
-
 	// Persist to database.
 	// Explicitly select all node columns so GORM includes nil/zero-value fields
 	// (see nodeUpdateColumns comment).
@@ -2342,7 +2339,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 
 	_, err := s.persistNodeWith(updatedNodeView, updateColumns, nil)
 	if err != nil {
-		return types.NodeView{}, false, err
+		return types.NodeView{}, types.NodeView{}, err
 	}
 
 	// Log completion
@@ -2356,7 +2353,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 			Msg("Node re-authorized")
 	}
 
-	return updatedNodeView, keysOnly, nil
+	return updatedNodeView, before, nil
 }
 
 // createAndSaveNewNode creates a new node, allocates IPs, saves to DB, and adds to [NodeStore].
@@ -2764,21 +2761,24 @@ func (s *State) HandleNodeFromAuthPath(
 
 	var (
 		finalNode types.NodeView
+		before    types.NodeView
 		keysOnly  bool
 	)
 
 	if nodeExists && !existingNode.IsTagged() {
 		updateParams.ExistingNode = existingNode
 
-		finalNode, keysOnly, err = s.applyAuthNodeUpdate(updateParams)
+		finalNode, before, err = s.applyAuthNodeUpdate(updateParams)
 		if err != nil {
 			return types.NodeView{}, s.policyChangeSince(genBefore), err
 		}
+
+		keysOnly = onlyKeysChanged(before, finalNode, time.Now())
 	} else if nodeExists {
 		updateParams.ExistingNode = existingNode
 		updateParams.IsConvertFromTag = true
 
-		finalNode, _, err = s.applyAuthNodeUpdate(updateParams)
+		finalNode, before, err = s.applyAuthNodeUpdate(updateParams)
 		if err != nil {
 			return types.NodeView{}, s.policyChangeSince(genBefore), err
 		}
@@ -2835,7 +2835,11 @@ func (s *State) HandleNodeFromAuthPath(
 	// keysOnly is set only for a same-user relogin; a tag->user conversion is
 	// excluded, as it changes the peer's User — a structural change peers must
 	// see in full, not a key-rotation patch.
-	return finalNode, reauthChange(finalNode, keysOnly, policyChanged), nil
+	return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, keysOnly, policyChanged))
+}
+
+func sortedPrefixes(p []netip.Prefix) []netip.Prefix {
+	return slices.SortedFunc(slices.Values(p), netip.Prefix.Compare)
 }
 
 // createNewNodeFromAuth creates a new node during auth callback.
@@ -3107,15 +3111,19 @@ func (s *State) HandleNodeFromPreAuthKey(
 		keysOnly  bool
 	)
 
+	var before types.NodeView
+
 	// If this node exists for this user, update the node in place. For a
 	// tags-only key (pak.User == nil) this is true when the machine already has
 	// a tagged node (findExistingNodeForPAK matches it under UserID 0); for a
 	// user-owned key it is true when the same user already has the node.
 	if existsSameUser && existingNodeSameUser.Valid() {
-		finalNode, keysOnly, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo)
+		finalNode, before, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo)
 		if err != nil {
 			return types.NodeView{}, s.policyChangeSince(genBefore), err
 		}
+
+		keysOnly = onlyKeysChanged(before, finalNode, time.Now())
 	} else {
 		// Node does not exist for this user with this machine key.
 		// For a user-owned key, check whether the machine key is already held
@@ -3209,7 +3217,34 @@ func (s *State) HandleNodeFromPreAuthKey(
 
 	policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty()
 
-	return finalNode, reauthChange(finalNode, keysOnly, policyChanged), nil
+	return s.approveRoutesAfterRegistration(before, finalNode, reauthChange(finalNode, keysOnly, policyChanged))
+}
+
+// approveRoutesAfterRegistration runs route auto-approval after a
+// registration whose NodeStore writer replaced before with node, merging its
+// change into c. As on Tailscale SaaS it runs for a new node, a new node key
+// or a change to the announced routes; a login that changes neither approves
+// nothing. before is what the writer replaced, not the caller's lookup, so an
+// announcement and an admin revoke landing in between do not count as a
+// change. It waits for the registration: the policy manager must know the
+// node's owner first.
+func (s *State) approveRoutesAfterRegistration(
+	before, node types.NodeView,
+	c change.Change,
+) (types.NodeView, change.Change, error) {
+	if before.Valid() && before.NodeKey() == node.NodeKey() &&
+		slices.Equal(sortedPrefixes(before.AnnouncedRoutes()), sortedPrefixes(node.AnnouncedRoutes())) {
+		return node, c, nil
+	}
+
+	routesChange, err := s.AutoApproveRoutes(node)
+	c = c.Merge(routesChange)
+
+	if err != nil {
+		return node, c, fmt.Errorf("auto approving routes: %w", err)
+	}
+
+	return node, c, nil
 }
 
 // reregisterNodeWithPAK updates an existing node in place for a pre-auth key
@@ -3220,8 +3255,7 @@ func (s *State) HandleNodeFromPreAuthKey(
 // in between, and the key or the node can expire. So the writer decides
 // whether the key must be valid on the node it replaces, at its own clock, and
 // leaves the node untouched when it is not. existingNodeSameUser is used only
-// for its ID and for logging. The bool reports whether peers read the node it
-// replaced and the update the same, apart from its keys; see [onlyKeysChanged].
+// for its ID and for logging. It also returns the node the writer replaced.
 func (s *State) reregisterNodeWithPAK(
 	existingNodeSameUser types.NodeView,
 	pak *types.PreAuthKey,
@@ -3229,7 +3263,7 @@ func (s *State) reregisterNodeWithPAK(
 	machineKey key.MachinePublic,
 	hostname string,
 	validHostinfo *tailcfg.Hostinfo,
-) (types.NodeView, bool, error) {
+) (types.NodeView, types.NodeView, error) {
 	log.Trace().
 		Caller().
 		Str(zf.NodeName, existingNodeSameUser.Hostname()).
@@ -3247,7 +3281,7 @@ func (s *State) reregisterNodeWithPAK(
 	// NodeStore NodeKey index, denying the victim service.
 	if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok &&
 		existing.MachineKey() != machineKey {
-		return types.NodeView{}, false, ErrNodeKeyInUse
+		return types.NodeView{}, types.NodeView{}, ErrNodeKeyInUse
 	}
 
 	// prior is the node the writer replaced: the NetInfo source and the
@@ -3372,7 +3406,7 @@ func (s *State) reregisterNodeWithPAK(
 	// A nil prior means the writer never saw the node (deleted, or the store
 	// stopped), so nothing was decided and nothing may be persisted.
 	if !ok || prior == nil {
-		return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID())
+		return types.NodeView{}, types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID())
 	}
 
 	if authErr != nil {
@@ -3384,7 +3418,7 @@ func (s *State) reregisterNodeWithPAK(
 			Err(authErr).
 			Msg("Re-registration needs a valid auth key when it applies, rejecting")
 
-		return types.NodeView{}, false, authErr
+		return types.NodeView{}, types.NodeView{}, authErr
 	}
 
 	// Only re-authentication writes a newly loaded key's reference. A wake-up
@@ -3412,7 +3446,7 @@ func (s *State) reregisterNodeWithPAK(
 	if err != nil {
 		s.undoReregistration(prior, written)
 
-		return types.NodeView{}, false, fmt.Errorf("writing node to database: %w", err)
+		return types.NodeView{}, types.NodeView{}, fmt.Errorf("writing node to database: %w", err)
 	}
 
 	log.Trace().
@@ -3424,7 +3458,7 @@ func (s *State) reregisterNodeWithPAK(
 		Str(zf.UserName, pak.Username()).
 		Msg("Node re-authorized")
 
-	return updatedNodeView, onlyKeysChanged(before, updatedNodeView, time.Now()), nil
+	return updatedNodeView, before, nil
 }
 
 // undoReregistration puts back the fields of a failed re-registration that