From e67ef8619ac65ee0ba9c067adae48d14bb08b94a Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 28 Oct 2026 14:22:20 +0000 Subject: [PATCH] state: need a valid pre-auth key to re-register an expired tagged node Fixes ANT-2026-LYVGXVSW --- CHANGELOG.md | 1 + hscontrol/servertest/reauth_compat_test.go | 8 --- hscontrol/state/auth_tagged_expiry_test.go | 78 ++++++++++------------ hscontrol/state/state.go | 31 ++++----- 4 files changed, 51 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 99a9d610e..57404c588 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index 4770b1ff9..4e69668ff 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/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" diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 4fa33cfd3..05a1bb728 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/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}, diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1c7205a0b..dd10f4581 100644 --- a/hscontrol/state/state.go +++ b/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 } })