state: need a valid pre-auth key to re-register an expired tagged node

Fixes ANT-2026-LYVGXVSW

(cherry picked from commit e67ef8619a, adapted: inline PAK checks on 0.29, test ported)

hscontrol/state/auth_tagged_expiry_test.go

@@ -992,49 +992,57 @@ func TestIssue3371_TaggedNodePastExpirySelfHealsOnReregister(t *testing.T) {
 	require.Equal(t, nodeID, healed.ID(), "must be the same node")
 }
 
-// TestIssue3371_ExpiredTaggedNodeSameSpentKeyNotRevalidated is the gating test
-// for the isExpired-gate exclusion (state.go: `&& !existingNodeSameUser.IsTagged()`).
-// A tagged node carrying a stale PAST expiry, re-registering with the SAME
-// single-use key and the SAME node key (no rotation), must take the
-// skip-validation fast path — otherwise the spent key is re-validated and
-// rejected with "authkey already used", the exact lockout #3371 fixes. Without
-// the tagged exclusion this fails; with it the node self-heals. The neighbours
-// all rotate the node key or use a reusable key, so validation runs regardless
-// and none probes this fast-path exclusion.
-func TestIssue3371_ExpiredTaggedNodeSameSpentKeyNotRevalidated(t *testing.T) {
-	s := newRetagTestState(t)
+func TestExpiredTaggedNodeRequiresValidPreAuthKey(t *testing.T) {
+	for _, reusable := range []bool{false, true} {
+		t.Run(fmt.Sprintf("reusable=%t", reusable), func(t *testing.T) {
+			s := newRetagTestState(t)
+			pak, err := s.CreatePreAuthKey(nil, reusable, false, nil, []string{"tag:foo"})
+			require.NoError(t, err)
 
-	// Single-use tagged key.
-	pak, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:foo"})
-	require.NoError(t, err)
+			machineKey := key.NewMachine().Public()
+			regReq := tailcfg.RegisterRequest{
+				Auth:     &tailcfg.RegisterResponseAuth{AuthKey: pak.Key},
+				NodeKey:  key.NewNode().Public(),
+				Hostinfo: &tailcfg.Hostinfo{Hostname: "h0"},
+				Expiry:   time.Now().Add(24 * time.Hour),
+			}
+			node, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey)
+			require.NoError(t, err)
 
-	machineKey := key.NewMachine()
-	nodeKey := key.NewNode()
-	regReq := tailcfg.RegisterRequest{
-		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: pak.Key},
-		NodeKey:  nodeKey.Public(),
-		Hostinfo: &tailcfg.Hostinfo{Hostname: "spent-tagged"},
+			regReq.Hostinfo = &tailcfg.Hostinfo{Hostname: "h1"}
+
+			past := time.Now().Add(-time.Hour)
+			_, _, err = s.SetNodeExpiry(node.ID(), &past)
+			require.NoError(t, err)
+
+			_, _, err = s.HandleNodeFromPreAuthKey(regReq, machineKey)
+			if !reusable {
+				require.ErrorIs(t, err, types.PAKError("authkey already used"))
+
+				current, ok := s.GetNodeByID(node.ID())
+				require.True(t, ok)
+				require.True(t, current.IsExpired(), "a spent key must leave the node expired")
+				require.Equal(t, node.Hostname(), current.Hostname(), "a spent key must leave the node untouched")
+
+				stored, err := s.db.GetNodeByID(node.ID())
+				require.NoError(t, err)
+				require.NotNil(t, stored.Expiry, "a spent key must leave the row expired")
+				require.Equal(t, past.Unix(), stored.Expiry.Unix())
+
+				return
+			}
+
+			require.NoError(t, err)
+
+			current, ok := s.GetNodeByID(node.ID())
+			require.True(t, ok)
+			require.False(t, current.Expiry().Valid(), "a valid key disables tagged node expiry")
+
+			stored, err := s.db.GetNodeByID(node.ID())
+			require.NoError(t, err)
+			require.Nil(t, stored.Expiry)
+		})
 	}
-	node, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public())
-	require.NoError(t, err)
-	require.True(t, node.IsTagged())
-
-	// Stale past (logout) expiry, and the single-use key is now spent.
-	past := time.Now().Add(-1 * time.Hour)
-	_, ok := s.nodeStore.UpdateNode(node.ID(), func(n *types.Node) {
-		n.Expiry = &past
-	})
-	require.True(t, ok)
-
-	// Re-register with the SAME key and the SAME node key (no rotation). The
-	// tagged exclusion from the isExpired gate must let this skip validation, so
-	// the spent single-use key is not rejected.
-	healed, _, err := s.HandleNodeFromPreAuthKey(regReq, machineKey.Public())
-	require.NoError(t, err,
-		"a tagged node with a stale past expiry must skip re-validation of its spent key")
-	require.False(t, healed.IsExpired(), "stale expiry cleared")
-	require.Nil(t, healed.AsStruct().Expiry, "tagged node key-expiry disabled")
-	require.Equal(t, node.ID(), healed.ID())
 }
 
 // TestTaggedPAKReauthRetagsExistingTaggedNode reproduces issue #3370: an

hscontrol/state/state.go

@@ -1809,7 +1809,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 				node.Expiry = regData.Expiry
 			}
 		case !wasTagged && isTagged:
-			// Personal → Tagged: clear expiry (tagged nodes don't expire)
+			// Personal → Tagged: clear expiry; tagged nodes disable it by default.
 			node.Expiry = nil
 		case params.IsConvertFromTag && !isTagged:
 			// Explicit conversion from tagged to user-owned: set expiry from client request
@@ -1826,13 +1826,12 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView
 				node.Expiry = regData.Expiry
 			}
 		case isTagged && node.IsExpired():
-			// Tagged → Tagged, but carrying a stale PAST expiry from an older
-			// headscale's logout stamp (#3371). Tagged nodes never expire, so
-			// clear it; a deliberate future expiry has IsExpired() == false and
-			// falls through to the no-op below.
+			// Tagged → Tagged and expired: a person re-authenticated it, so clear
+			// the past expiry. A future administrative expiry has not taken
+			// effect and falls through to the no-op below.
 			node.Expiry = nil
 		}
-		// Tagged → Tagged with no stale expiry: keep existing expiry - no action.
+		// Tagged → Tagged and not expired: keep existing expiry - no action.
 
 		// Apply default node expiry for non-tagged nodes when the
 		// resolved expiry is still nil or zero (e.g., CLI registration
@@ -2516,13 +2515,10 @@ func (s *State) HandleNodeFromPreAuthKey(
 	// after expiry would skip validation and be re-authorised with a spent or
 	// expired key; the boundary must not depend on the client rotating its key.
 	//
-	// Tagged nodes are excluded: they never expire (KB 1068), so an
-	// IsExpired() tagged node only reflects a stale logout stamp left by an
-	// older headscale (#3371). Forcing it down the re-validation path burns its
-	// fresh key and blocks re-auth forever; treat it as a plain re-registration
-	// and clear the stale expiry in the update below.
+	// Tagged nodes count too: they can carry an administrative expiry or
+	// retain their expiry after an admin tags them. Legacy logout stamps were
+	// cleared by migration; logout no longer expires tagged nodes.
 	isExpired := existsSameUser && existingNodeSameUser.Valid() &&
-		!existingNodeSameUser.IsTagged() &&
 		existingNodeSameUser.IsExpired()
 
 	// A tagged key presented for a currently user-owned node converts that node
@@ -2659,10 +2655,10 @@ func (s *State) HandleNodeFromPreAuthKey(
 				node.User = nil
 
 				// Converting a user-owned node to tagged drops the user's key
-				// expiry (tagged nodes never expire). But retagging an
+				// expiry (tagged nodes disable it by default). But retagging an
 				// already-tagged node must preserve a deliberate FUTURE expiry
 				// set via `headscale nodes expire` - that is a node property, not
-				// tied to the auth key - and only clear a stale PAST expiry. This
+				// tied to the auth key - and only clear a past expiry with a valid key. This
 				// keeps the retag path symmetric with the same-key relogin path
 				// (#3371) rather than silently overriding an admin decision.
 				if wasUserOwned || node.IsExpired() {
@@ -2691,11 +2687,8 @@ func (s *State) HandleNodeFromPreAuthKey(
 					node.Expiry = nil
 				}
 			} else if node.IsExpired() {
-				// #3371: a tagged node must never carry key expiry. Clear a
-				// stale PAST expiry left by a logout (older headscale) so
-				// re-auth is not permanently blocked. A deliberate future
-				// expiry (headscale nodes expire) has IsExpired() == false and
-				// is left untouched.
+				// A valid key re-authenticated this expired tagged node. A future
+				// administrative expiry has not taken effect and stays untouched.
 				node.Expiry = nil
 			}
 		})