diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 67d0b1ecd..81452620a 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/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 diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index cf7d630cb..1817db698 100644 --- a/hscontrol/state/state.go +++ b/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 } })