state: revalidate pre-auth key reuse when registration applies

CHANGELOG.md

@@ -130,6 +130,8 @@ clients, and how to run the same setup without Nix.
 - A registration request from a client below the minimum supported version is now rejected before it can log a node out, use a pre-auth key or start a login [#3519](https://github.com/juanfont/headscale/pull/3519)
 - Fix SSH check accepting a repeated follow-up for an already-decided session, even after a rejection [#3526](https://github.com/juanfont/headscale/pull/3526)
 
+- A node re-registering with a spent, expired or revoked pre-auth key is now rejected if it expired or changed node key while the re-registration was in flight
+
 ## 0.29.5 (202x-xx-xx)
 
 **Minimum supported Tailscale client version: v1.80.0**

hscontrol/state/auth_tagged_expiry_test.go

@@ -2,6 +2,9 @@ package state
 
 import (
 	"fmt"
+	"runtime"
+	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -9,6 +12,7 @@ import (
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/util"
 	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/key"
 )
@@ -1614,3 +1618,585 @@ func TestTaggingPreservesNodeExpiry(t *testing.T) {
 	require.NotNil(t, tagged.AsStruct().Expiry, "tag change must not clear expiry")
 	require.Equal(t, expiry.Unix(), tagged.AsStruct().Expiry.Unix())
 }
+
+// oldPAKSkipExpr is the inline skip expression pakSkipsValidation replaced,
+// with the lookup's existsSameUser taken as true. It is the oracle for nodes
+// findExistingNodeForPAK can return, away from the expiry boundary.
+func oldPAKSkipExpr(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic) bool {
+	isExistingNodeReregistering := n.Valid()
+	isNodeKeyRotation := n.Valid() && n.NodeKey() != nodeKey
+	isExpired := n.Valid() && !n.IsTagged() && n.IsExpired()
+	isOwnershipConversion := n.Valid() && pak.IsTagged() && !n.IsTagged()
+	isRetag := n.Valid() && pak.IsTagged() && n.IsTagged() &&
+		(!n.AuthKeyID().Valid() || n.AuthKeyID().Get() != pak.ID)
+
+	return isExistingNodeReregistering && !isNodeKeyRotation && !isExpired &&
+		!isOwnershipConversion && !isRetag
+}
+
+func TestPAKSkipsValidation(t *testing.T) {
+	now := time.Now()
+	past := now.Add(-time.Hour)
+	future := now.Add(time.Hour)
+	zero := time.Time{}
+	oneNanoAgo := now.Add(-time.Nanosecond)
+
+	nodeKey := key.NewNode().Public()
+	otherNodeKey := key.NewNode().Public()
+	ownerID, otherUserID := uint(1), uint(2)
+	keyID, otherKeyID := uint64(10), uint64(11)
+
+	userKey := &types.PreAuthKey{ID: keyID, User: &types.User{ID: ownerID}}
+	taggedKey := &types.PreAuthKey{ID: keyID, Tags: []string{"tag:a"}}
+
+	userNode := func(uid uint, expiry *time.Time) *types.Node {
+		return &types.Node{
+			ID:      1,
+			NodeKey: nodeKey,
+			UserID:  &uid,
+			User:    &types.User{ID: uid},
+			Expiry:  expiry,
+		}
+	}
+	taggedNode := func(expiry *time.Time, authKeyID *uint64) *types.Node {
+		return &types.Node{
+			ID:        1,
+			NodeKey:   nodeKey,
+			Tags:      []string{"tag:a"},
+			Expiry:    expiry,
+			AuthKeyID: authKeyID,
+		}
+	}
+
+	tests := []struct {
+		name    string
+		node    *types.Node
+		pak     *types.PreAuthKey
+		nodeKey key.NodePublic
+		// lookup marks nodes findExistingNodeForPAK can return for pak, away
+		// from the expiry boundary, where the old expression is the oracle.
+		lookup bool
+		want   bool
+	}{
+		{"plain restart", userNode(ownerID, &future), userKey, nodeKey, true, true},
+		{"rotation", userNode(ownerID, &future), userKey, otherNodeKey, true, false},
+		{"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},
+		{"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 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},
+		{"invalid view", nil, userKey, nodeKey, false, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var view types.NodeView
+			if tt.node != nil {
+				view = tt.node.View()
+			}
+
+			require.Equal(t, tt.want, pakSkipsValidation(view, tt.pak, tt.nodeKey, now))
+
+			if tt.lookup {
+				require.Equal(t, tt.want, oldPAKSkipExpr(view, tt.pak, tt.nodeKey),
+					"must match the expression it replaced")
+			}
+		})
+	}
+}
+
+// pakReregCase is a node registered with a pre-auth key as hostname "h0", and
+// regReq, a re-registration of it with the same node key as hostname "h1".
+type pakReregCase struct {
+	s          *State
+	machineKey key.MachinePublic
+	node       types.NodeView
+	regReq     tailcfg.RegisterRequest
+}
+
+func registerForPAKRereg(t *testing.T, s *State, k *types.PreAuthKeyNew) pakReregCase {
+	t.Helper()
+
+	machineKey := key.NewMachine().Public()
+	regReq := tailcfg.RegisterRequest{
+		Auth:     &tailcfg.RegisterResponseAuth{AuthKey: k.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)
+
+	regReq.Hostinfo = &tailcfg.Hostinfo{Hostname: "h1"}
+
+	return pakReregCase{s: s, machineKey: machineKey, node: node, regReq: regReq}
+}
+
+// lookup runs the lookup half of a re-registration with keyStr, as
+// HandleNodeFromPreAuthKey does, and requires that it skips key validation.
+// The returned view and key row go stale once the caller writes concurrently.
+func (c pakReregCase) lookup(t *testing.T, keyStr string) (types.NodeView, *types.PreAuthKey) {
+	t.Helper()
+
+	pak, err := c.s.GetPreAuthKey(keyStr)
+	require.NoError(t, err)
+
+	view, ok, err := c.s.findExistingNodeForPAK(c.machineKey, pak)
+	require.NoError(t, err)
+	require.True(t, ok)
+	require.True(t, pakSkipsValidation(view, pak, c.regReq.NodeKey, time.Now()),
+		"precondition: the lookup skips key validation")
+
+	return view, pak
+}
+
+// reregister runs the mutation half with the lookup's view and key row.
+func (c pakReregCase) reregister(
+	view types.NodeView,
+	pak *types.PreAuthKey,
+	regReq tailcfg.RegisterRequest,
+) (types.NodeView, error) {
+	hi := regReq.Hostinfo.Clone()
+
+	return c.s.reregisterNodeWithPAK(view, pak, regReq, c.machineKey, hi.Hostname, hi)
+}
+
+// pakNodeFields are the node fields a re-registration writes. Expiry is in
+// Unix nanoseconds (0 for none) so NodeStore and database values compare.
+type pakNodeFields struct {
+	NodeKey   key.NodePublic
+	Hostname  string
+	AuthKeyID *uint64
+	Expiry    int64
+	Tags      []string
+	UserID    *uint
+}
+
+func pakFieldsOf(n *types.Node) pakNodeFields {
+	f := pakNodeFields{
+		NodeKey:   n.NodeKey,
+		Hostname:  n.Hostname,
+		AuthKeyID: n.AuthKeyID,
+		Tags:      n.Tags,
+		UserID:    n.UserID,
+	}
+	if n.Expiry != nil {
+		f.Expiry = n.Expiry.UnixNano()
+	}
+
+	return f
+}
+
+// fields returns the node's fields in the NodeStore and in the database.
+func (c pakReregCase) fields(t *testing.T) (pakNodeFields, pakNodeFields) {
+	t.Helper()
+
+	ns, ok := c.s.GetNodeByID(c.node.ID())
+	require.True(t, ok)
+
+	dbNode, err := c.s.db.GetNodeByID(c.node.ID())
+	require.NoError(t, err)
+
+	return pakFieldsOf(ns.AsStruct()), pakFieldsOf(dbNode)
+}
+
+// waitParkedOnWriteQueue waits until a goroutine running fn is blocked handing
+// a write to the NodeStore writer, which proves that everything fn does before
+// that write has already run.
+func waitParkedOnWriteQueue(t *testing.T, fn string) {
+	t.Helper()
+
+	buf := make([]byte, 1<<22)
+
+	require.Eventually(t, func() bool {
+		stacks := string(buf[:runtime.Stack(buf, true)])
+		for g := range strings.SplitSeq(stacks, "\n\n") {
+			if strings.Contains(g, "[select") &&
+				strings.Contains(g, "(*NodeStore).UpdateNodes(") &&
+				strings.Contains(g, "."+fn+"(") {
+				return true
+			}
+		}
+
+		return false
+	}, 5*time.Second, time.Millisecond)
+}
+
+// TestPAKReregisterRevalidatesAtMutation: a re-registration whose lookup
+// skipped key validation must decide again on the node the NodeStore writer
+// holds, because a concurrent write can land between lookup and mutation.
+func TestPAKReregisterRevalidatesAtMutation(t *testing.T) {
+	t.Run("admin_expire_spent_key", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k)
+		view, pak := c.lookup(t, k.Key)
+		require.True(t, pak.Used, "precondition: the single-use key is spent")
+
+		tA := time.Now()
+		_, _, err = s.SetNodeExpiry(c.node.ID(), &tA)
+		require.NoError(t, err)
+
+		nsBefore, dbBefore := c.fields(t)
+
+		_, err = c.reregister(view, pak, c.regReq)
+		require.ErrorIs(t, err, types.PAKError("authkey already used"))
+
+		nsAfter, dbAfter := c.fields(t)
+		require.Equal(t, nsBefore, nsAfter, "NodeStore node must be untouched")
+		require.Equal(t, dbBefore, dbAfter, "database node must be untouched")
+		require.Equal(t, tA.UnixNano(), nsAfter.Expiry)
+		require.Equal(t, tA.UnixNano(), dbAfter.Expiry)
+		require.Equal(t, "h0", nsAfter.Hostname)
+
+		stored, err := s.GetPreAuthKey(k.Key)
+		require.NoError(t, err)
+		require.True(t, stored.Used, "the key is not consumed again")
+	})
+
+	t.Run("admin_expire_reusable_key", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		k, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k)
+		view, pak := c.lookup(t, k.Key)
+
+		tA := time.Now()
+		_, _, err = s.SetNodeExpiry(c.node.ID(), &tA)
+		require.NoError(t, err)
+
+		got, err := c.reregister(view, pak, c.regReq)
+		require.NoError(t, err, "a valid key re-authorises whatever raced")
+		require.False(t, got.IsExpired())
+
+		ns, dbNode := c.fields(t)
+		require.Equal(t, c.regReq.Expiry.UnixNano(), ns.Expiry, "expiry is extended")
+		require.Equal(t, ns, dbNode, "database must equal the NodeStore")
+	})
+
+	t.Run("nodekey_changed_spent_key", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k)
+		view, pak := c.lookup(t, k.Key)
+
+		// A MapRequest reconcile moves the node to a new node key.
+		reconciled := key.NewNode().Public()
+		_, ok := s.nodeStore.UpdateNode(c.node.ID(), func(n *types.Node) {
+			n.NodeKey = reconciled
+		})
+		require.True(t, ok)
+
+		nsBefore, dbBefore := c.fields(t)
+
+		_, err = c.reregister(view, pak, c.regReq)
+		require.ErrorIs(t, err, types.PAKError("authkey already used"))
+
+		nsAfter, dbAfter := c.fields(t)
+		require.Equal(t, nsBefore, nsAfter, "NodeStore node must be untouched")
+		require.Equal(t, dbBefore, dbAfter, "database node must be untouched")
+		require.Equal(t, reconciled, nsAfter.NodeKey)
+	})
+
+	t.Run("retag_same_tagged_key", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		_, err := s.SetPolicy(fmt.Appendf(nil,
+			`{"tagOwners":{"tag:a":["%s@"],"tag:b":["%s@"]}}`, user.Name, user.Name))
+		require.NoError(t, err)
+
+		k, err := s.CreatePreAuthKey(nil, false, false, nil, []string{"tag:a"})
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k)
+		view, pak := c.lookup(t, k.Key)
+
+		_, _, err = s.SetNodeTags(c.node.ID(), []string{"tag:b"})
+		require.NoError(t, err)
+
+		_, err = c.reregister(view, pak, c.regReq)
+		require.NoError(t, err)
+
+		ns, dbNode := c.fields(t)
+		require.Equal(t, []string{"tag:b"}, ns.Tags, "the admin's tags are kept")
+		require.Equal(t, []string{"tag:b"}, dbNode.Tags)
+	})
+
+	t.Run("owner_to_tagged_user_key", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		_, err := s.SetPolicy(fmt.Appendf(nil, `{"tagOwners":{"tag:a":["%s@"]}}`, user.Name))
+		require.NoError(t, err)
+
+		k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k)
+		view, pak := c.lookup(t, k.Key)
+		require.False(t, view.IsTagged(), "precondition: the lookup saw a user-owned node")
+
+		_, _, err = s.SetNodeTags(c.node.ID(), []string{"tag:a"})
+		require.NoError(t, err)
+
+		got, err := c.reregister(view, pak, c.regReq)
+		require.NoError(t, err, "a user key on a tagged node skips validation")
+		require.True(t, got.IsTagged())
+
+		ns, dbNode := c.fields(t)
+		require.Nil(t, ns.UserID)
+		require.Nil(t, dbNode.UserID)
+		require.Equal(t, []string{"tag:a"}, dbNode.Tags)
+	})
+
+	t.Run("deleted", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		k, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k)
+		view, pak := c.lookup(t, k.Key)
+
+		_, err = s.DeleteNode(view)
+		require.NoError(t, err)
+
+		_, err = c.reregister(view, pak, c.regReq)
+		require.ErrorIs(t, err, ErrNodeNotInNodeStore)
+
+		_, err = s.db.GetNodeByID(c.node.ID())
+		require.ErrorIs(t, err, gorm.ErrRecordNotFound, "no database row may be written")
+	})
+
+	// A stopped NodeStore drops the write, so the writer never decides and
+	// UpdateNode still reports the node from the last snapshot.
+	t.Run("store_stopped", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		k0, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k0)
+
+		k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil)
+		require.NoError(t, err)
+
+		regReq := c.regReq
+		regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key}
+
+		view, pak := c.lookup(t, k.Key)
+		require.False(t, pak.Used, "precondition: the lookup read a fresh single-use key")
+
+		_, dbBefore := c.fields(t)
+
+		s.nodeStore.Stop()
+
+		_, err = c.reregister(view, pak, regReq)
+		require.ErrorIs(t, err, ErrNodeNotInNodeStore)
+
+		_, dbAfter := c.fields(t)
+		require.Equal(t, dbBefore, dbAfter, "database node must be untouched")
+
+		stored, err := s.GetPreAuthKey(k.Key)
+		require.NoError(t, err)
+		require.False(t, stored.Used, "the key must not be consumed")
+	})
+
+	// The node is expired after the lookup, then the key expires, then the
+	// writer runs. The decision must be taken at the writer's own clock: any
+	// validity or clock read taken before the NodeStore write lets the expired
+	// key revive the node.
+	t.Run("admin_expire_then_key_expires", func(t *testing.T) {
+		s := newRetagTestState(t)
+		user := s.CreateUserForTest("pakuser")
+
+		k0, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+		require.NoError(t, err)
+
+		c := registerForPAKRereg(t, s, k0)
+
+		// Wide enough that setup on a slow disk still stalls the writer
+		// well before the key expires.
+		expiration := time.Now().Add(2 * time.Second)
+		k, err := s.CreatePreAuthKey(user.TypedID(), true, false, &expiration, nil)
+		require.NoError(t, err)
+
+		regReq := c.regReq
+		regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key}
+
+		view, pak := c.lookup(t, k.Key)
+		require.NoError(t, pak.ValidAt(time.Now()))
+
+		tA := time.Now()
+		_, _, err = s.SetNodeExpiry(c.node.ID(), &tA)
+		require.NoError(t, err)
+
+		// Stall the writer so the re-registration's write is applied later.
+		entered, release := make(chan struct{}), make(chan struct{})
+
+		releaseOnce := sync.OnceFunc(func() { close(release) })
+		defer releaseOnce()
+
+		stalled := make(chan struct{})
+
+		go func() {
+			defer close(stalled)
+
+			s.nodeStore.UpdateNode(c.node.ID(), func(*types.Node) {
+				close(entered)
+				<-release
+			})
+		}()
+
+		<-entered
+		require.Greater(t, time.Until(expiration), time.Second,
+			"setup must stall the writer well before the key expires")
+
+		type result struct {
+			node types.NodeView
+			err  error
+		}
+
+		done := make(chan result, 1)
+
+		go func() {
+			n, err := c.reregister(view, pak, regReq)
+			done <- result{n, err}
+		}()
+
+		waitParkedOnWriteQueue(t, "reregisterNodeWithPAK")
+		require.True(t, time.Now().Before(expiration),
+			"the re-registration must reach its NodeStore write before the key expires")
+
+		// Only now let the key expire, then let the writer run.
+		require.Eventually(t, func() bool {
+			return pak.ValidAt(time.Now()) != nil
+		}, time.Until(expiration)+time.Second, time.Millisecond)
+		releaseOnce()
+		<-stalled
+
+		res := <-done
+		require.ErrorIs(t, res.err, types.PAKError("authkey expired"))
+
+		ns, dbNode := c.fields(t)
+		for _, f := range []pakNodeFields{ns, dbNode} {
+			require.Equal(t, tA.UnixNano(), f.Expiry)
+			require.Equal(t, c.regReq.NodeKey, f.NodeKey)
+			require.Equal(t, &k0.ID, f.AuthKeyID)
+			require.Equal(t, "h0", f.Hostname)
+		}
+	})
+}
+
+// TestPAKReregisterRollbackKeepsPriorWrite: when the database rejects a
+// re-registration, the NodeStore rollback must restore the node the writer
+// replaced, not the lookup's older view, so an admin expiry landed in between
+// survives.
+func TestPAKReregisterRollbackKeepsPriorWrite(t *testing.T) {
+	s := newRetagTestState(t)
+	user := s.CreateUserForTest("pakuser")
+
+	k0, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil)
+	require.NoError(t, err)
+
+	c := registerForPAKRereg(t, s, k0)
+
+	k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil)
+	require.NoError(t, err)
+
+	regReq := c.regReq
+	regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key}
+
+	view, pak := c.lookup(t, k.Key)
+	require.False(t, pak.Used, "precondition: the lookup read a fresh single-use key")
+
+	tA := time.Now()
+	_, _, err = s.SetNodeExpiry(c.node.ID(), &tA)
+	require.NoError(t, err)
+
+	// Another registration consumes the key after the lookup read its row, so
+	// the transaction's compare-and-set fails.
+	err = s.db.Write(func(tx *gorm.DB) error {
+		return db.UsePreAuthKey(tx, &types.PreAuthKey{ID: pak.ID})
+	})
+	require.NoError(t, err)
+
+	_, err = c.reregister(view, pak, regReq)
+	require.ErrorIs(t, err, types.PAKError("authkey already used"))
+
+	ns, _ := c.fields(t)
+	require.Equal(t, tA.UnixNano(), ns.Expiry, "rollback must keep the admin expiry")
+	require.Equal(t, &k0.ID, ns.AuthKeyID)
+	require.Equal(t, "h0", ns.Hostname)
+}
+
+// TestPAKReregisterConcurrentAdminExpire races a same-key restart with a spent
+// key against an admin expiry. Either serial order leaves the node expired.
+// Only the NodeStore is checked: the order of the two database writes is not
+// serialised with it.
+func TestPAKReregisterConcurrentAdminExpire(t *testing.T) {
+	s := newRetagTestState(t)
+	user := s.CreateUserForTest("pakuser")
+
+	k, err := s.CreatePreAuthKey(user.TypedID(), false, false, nil, nil)
+	require.NoError(t, err)
+
+	c := registerForPAKRereg(t, s, k)
+	future := time.Now().Add(time.Hour)
+
+	for i := range 100 {
+		_, _, err := s.SetNodeExpiry(c.node.ID(), &future)
+		require.NoError(t, err)
+
+		var (
+			wg        sync.WaitGroup
+			expireErr error
+		)
+
+		start := make(chan struct{})
+
+		wg.Go(func() {
+			<-start
+
+			_, _, _ = s.HandleNodeFromPreAuthKey(c.regReq, c.machineKey)
+		})
+		wg.Go(func() {
+			<-start
+
+			now := time.Now()
+			_, _, expireErr = s.SetNodeExpiry(c.node.ID(), &now)
+		})
+		close(start)
+		wg.Wait()
+
+		require.NoError(t, expireErr)
+
+		n, ok := s.GetNodeByID(c.node.ID())
+		require.True(t, ok)
+		require.True(t, n.IsExpired(), "iteration %d: the spent key revived an expired node", i)
+	}
+}

hscontrol/state/state.go

@@ -2612,6 +2612,47 @@ func (s *State) findExistingNodeForPAK(
 	return types.NodeView{}, false, nil
 }
 
+// pakSkipsValidation reports whether re-registering n with pak may skip key
+// validation at now. The machine key proves identity and a pre-auth key only
+// authorises the initial join, so a plain restart (container "tailscale up
+// --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;
+//   - 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.
+//
+// It is a pure function of the node so the NodeStore writer can decide again
+// on the node it actually replaces.
+func pakSkipsValidation(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) bool {
+	if !n.Valid() {
+		return false
+	}
+
+	isNodeKeyRotation := n.NodeKey() != nodeKey
+	isExpiredUserOwned := !n.IsTagged() && 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 &&
+		!isOwnershipConversion && !isRetag
+}
+
+// pakReregisterErr is the authorisation decision for re-registering n with
+// pak at now: nil when validation may be skipped, else the key's validity.
+func pakReregisterErr(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) error {
+	if pakSkipsValidation(n, pak, nodeKey, now) {
+		return nil
+	}
+
+	return pak.ValidAt(now)
+}
+
 //nolint:gocyclo // sequential validation/update/create paths with security-sensitive ordering
 func (s *State) HandleNodeFromPreAuthKey(
 	regReq tailcfg.RegisterRequest,
@@ -2648,72 +2689,15 @@ func (s *State) HandleNodeFromPreAuthKey(
 		}
 	}
 
-	// Helper to get username for logging (handles nil User for tags-only keys)
-	pakUsername := func() string {
-		if pak.User != nil {
-			return pak.User.Username()
-		}
-
-		return types.TaggedDevices.Name
-	}
-
 	existingNodeSameUser, existsSameUser, err := s.findExistingNodeForPAK(machineKey, pak)
 	if err != nil {
 		return types.NodeView{}, s.policyChangeSince(genBefore), err
 	}
 
-	// For existing nodes, skip validation if:
-	// 1. MachineKey matches (cryptographic proof of machine identity)
-	// 2. User/tag ownership matches (from the PAK being used)
-	// 3. Not a NodeKey rotation (rotation requires fresh validation)
-	//
-	// Security: MachineKey is the cryptographic identity. If someone has the MachineKey,
-	// they control the machine. The PAK was only needed to authorize initial join.
-	// We don't check which specific PAK was used originally because:
-	// - Container restarts may use different PAKs (e.g., env var changed)
-	// - Original PAK may be deleted
-	// - MachineKey + ownership is sufficient to prove this is the same node
-	isExistingNodeReregistering := existsSameUser && existingNodeSameUser.Valid()
-
-	// Check if this is a NodeKey rotation (different NodeKey)
-	isNodeKeyRotation := existsSameUser && existingNodeSameUser.Valid() &&
-		existingNodeSameUser.NodeKey() != regReq.NodeKey
-
-	// An expired node is genuinely re-authenticating, not just waking up, so it
-	// must present a valid key. Without this a node that re-uses its NodeKey
-	// 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.
-	isExpired := existsSameUser && existingNodeSameUser.Valid() &&
-		!existingNodeSameUser.IsTagged() &&
-		existingNodeSameUser.IsExpired()
-
-	// A tagged key presented for a currently user-owned node converts that node
-	// to tagged. That is an ownership change, not a plain refresh, so it must
-	// present a valid key rather than ride the skip-validation fast-path.
-	isOwnershipConversion := existsSameUser && existingNodeSameUser.Valid() &&
-		pak.IsTagged() && !existingNodeSameUser.IsTagged()
-
-	// A tagged key that differs from the one the node last authed with retags
-	// the node (see the in-place update below). Applying a key's tags is an
-	// authorisation decision, so the key must be validated rather than ride the
-	// skip-validation fast-path; otherwise a spent, revoked or expired tagged
-	// key could still retag a node that reuses its node key. Like isExpired,
-	// this boundary must not depend on the client rotating its key.
-	isRetag := existsSameUser && existingNodeSameUser.Valid() &&
-		pak.IsTagged() && existingNodeSameUser.IsTagged() &&
-		(!existingNodeSameUser.AuthKeyID().Valid() || existingNodeSameUser.AuthKeyID().Get() != pak.ID)
-
-	if isExistingNodeReregistering && !isNodeKeyRotation && !isExpired && !isOwnershipConversion && !isRetag {
-		// Existing, still-valid node re-registering with same NodeKey: skip
-		// validation. Pre-auth keys are only needed for initial authentication.
-		// Critical for containers that run "tailscale up --authkey=KEY" on every
-		// restart.
+	// This decision is only an early reject that keeps a key error ahead of
+	// ErrNodeKeyInUse. An in-place re-registration decides again in the
+	// NodeStore writer, on the node it replaces; see reregisterNodeWithPAK.
+	if existsSameUser && pakSkipsValidation(existingNodeSameUser, pak, regReq.NodeKey, time.Now()) {
 		log.Debug().
 			Caller().
 			Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()).
@@ -2725,10 +2709,8 @@ func (s *State) HandleNodeFromPreAuthKey(
 			Bool(zf.AuthKeyUsed, pak.Used).
 			Bool(zf.AuthKeyExpired, pak.Expiration != nil && pak.Expiration.Before(time.Now())).
 			Bool(zf.AuthKeyReusable, pak.Reusable).
-			Bool(zf.NodeKeyRotation, isNodeKeyRotation).
-			Msg("Existing node re-registering with same NodeKey and auth key, skipping validation")
+			Msg("Existing node re-registering with same NodeKey; lookup permits skipping key validation, writer re-decides")
 	} else {
-		// New node or NodeKey rotation: require valid auth key.
 		err = pak.Validate()
 		if err != nil {
 			return types.NodeView{}, s.policyChangeSince(genBefore), err
@@ -2752,7 +2734,7 @@ func (s *State) HandleNodeFromPreAuthKey(
 		Str(zf.NodeName, hostname).
 		Str(zf.MachineKey, machineKey.ShortString()).
 		Str(zf.NodeKey, regReq.NodeKey.ShortString()).
-		Str(zf.UserName, pakUsername()).
+		Str(zf.UserName, pak.Username()).
 		Msg("Registering node with pre-auth key")
 
 	var finalNode types.NodeView
@@ -2762,184 +2744,10 @@ func (s *State) HandleNodeFromPreAuthKey(
 	// 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() {
-		log.Trace().
-			Caller().
-			Str(zf.NodeName, existingNodeSameUser.Hostname()).
-			Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()).
-			Str(zf.MachineKey, machineKey.ShortString()).
-			Str(zf.NodeKey, existingNodeSameUser.NodeKey().ShortString()).
-			Str(zf.UserName, pakUsername()).
-			Msg("Node re-registering with existing machine key and user, updating in place")
-
-		// Re-registration rotates the NodeKey to the client-supplied value.
-		// Enforce the same 1:1 NodeKey<->MachineKey binding the auth path
-		// (applyAuthNodeUpdate) and poll-time validation enforce: a NodeKey
-		// already bound to a different machine must not be claimed here, or a
-		// re-registering node could rotate its key to a victim's and poison the
-		// NodeStore NodeKey index, denying the victim service.
-		if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok &&
-			existing.MachineKey() != machineKey {
-			return types.NodeView{}, s.policyChangeSince(genBefore), ErrNodeKeyInUse
-		}
-
-		// Snapshot the pre-update node so the NodeStore can be rolled back if
-		// the database write below fails. The view points at the immutable
-		// pre-update snapshot (UpdateNode swaps in a new one), so this stays
-		// valid after the mutation.
-		priorNode := existingNodeSameUser.AsStruct()
-
-		// Update existing node - NodeStore first, then database
-		updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) {
-			node.NodeKey = regReq.NodeKey
-			node.Hostname = hostname
-
-			// TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics
-			// when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest).
-
-			// Preserve NetInfo from existing node when re-registering
-			node.Hostinfo = validHostinfo
-			node.Hostinfo.NetInfo = preserveNetInfo(existingNodeSameUser, existingNodeSameUser.ID(), validHostinfo)
-
-			node.RegisterMethod = util.RegisterMethodAuthKey
-
-			// Tags from a PreAuthKey are applied on initial registration and
-			// re-applied whenever a *different* key is presented on
-			// re-registration: re-keying is Tailscale's documented way to change
-			// an auth-key device's tags (KB 1068 - "generate a new auth key with
-			// the new set of tags ... doing so replaces the device's existing
-			// tags"). Presenting the SAME key again (container restart,
-			// #2830/#3312) preserves the node's current tags and any admin
-			// override. A tagged key presented for a user-owned node also converts
-			// it, dropping user ownership.
-			//
-			// node.AuthKeyID still holds the prior key's ID here (it is reassigned
-			// below), and SetNodeTags leaves AuthKeyID intact, so an admin retag
-			// cannot masquerade as a new key.
-			keyChanged := node.AuthKeyID == nil || *node.AuthKeyID != pak.ID
-			if pak.IsTagged() && (!node.IsTagged() || keyChanged) {
-				wasUserOwned := !node.IsTagged()
-
-				node.Tags = pak.Tags
-				node.UserID = nil
-				node.User = nil
-
-				// Converting a user-owned node to tagged drops the user's key
-				// expiry (tagged nodes never expire). 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
-				// keeps the retag path symmetric with the same-key relogin path
-				// (#3371) rather than silently overriding an admin decision.
-				if wasUserOwned || node.IsExpired() {
-					node.Expiry = nil
-				}
-			}
-
-			node.AuthKey = pak.AsCredential()
-			node.AuthKeyID = &pak.ID
-			// If this registration will consume a single-use key (the tx below
-			// calls UsePreAuthKey under the same condition), reflect that in the
-			// cached AuthKey so the NodeStore copy matches the database.
-			if !pak.Reusable && !pak.Used {
-				node.AuthKey.Used = true
-			}
-			// Preserve online state during re-registration so a live node does
-			// not appear offline before the client restarts its map stream.
-			node.LastSeen = new(time.Now())
-
-			// Tagged nodes keep their existing expiry (disabled).
-			// User-owned nodes update expiry from the client request,
-			// falling back to the configured default if the client
-			// did not request a specific expiry. If neither is set,
-			// clear the expiry so the database holds NULL instead of
-			// a pointer to zero time.
-			if !node.IsTagged() {
-				if !regReq.Expiry.IsZero() {
-					node.Expiry = &regReq.Expiry
-				} else if s.cfg.Node.Expiry > 0 {
-					exp := time.Now().Add(s.cfg.Node.Expiry)
-					node.Expiry = &exp
-				} else {
-					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.
-				node.Expiry = nil
-			}
-		})
-
-		if !ok {
-			return types.NodeView{}, s.policyChangeSince(genBefore), fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID())
-		}
-
-		_, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) {
-			// Explicitly select all node columns so GORM includes nil/zero-value fields
-			// (see nodeUpdateColumns comment). AuthKeyID is normally excluded to
-			// avoid persisting a deleted key's stale reference on MapRequest
-			// (#2862), but re-registration presents a freshly-validated key, so
-			// its ID must be persisted here — otherwise a restart reloads the old
-			// key and any key-scoped property (e.g. Ephemeral) silently reverts.
-			reregColumns := append(slices.Clone(nodeUpdateColumns), "AuthKeyID")
-
-			err := tx.Select(reregColumns).Updates(updatedNodeView.AsStruct()).Error
-			if err != nil {
-				return nil, fmt.Errorf("saving node: %w", err)
-			}
-
-			// Only mark the key used on the *first* registration. On
-			// re-registration the same key is already used and the
-			// atomic compare-and-set in [hsdb.UsePreAuthKey] would otherwise
-			// reject it as "authkey already used". This is the path
-			// behind issue #2830 where containers restart with the
-			// same one-shot key.
-			if !pak.Reusable && !pak.Used {
-				err = hsdb.UsePreAuthKey(tx, pak)
-				if err != nil {
-					return nil, fmt.Errorf("using pre auth key: %w", err)
-				}
-			}
-
-			return nil, nil //nolint:nilnil // intentional: transaction success
-		})
+		finalNode, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo)
 		if err != nil {
-			// The NodeStore was updated before the database write. Roll it back
-			// so it does not advertise a registration the database rejected
-			// (e.g. a node key that a restart would not reload). Restore only
-			// the fields the update above wrote: sessions, endpoints and health
-			// may have moved since priorNode was taken. LastSeen stays, as the
-			// node did contact us.
-			if priorNode != nil {
-				s.nodeStore.UpdateNode(priorNode.ID, func(n *types.Node) {
-					n.NodeKey = priorNode.NodeKey
-					n.Hostname = priorNode.Hostname
-					n.Hostinfo = priorNode.Hostinfo
-					n.RegisterMethod = priorNode.RegisterMethod
-					n.Tags = priorNode.Tags
-					n.UserID = priorNode.UserID
-					n.User = priorNode.User
-					n.Expiry = priorNode.Expiry
-					n.AuthKey = priorNode.AuthKey
-					n.AuthKeyID = priorNode.AuthKeyID
-				})
-			}
-
-			return types.NodeView{}, s.policyChangeSince(genBefore), fmt.Errorf("writing node to database: %w", err)
+			return types.NodeView{}, s.policyChangeSince(genBefore), err
 		}
-
-		log.Trace().
-			Caller().
-			Str(zf.NodeName, updatedNodeView.Hostname()).
-			Uint64(zf.NodeID, updatedNodeView.ID().Uint64()).
-			Str(zf.MachineKey, machineKey.ShortString()).
-			Str(zf.NodeKey, updatedNodeView.NodeKey().ShortString()).
-			Str(zf.UserName, pakUsername()).
-			Msg("Node re-authorized")
-
-		finalNode = updatedNodeView
 	} 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
@@ -2976,7 +2784,7 @@ func (s *State) HandleNodeFromPreAuthKey(
 				Uint64(zf.ExistingNodeID, differentUserNode.ID().Uint64()).
 				Str(zf.MachineKey, machineKey.ShortString()).
 				Str(zf.OldUser, oldUserName).
-				Str(zf.NewUser, pakUsername()).
+				Str(zf.NewUser, pak.Username()).
 				Msg("Creating new node for different user (same machine key exists for another user)")
 		}
 
@@ -3036,6 +2844,219 @@ func (s *State) HandleNodeFromPreAuthKey(
 	return finalNode, reauthChange(finalNode, existsSameUser, policyChanged), nil
 }
 
+// reregisterNodeWithPAK updates an existing node in place for a pre-auth key
+// re-registration: NodeStore first, then the database.
+//
+// The caller's lookup view goes stale before the NodeStore writer runs: an
+// admin or logout expiry, a node key reconcile, a retag or a deletion can land
+// 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.
+func (s *State) reregisterNodeWithPAK(
+	existingNodeSameUser types.NodeView,
+	pak *types.PreAuthKey,
+	regReq tailcfg.RegisterRequest,
+	machineKey key.MachinePublic,
+	hostname string,
+	validHostinfo *tailcfg.Hostinfo,
+) (types.NodeView, error) {
+	log.Trace().
+		Caller().
+		Str(zf.NodeName, existingNodeSameUser.Hostname()).
+		Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()).
+		Str(zf.MachineKey, machineKey.ShortString()).
+		Str(zf.NodeKey, existingNodeSameUser.NodeKey().ShortString()).
+		Str(zf.UserName, pak.Username()).
+		Msg("Node re-registering with existing machine key and user, updating in place")
+
+	// Re-registration rotates the NodeKey to the client-supplied value.
+	// Enforce the same 1:1 NodeKey<->MachineKey binding the auth path
+	// (applyAuthNodeUpdate) and poll-time validation enforce: a NodeKey
+	// already bound to a different machine must not be claimed here, or a
+	// re-registering node could rotate its key to a victim's and poison the
+	// NodeStore NodeKey index, denying the victim service.
+	if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok &&
+		existing.MachineKey() != machineKey {
+		return types.NodeView{}, ErrNodeKeyInUse
+	}
+
+	// prior is the node the writer replaced: the NetInfo source and the
+	// rollback target if the database write below fails.
+	var (
+		prior   *types.Node
+		authErr error
+	)
+
+	// Update existing node - NodeStore first, then database
+	updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) {
+		// One clock read, so the whole mutation is decided at one instant.
+		now := time.Now()
+
+		prior = node.Clone()
+
+		authErr = pakReregisterErr(node.View(), pak, regReq.NodeKey, now)
+		if authErr != nil {
+			return
+		}
+
+		node.NodeKey = regReq.NodeKey
+		node.Hostname = hostname
+
+		// TODO(kradalby): We should ensure we use the same hostinfo and node merge semantics
+		// when a node re-registers as we do when it sends a map request (UpdateNodeFromMapRequest).
+
+		// Preserve NetInfo from existing node when re-registering
+		node.Hostinfo = validHostinfo
+		node.Hostinfo.NetInfo = preserveNetInfo(prior.View(), prior.ID, validHostinfo)
+
+		node.RegisterMethod = util.RegisterMethodAuthKey
+
+		// Tags from a PreAuthKey are applied on initial registration and
+		// re-applied whenever a *different* key is presented on
+		// re-registration: re-keying is Tailscale's documented way to change
+		// an auth-key device's tags, replacing its existing ones. Presenting
+		// the SAME key again (a container restart) preserves the node's
+		// current tags and any admin override. A tagged key presented for a
+		// user-owned node also converts it, dropping user ownership.
+		// See https://tailscale.com/kb/1068/tags#apply-a-tag-to-a-device-with-the-cli.
+		//
+		// node.AuthKeyID still holds the prior key's ID here (it is reassigned
+		// below), and SetNodeTags leaves AuthKeyID intact, so an admin retag
+		// cannot masquerade as a new key.
+		if pak.ConvertsNodeToTagged(node.View()) || pak.RetagsNode(node.View()) {
+			wasUserOwned := !node.IsTagged()
+
+			node.Tags = pak.Tags
+			node.UserID = nil
+			node.User = nil
+
+			// Converting a user-owned node to tagged drops the user's key
+			// expiry (tagged nodes never expire). 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
+			// keeps the retag path symmetric with the same-key relogin path
+			// below rather than silently overriding an admin decision.
+			if wasUserOwned || node.IsExpiredAt(now) {
+				node.Expiry = nil
+			}
+		}
+
+		node.AuthKey = pak.AsCredential()
+		node.AuthKeyID = &pak.ID
+		// If this registration will consume a single-use key (the tx below
+		// calls UsePreAuthKey under the same condition), reflect that in the
+		// cached AuthKey so the NodeStore copy matches the database.
+		if !pak.Reusable && !pak.Used {
+			node.AuthKey.Used = true
+		}
+		// Preserve online state during re-registration so a live node does
+		// not appear offline before the client restarts its map stream.
+		node.LastSeen = new(now)
+
+		// Tagged nodes keep their existing expiry (disabled).
+		// User-owned nodes update expiry from the client request,
+		// falling back to the configured default if the client
+		// did not request a specific expiry. If neither is set,
+		// clear the expiry so the database holds NULL instead of
+		// a pointer to zero time.
+		if !node.IsTagged() {
+			if !regReq.Expiry.IsZero() {
+				node.Expiry = &regReq.Expiry
+			} else if s.cfg.Node.Expiry > 0 {
+				exp := now.Add(s.cfg.Node.Expiry)
+				node.Expiry = &exp
+			} else {
+				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.
+			node.Expiry = nil
+		}
+	})
+
+	// 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{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID())
+	}
+
+	if authErr != nil {
+		log.Debug().
+			Caller().
+			Uint64(zf.NodeID, existingNodeSameUser.ID().Uint64()).
+			Str(zf.MachineKey, machineKey.ShortString()).
+			Uint64(zf.AuthKeyID, pak.ID).
+			Err(authErr).
+			Msg("Re-registration needs a valid auth key when it applies, rejecting")
+
+		return types.NodeView{}, authErr
+	}
+
+	_, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) {
+		// Explicitly select all node columns so GORM includes nil/zero-value fields
+		// (see nodeUpdateColumns comment). AuthKeyID is excluded on MapRequest
+		// so a deleted key's stale reference is not persisted, but
+		// re-registration presents a key just loaded from the database, so
+		// its ID must be persisted here — otherwise a restart reloads the old
+		// key and any key-scoped property (e.g. Ephemeral) silently reverts.
+		reregColumns := append(slices.Clone(nodeUpdateColumns), "AuthKeyID")
+
+		err := tx.Select(reregColumns).Updates(updatedNodeView.AsStruct()).Error
+		if err != nil {
+			return nil, fmt.Errorf("saving node: %w", err)
+		}
+
+		// Only mark the key used on the *first* registration. On
+		// re-registration the same key is already used and the
+		// atomic compare-and-set in [hsdb.UsePreAuthKey] would otherwise
+		// reject it as "authkey already used", locking out containers
+		// that restart with the same one-shot key.
+		if !pak.Reusable && !pak.Used {
+			err = hsdb.UsePreAuthKey(tx, pak)
+			if err != nil {
+				return nil, fmt.Errorf("using pre auth key: %w", err)
+			}
+		}
+
+		return nil, nil //nolint:nilnil // intentional: transaction success
+	})
+	if err != nil {
+		// Restore the registration fields from the node the writer replaced.
+		// Sessions, endpoints and health may have moved since that snapshot;
+		// preserve them. LastSeen stays because the node did contact us.
+		s.nodeStore.UpdateNode(prior.ID, func(n *types.Node) {
+			n.NodeKey = prior.NodeKey
+			n.Hostname = prior.Hostname
+			n.Hostinfo = prior.Hostinfo
+			n.RegisterMethod = prior.RegisterMethod
+			n.Tags = prior.Tags
+			n.UserID = prior.UserID
+			n.User = prior.User
+			n.Expiry = prior.Expiry
+			n.AuthKey = prior.AuthKey
+			n.AuthKeyID = prior.AuthKeyID
+		})
+
+		return types.NodeView{}, fmt.Errorf("writing node to database: %w", err)
+	}
+
+	log.Trace().
+		Caller().
+		Str(zf.NodeName, updatedNodeView.Hostname()).
+		Uint64(zf.NodeID, updatedNodeView.ID().Uint64()).
+		Str(zf.MachineKey, machineKey.ShortString()).
+		Str(zf.NodeKey, updatedNodeView.NodeKey().ShortString()).
+		Str(zf.UserName, pak.Username()).
+		Msg("Node re-authorized")
+
+	return updatedNodeView, nil
+}
+
 // reauthChange returns the [change.Change] to broadcast after an authentication
 // that updated or created a node.
 //

hscontrol/types/node.go

@@ -219,6 +219,11 @@ func (ns Nodes) ViewSlice() views.Slice[NodeView] {
 
 // IsExpired returns whether the node registration has expired.
 func (node *Node) IsExpired() bool {
+	return node.IsExpiredAt(time.Now())
+}
+
+// IsExpiredAt reports whether the node registration has expired at now.
+func (node *Node) IsExpiredAt(now time.Time) bool {
 	// If Expiry is not set, the client has not indicated that
 	// it wants an expiry time, it is therefore considered
 	// to mean "not expired"
@@ -226,7 +231,7 @@ func (node *Node) IsExpired() bool {
 		return false
 	}
 
-	return time.Since(*node.Expiry) > 0
+	return now.After(*node.Expiry)
 }
 
 // Online reports the node's last known connectivity. Unknown counts as
@@ -933,6 +938,15 @@ func (nv NodeView) IsExpired() bool {
 	return nv.ж.IsExpired()
 }
 
+// IsExpiredAt reports whether the node registration has expired at now.
+func (nv NodeView) IsExpiredAt(now time.Time) bool {
+	if !nv.Valid() {
+		return true
+	}
+
+	return nv.ж.IsExpiredAt(now)
+}
+
 // Online reports the node's last known connectivity.
 func (nv NodeView) Online() bool {
 	if !nv.Valid() {

hscontrol/types/node_test.go

@@ -1043,3 +1043,33 @@ func TestHasPolicyChangeFields(t *testing.T) {
 		})
 	}
 }
+
+func TestNodeIsExpiredAt(t *testing.T) {
+	now := time.Now()
+	zero := time.Time{}
+	justBefore := now.Add(-time.Nanosecond)
+
+	tests := []struct {
+		name   string
+		expiry *time.Time
+		want   bool
+	}{
+		{name: "nil expiry", expiry: nil, want: false},
+		{name: "zero expiry", expiry: &zero, want: false},
+		{name: "expiry equals now", expiry: &now, want: false},
+		{name: "now one nanosecond after expiry", expiry: &justBefore, want: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			n := &Node{Expiry: tt.expiry}
+			require.Equal(t, tt.want, n.IsExpiredAt(now))
+			require.Equal(t, tt.want, n.View().IsExpiredAt(now))
+		})
+	}
+
+	t.Run("invalid view", func(t *testing.T) {
+		require.True(t, NodeView{}.IsExpiredAt(now),
+			"an invalid view counts as expired, like IsExpired")
+	})
+}

hscontrol/types/preauth_key.go

@@ -98,11 +98,21 @@ func (pak *PreAuthKey) Validate() error {
 		EmbedObject(pak).
 		Msg("PreAuthKey.Validate: checking key")
 
+	return pak.ValidAt(time.Now())
+}
+
+// ValidAt is [PreAuthKey.Validate] at a caller-chosen instant, without
+// logging, so a NodeStore writer can decide at its own clock.
+func (pak *PreAuthKey) ValidAt(now time.Time) error {
+	if pak == nil {
+		return PAKError("invalid authkey")
+	}
+
 	if pak.Revoked != nil {
 		return PAKError("authkey revoked")
 	}
 
-	if pak.Expiration != nil && pak.Expiration.Before(time.Now()) {
+	if pak.Expiration != nil && pak.Expiration.Before(now) {
 		return PAKError("authkey expired")
 	}
 
@@ -124,6 +134,29 @@ func (pak *PreAuthKey) IsTagged() bool {
 	return len(pak.Tags) > 0
 }
 
+// Username returns the associated user's name, or TaggedDevices for a key
+// without a user. For tagged keys the associated user is the creator.
+func (pak *PreAuthKey) Username() string {
+	if pak.User != nil {
+		return pak.User.Username()
+	}
+
+	return TaggedDevices.Name
+}
+
+// ConvertsNodeToTagged reports whether this key changes a user-owned node
+// to tag ownership.
+func (pak *PreAuthKey) ConvertsNodeToTagged(node NodeView) bool {
+	return node.Valid() && pak.IsTagged() && !node.IsTagged()
+}
+
+// RetagsNode reports whether this key replaces an already-tagged node's
+// tags. Reusing the last key preserves subsequent admin tag changes.
+func (pak *PreAuthKey) RetagsNode(node NodeView) bool {
+	return node.IsTagged() && pak.IsTagged() &&
+		(!node.AuthKeyID().Valid() || node.AuthKeyID().Get() != pak.ID)
+}
+
 // maskedPrefix returns the key prefix in masked format for safe logging.
 // SECURITY: Never log the full key or hash, only the masked prefix.
 func (pak *PreAuthKey) maskedPrefix() string {

hscontrol/types/preauth_key_test.go

@@ -6,6 +6,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCanUsePreAuthKey(t *testing.T) {
@@ -14,11 +15,32 @@ func TestCanUsePreAuthKey(t *testing.T) {
 	future := now.Add(time.Hour)
 
 	tests := []struct {
-		name    string
-		pak     *PreAuthKey
+		name string
+		pak  *PreAuthKey
+		// at, when set, checks ValidAt(at) instead of Validate.
+		at      time.Time
 		wantErr bool
 		err     PAKError
 	}{
+		{
+			name: "valid at the instant of expiration",
+			pak: &PreAuthKey{
+				Reusable:   true,
+				Expiration: &now,
+			},
+			at:      now,
+			wantErr: false,
+		},
+		{
+			name: "expired one nanosecond after expiration",
+			pak: &PreAuthKey{
+				Reusable:   true,
+				Expiration: &now,
+			},
+			at:      now.Add(time.Nanosecond),
+			wantErr: true,
+			err:     PAKError("authkey expired"),
+		},
 		{
 			name: "valid reusable key",
 			pak: &PreAuthKey{
@@ -105,7 +127,16 @@ func TestCanUsePreAuthKey(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := tt.pak.Validate()
+			var err error
+			if tt.at.IsZero() {
+				err = tt.pak.Validate()
+				if diff := cmp.Diff(err, tt.pak.ValidAt(time.Now())); diff != "" {
+					t.Errorf("Validate and ValidAt(now) disagree (-Validate +ValidAt):\n%s", diff)
+				}
+			} else {
+				err = tt.pak.ValidAt(tt.at)
+			}
+
 			if tt.wantErr {
 				if err == nil {
 					t.Errorf("expected error but got none")
@@ -127,3 +158,44 @@ func TestCanUsePreAuthKey(t *testing.T) {
 		})
 	}
 }
+
+func TestPreAuthKeyUsername(t *testing.T) {
+	user := &User{Name: "creator", Email: "creator@example.com"}
+	for _, pak := range []*PreAuthKey{
+		{User: user},
+		{User: user, Tags: []string{"tag:server"}},
+	} {
+		require.Equal(t, user.Username(), pak.Username())
+	}
+
+	require.Equal(t, TaggedDevices.Name, (&PreAuthKey{Tags: []string{"tag:server"}}).Username())
+}
+
+func TestPreAuthKeyTagChanges(t *testing.T) {
+	keyID := uint64(1)
+	userID := uint(1)
+	pak := &PreAuthKey{ID: keyID, Tags: []string{"tag:original"}, User: &User{ID: userID}}
+
+	userNode := (&Node{ID: 1, UserID: &userID}).View()
+	require.True(t, pak.ConvertsNodeToTagged(userNode))
+	require.False(t, pak.RetagsNode(userNode))
+
+	// An admin changed the tags, but the key identity is unchanged. The
+	// creator's UserID does not make this tagged node user-owned.
+	taggedNode := (&Node{
+		ID: 1, Tags: []string{"tag:admin"}, AuthKeyID: &keyID, UserID: &userID,
+	}).View()
+	require.False(t, pak.ConvertsNodeToTagged(taggedNode))
+	require.False(t, pak.RetagsNode(taggedNode))
+
+	otherKey := &PreAuthKey{ID: 2, Tags: []string{"tag:replacement"}}
+	require.True(t, otherKey.RetagsNode(taggedNode))
+	require.False(t, otherKey.ConvertsNodeToTagged(taggedNode))
+	require.True(t, pak.RetagsNode((&Node{ID: 1, Tags: []string{"tag:admin"}}).View()))
+
+	userKey := &PreAuthKey{User: &User{ID: userID}}
+	require.False(t, userKey.ConvertsNodeToTagged(userNode))
+	require.False(t, userKey.RetagsNode(taggedNode))
+	require.False(t, pak.ConvertsNodeToTagged(NodeView{}))
+	require.False(t, pak.RetagsNode(NodeView{}))
+}

hscontrol/util/zlog/zf/fields.go

@@ -78,7 +78,6 @@ const (
 	AuthKeyUsed     = "authkey.used"
 	AuthKeyExpired  = "authkey.expired"
 	AuthKeyReusable = "authkey.reusable"
-	NodeKeyRotation = "nodekey.rotation"
 )
 
 // APIKey fields.