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

Fixes ANT-2026-LYVGXVSW

CHANGELOG.md

@@ -154,6 +154,7 @@ the ACME DNS-01 challenge through a built-in DNS provider: `acmedns`,
 - Deleting an ephemeral node no longer logs a spurious `ephemeral node deletion failed` error later [#3575](https://github.com/juanfont/headscale/pull/3575)
 - Add the v2 `acl/validate` endpoint, which Terraform provider 0.29.3 and newer call to check `tailscale_acl` at plan time; before it, those releases failed with `invalid character 'p' after top-level value`
 - A route that is a single address inside the node ranges, in either spelling, is approved and listed but never served; wider routes and `::ffff:` prefixes are still served
+- Require a valid pre-auth key to re-register an expired tagged node
 
 ## 0.29.7 (202x-xx-xx)
 

hscontrol/servertest/reauth_compat_test.go

@@ -56,13 +56,6 @@ var reauthSkipReasons = map[string]string{
 	"reauth-user/a5-expired-valid-key/u-valid/outcome":  expiredKeyNotRotated,
 	"reauth-user/a5-expired-valid-key/u-valid/node-key": expiredKeyAccepted,
 
-	"reauth/t-spent-relogin/t-spent/expiry":         expiredTaggedRevived,
-	"reauth/t-spent-relogin/t-spent/peer-obs":       expiredTaggedRevived,
-	"reauth/t-spent-relogin-force/t-spent/expiry":   expiredTaggedRevived,
-	"reauth/t-spent-relogin-force/t-spent/peer-obs": expiredTaggedRevived,
-	"reauth/t-revoked-relogin/t-revoked/expiry":     expiredTaggedRevived,
-	"reauth/t-revoked-relogin/t-revoked/peer-obs":   expiredTaggedRevived,
-
 	"reauth-owner/u-renew-relogin/u-renew/expiry": sameKeyRenewsExpiry,
 	"reauth-user/a1-spent-key/u-spent/expiry":     sameKeyRenewsExpiry,
 	"reauth-user/a2-valid-key/u-valid/expiry":     sameKeyRenewsExpiry,
@@ -128,7 +121,6 @@ const (
 
 const (
 	expiredKeyNotRotated      = "EXPIRED_KEY_NOT_ROTATED: headscale answers an expired node's key login without asking for a new node key; SaaS answers NodeKeyExpired first, so the client rotates it"
-	expiredTaggedRevived      = "EXPIRED_TAGGED_REVIVED: headscale lets an expired tagged node log in again with its spent or revoked key and lifts its expiry; SaaS refuses the key and the node stays expired"
 	sameKeyRenewsExpiry       = "SAME_KEY_LOGIN_RENEWS_EXPIRY: headscale renews a user node's expiry on a key login that keeps its node key; SaaS renews it only with a new node key"
 	otherUserKeyReauthsTagged = "OTHER_USER_KEY_REAUTHS_TAGGED: headscale re-registers a tagged device in place when another user's key logs in anew on its machine; SaaS keeps the tagged device and adds one for that user"
 	loginTakesTaggedDevice    = "LOGIN_TAKES_TAGGED_DEVICE: headscale's interactive login gives another user the tagged device on the machine; SaaS's key login keeps it and adds a device for that user"

hscontrol/state/auth_tagged_expiry_test.go

@@ -996,49 +996,42 @@ 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)
+			c := registerForPAKRereg(t, s, pak)
+			past := time.Now().Add(-time.Hour)
+			_, _, err = s.SetNodeExpiry(c.node.ID(), &past)
+			require.NoError(t, err)
+			nsBefore, rowBefore := c.fields(t)
+			require.Equal(t, past.UnixNano(), nsBefore.Expiry)
+			require.Equal(t, nsBefore, rowBefore)
 
-	// Single-use tagged key.
-	pak, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:foo"})
-	require.NoError(t, err)
+			node, _, err := s.HandleNodeFromPreAuthKey(c.regReq, c.machineKey)
+			if !reusable {
+				require.ErrorIs(t, err, types.PAKError("authkey already used"))
+				nsAfter, rowAfter := c.fields(t)
+				require.Equal(t, nsBefore, nsAfter, "a spent key must leave the node expired")
+				require.Equal(t, rowBefore, rowAfter, "a spent key must leave the row expired")
 
-	machineKey := key.NewMachine()
-	nodeKey := key.NewNode()
-	regReq := tailcfg.RegisterRequest{
-		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: pak.Key},
-		NodeKey:  nodeKey.Public(),
-		Hostinfo: &tailcfg.Hostinfo{Hostname: "spent-tagged"},
+				return
+			}
+
+			require.NoError(t, err)
+			require.Equal(t, c.node.ID(), node.ID())
+			require.False(t, node.Expiry().Valid(), "a valid key disables tagged node expiry")
+			current, ok := s.GetNodeByID(c.node.ID())
+			require.True(t, ok)
+			require.False(t, current.Expiry().Valid())
+
+			stored, err := s.db.GetNodeByID(c.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
@@ -1683,14 +1676,15 @@ func TestPAKSkipsValidation(t *testing.T) {
 		{"expired user node", userNode(ownerID, &past), userKey, nodeKey, true, false},
 		{"zero expiry", userNode(ownerID, &zero), userKey, nodeKey, true, true},
 		{"nil expiry", userNode(ownerID, nil), userKey, nodeKey, true, true},
-		{"expired tagged node, same key", taggedNode(&past, &keyID), taggedKey, nodeKey, true, true},
+		{"expired tagged node, same key", taggedNode(&past, &keyID), taggedKey, nodeKey, false, false},
+		{"tagged node with future expiry, same key", taggedNode(&future, &keyID), taggedKey, nodeKey, true, true},
 		{"conversion", userNode(ownerID, &future), taggedKey, nodeKey, true, false},
 		{"retag, AuthKeyID nil", taggedNode(nil, nil), taggedKey, nodeKey, true, false},
 		{"retag, other AuthKeyID", taggedNode(nil, &otherKeyID), taggedKey, nodeKey, true, false},
 		{"retag, same AuthKeyID", taggedNode(nil, &keyID), taggedKey, nodeKey, true, true},
 		{"tagged key, rotation", taggedNode(nil, &keyID), taggedKey, otherNodeKey, true, false},
 		{"user key on a tagged node", taggedNode(nil, &otherKeyID), userKey, nodeKey, true, true},
-		{"user key on an expired tagged node", taggedNode(&past, nil), userKey, nodeKey, true, true},
+		{"user key on an expired tagged node", taggedNode(&past, nil), userKey, nodeKey, false, false},
 		{"user key on another user's node", userNode(otherUserID, &future), userKey, nodeKey, false, false},
 		{"user node expiry equals now", userNode(ownerID, &now), userKey, nodeKey, false, true},
 		{"user node expired one nanosecond before now", userNode(ownerID, &oneNanoAgo), userKey, nodeKey, false, false},

hscontrol/state/state.go

@@ -2055,7 +2055,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
@@ -2072,13 +2072,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
@@ -2721,9 +2720,9 @@ func (s *State) findExistingNodeForPAK(
 // --authkey=KEY") re-presents a spent key legitimately. Anything that
 // authorises something new must present a valid key:
 //   - a node key rotation;
-//   - an expired user-owned node, which is re-authenticating rather than
-//     waking up. Tagged nodes never expire, so their past expiry is only a
-//     stale logout stamp and does not count;
+//   - an expired node. Tagged nodes 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;
 //   - a tagged key converting a user-owned node to tagged;
 //   - a tagged key other than the one the node last authed with, which retags
 //     it.
@@ -2736,13 +2735,13 @@ func pakSkipsValidation(n types.NodeView, pak *types.PreAuthKey, nodeKey key.Nod
 	}
 
 	isNodeKeyRotation := n.NodeKey() != nodeKey
-	isExpiredUserOwned := !n.IsTagged() && n.IsExpiredAt(now)
+	isExpired := n.IsExpiredAt(now)
 	isOwnershipConversion := pak.ConvertsNodeToTagged(n)
 	isRetag := pak.RetagsNode(n)
 	ownershipMatches := n.IsTagged() ||
 		(pak.User != nil && n.UserID().Valid() && n.UserID().Get() == pak.User.ID)
 
-	return ownershipMatches && !isNodeKeyRotation && !isExpiredUserOwned &&
+	return ownershipMatches && !isNodeKeyRotation && !isExpired &&
 		!isOwnershipConversion && !isRetag
 }
 
@@ -3038,10 +3037,10 @@ func (s *State) reregisterNodeWithPAK(
 			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
 			// below rather than silently overriding an admin decision.
 			if wasUserOwned || node.IsExpiredAt(now) {
@@ -3077,10 +3076,8 @@ func (s *State) reregisterNodeWithPAK(
 				node.Expiry = nil
 			}
 		} else if node.IsExpiredAt(now) {
-			// A tagged node must never carry key expiry. Clear a stale
-			// PAST logout stamp so re-auth is not permanently blocked. A
-			// deliberate future expiry (headscale nodes expire) is not yet
-			// expired 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
 		}
 	})