auth, state: answer an expired node's same-key login with NodeKeyExpired

The client then rotates its node key and logs in again, so no key is
judged under the expired one. The machine stays authorised.

hscontrol/auth.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/juanfont/headscale/hscontrol/state"
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/rs/zerolog/log"
 	"gorm.io/gorm"
@@ -182,9 +183,11 @@ func (h *Headscale) handleLogout(
 			Bool("unexpected", true).
 			Msg("Node key expired, forcing re-authentication")
 
+		// The machine stays authorised, as on Tailscale SaaS; only its node
+		// key needs replacing.
 		return &tailcfg.RegisterResponse{
 			NodeKeyExpired:    true,
-			MachineAuthorized: false,
+			MachineAuthorized: true,
 			AuthURL:           "", // Client will need to re-authenticate
 		}, nil
 	}
@@ -420,6 +423,10 @@ func (h *Headscale) handleRegisterWithAuthKey(
 	if err != nil {
 		h.Change(changed)
 
+		if errors.Is(err, state.ErrNodeKeyExpired) {
+			return &tailcfg.RegisterResponse{NodeKeyExpired: true, MachineAuthorized: true}, nil
+		}
+
 		if errors.Is(err, gorm.ErrRecordNotFound) {
 			return nil, NewHTTPError(http.StatusUnauthorized, "invalid pre auth key", nil)
 		}

hscontrol/auth_compat_test.go

@@ -30,12 +30,7 @@ import (
 // pendingLoginSkipReasons lists checks where headscale differs from SaaS,
 // keyed <test_id>/<step>/<node>/<check>. A listed check that matches fails,
 // so a fix must delete its entry.
-var pendingLoginSkipReasons = map[string]string{
-	"pending-login/PL-E2-expire-restart/pl-laptop/exchange-0": expiredRestartUnauthorized,
-	"pending-login/PL-E3-restart/pl-laptop/exchange-1":        expiredRestartUnauthorized,
-}
-
-const expiredRestartUnauthorized = "EXPIRED_RESTART_UNAUTHORIZED: headscale answers an expired node's restart with MachineAuthorized false; SaaS answers true, and both say the node key expired"
+var pendingLoginSkipReasons = map[string]string{}
 
 // pendingLoginNotCompared lists, by key prefix, checks left out: SaaS's
 // answer there is in doubt, so it is no reference.

hscontrol/auth_test.go

@@ -601,10 +601,11 @@ func TestAuthenticationFlows(t *testing.T) {
 				}
 			},
 			machineKey:  machineKey1.Public,
+			wantAuth:    true,
 			wantExpired: true,
 			validate: func(t *testing.T, resp *tailcfg.RegisterResponse, app *Headscale) { //nolint:thelper //nolint:thelper
 				assert.True(t, resp.NodeKeyExpired)
-				assert.False(t, resp.MachineAuthorized)
+				assert.True(t, resp.MachineAuthorized, "the machine stays authorised; only its node key expired")
 			},
 		},
 		// TEST: Ephemeral node is deleted on logout

hscontrol/servertest/reauth_compat_test.go

@@ -39,8 +39,6 @@ import (
 var reauthSkipReasons = map[string]string{
 	"owner-routes/r-expired-retag/expire/online": expiredPeerOffline,
 
-	"owner-routes/r-expired-retag/owner/node-key": expiredKeyAccepted,
-
 	"owner-routes/r-admin/owner/enabled":     adminRetagNoApproval,
 	"owner-routes/r-admin/owner/served":      adminRetagNoApproval,
 	"owner-routes/r-admin/owner/self-routes": adminRetagNoApproval,
@@ -49,14 +47,6 @@ var reauthSkipReasons = map[string]string{
 	"owner-routes/r-expired-retag/withdraw/enabled": withdrawKeepsAutoApproval,
 	"owner-routes/u-convert/withdraw/enabled":       withdrawKeepsAutoApproval,
 
-	"reauth/t-spent-relogin/t-spent/outcome":            expiredKeyNotRotated,
-	"reauth/t-revoked-relogin/t-revoked/outcome":        expiredKeyNotRotated,
-	"reauth/t-valid-relogin/t-valid/outcome":            expiredKeyNotRotated,
-	"reauth/t-valid-relogin/t-valid/node-key":           expiredKeyAccepted,
-	"reauth-user/a4-expired-spent-key/u-spent/outcome":  expiredKeyNotRotated,
-	"reauth-user/a5-expired-valid-key/u-valid/outcome":  expiredKeyNotRotated,
-	"reauth-user/a5-expired-valid-key/u-valid/node-key": expiredKeyAccepted,
-
 	"reauth-foreign/t-logout-logout/t-logout/expiry":    taggedLogoutNotExpired,
 	"reauth-foreign/t-logout-logout/t-logout/peer-obs":  taggedLogoutNotExpired,
 	"reauth-foreign/t-logout-relogin/t-logout/expiry":   taggedLogoutNotExpired,
@@ -101,13 +91,11 @@ var reauthSkipReasons = map[string]string{
 }
 
 const (
-	expiredKeyAccepted        = "EXPIRED_KEY_ACCEPTED: headscale re-registers an expired node under its old node key; SaaS answers NodeKeyExpired, so the client rotates it"
 	adminRetagNoApproval      = "ADMIN_RETAG_NO_APPROVAL: an admin tag change does not auto-approve the routes the new tags may approve; SaaS approves them at once"
 	withdrawKeepsAutoApproval = "WITHDRAW_KEEPS_AUTO_APPROVAL: headscale keeps an auto-approved route once it is withdrawn; SaaS drops it and keeps only admin-set approvals"
 )
 
 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"
 	taggedLogoutNotExpired   = "TAGGED_LOGOUT_NOT_EXPIRED: headscale leaves a tagged node unexpired when it logs out; SaaS expires it"
 	keyLoginTagsRefused      = "KEY_LOGIN_TAGS_REFUSED: headscale refuses a user's key login that requests tags the user owns; SaaS tags the device, converting it in place or creating it tagged"
 	taggedKeyOwnedTagRefused = "TAGGED_KEY_OWNED_TAG_REFUSED: headscale refuses a tagged key's login that requests a tag the key's tags own; SaaS gives the device the requested tags"

hscontrol/state/auth_tagged_expiry_test.go

@@ -235,13 +235,11 @@ func TestExpiredUserNodeReusedOneShotKey_RotatedNodeKey(t *testing.T) {
 
 // TestExpiredUserNodeReusedOneShotKey_SameNodeKey: the security boundary must
 // not depend on the client rotating its node key. An expired node re-using its
-// node key must still re-validate the key, otherwise a spent one-shot key
-// silently re-authorises it.
+// node key is asked for a new one, so a spent one-shot key never re-authorises
+// it.
 func TestExpiredUserNodeReusedOneShotKey_SameNodeKey(t *testing.T) {
 	_, err := reregisterExpiredUserNodeWithSpentKey(t, true)
-	require.Error(t, err,
-		"expired node re-registering with the same node key must re-validate its key")
-	require.Contains(t, err.Error(), "authkey already used")
+	require.ErrorIs(t, err, ErrNodeKeyExpired)
 }
 
 // TestReusableUserPAKReauthOnTaggedNodeNoDuplicate guards against a reusable
@@ -1098,7 +1096,13 @@ func TestExpiredTaggedNodeRequiresValidPreAuthKey(t *testing.T) {
 			require.Equal(t, past.UnixNano(), nsBefore.Expiry)
 			require.Equal(t, nsBefore, rowBefore)
 
-			node, _, err := s.HandleNodeFromPreAuthKey(c.regReq, c.machineKey)
+			_, _, err = s.HandleNodeFromPreAuthKey(c.regReq, c.machineKey)
+			require.ErrorIs(t, err, ErrNodeKeyExpired, "the old node key is answered first")
+
+			req := c.regReq
+			req.NodeKey = key.NewNode().Public()
+
+			node, _, err := s.HandleNodeFromPreAuthKey(req, c.machineKey)
 			if !reusable {
 				require.ErrorIs(t, err, types.PAKError("authkey already used"))
 				nsAfter, rowAfter := c.fields(t)
@@ -1937,7 +1941,7 @@ func TestPAKReregisterRevalidatesAtMutation(t *testing.T) {
 		nsBefore, dbBefore := c.fields(t)
 
 		_, err = c.reregister(view, pak, c.regReq)
-		require.ErrorIs(t, err, types.PAKError("authkey already used"))
+		require.ErrorIs(t, err, ErrNodeKeyExpired)
 
 		nsAfter, dbAfter := c.fields(t)
 		require.Equal(t, nsBefore, nsAfter, "NodeStore node must be untouched")
@@ -1965,13 +1969,14 @@ func TestPAKReregisterRevalidatesAtMutation(t *testing.T) {
 		_, _, 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())
+		nsBefore, dbBefore := c.fields(t)
 
-		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")
+		_, err = c.reregister(view, pak, c.regReq)
+		require.ErrorIs(t, err, ErrNodeKeyExpired, "even a valid key needs a new node key")
+
+		nsAfter, dbAfter := c.fields(t)
+		require.Equal(t, nsBefore, nsAfter)
+		require.Equal(t, dbBefore, dbAfter)
 	})
 
 	t.Run("nodekey_changed_spent_key", func(t *testing.T) {
@@ -2110,9 +2115,9 @@ func TestPAKReregisterRevalidatesAtMutation(t *testing.T) {
 	})
 
 	// 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.
+	// writer runs a login under a new node key. 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")
@@ -2130,6 +2135,7 @@ func TestPAKReregisterRevalidatesAtMutation(t *testing.T) {
 
 		regReq := c.regReq
 		regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key}
+		regReq.NodeKey = key.NewNode().Public()
 
 		view, pak := c.lookup(t, k.Key)
 		require.NoError(t, pak.ValidAt(time.Now()))
@@ -2213,11 +2219,12 @@ func TestPAKReregisterRollbackKeepsPriorWrite(t *testing.T) {
 
 	regReq := c.regReq
 	regReq.Auth = &tailcfg.RegisterResponseAuth{AuthKey: k.Key}
+	regReq.NodeKey = key.NewNode().Public()
 
 	view, pak := c.lookup(t, k.Key)
 	require.False(t, pak.Used, "precondition: the lookup read a fresh single-use key")
 
-	tA := time.Now()
+	tA := time.Now().Add(time.Hour)
 	_, _, err = s.SetNodeExpiry(c.node.ID(), &tA)
 	require.NoError(t, err)
 
@@ -2235,6 +2242,7 @@ func TestPAKReregisterRollbackKeepsPriorWrite(t *testing.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)
+	require.Equal(t, c.regReq.NodeKey, ns.NodeKey)
 }
 
 // TestPAKReregisterConcurrentAdminExpire races a same-key restart with a spent
@@ -3042,8 +3050,8 @@ func TestPAKWakeAcceptsRequestedTags(t *testing.T) {
 
 // TestUserKeyOnTaggedMachine: a user key on a tagged machine re-authenticates
 // the tagged node only when it speaks for it (the user an admin tagged it
-// from); another user's key wakes it, gets a device of its own beside it with
-// a new node key, and cannot lift its expiry.
+// from); another user's key wakes it or gets a device of its own beside it
+// with a new node key. An expired node is asked for a new node key first.
 func TestUserKeyOnTaggedMachine(t *testing.T) {
 	for _, adminTagged := range []bool{false, true} {
 		for _, mode := range []string{"expired", "rotated", "wake"} {
@@ -3099,8 +3107,8 @@ func TestUserKeyOnTaggedMachine(t *testing.T) {
 				require.NoError(t, keyErr)
 
 				switch {
-				case !adminTagged && mode == "expired":
-					require.EqualError(t, err, "authkey cannot re-authenticate this node")
+				case mode == "expired":
+					require.ErrorIs(t, err, ErrNodeKeyExpired)
 					assert.Equal(t, beforeCache, afterCache)
 					assert.Equal(t, beforeDB, afterDB)
 					assert.False(t, storedKey.Used)
@@ -3133,13 +3141,8 @@ func TestUserKeyOnTaggedMachine(t *testing.T) {
 					require.NoError(t, err)
 					assert.Equal(t, []string{"tag:prod"}, afterCache.Tags, "a same-key relogin keeps it tagged")
 					assert.Equal(t, presented.ID, *afterCache.AuthKeyID)
+					assert.Equal(t, beforeCache.Expiry, afterCache.Expiry)
 					assert.Equal(t, 1, s.ListNodes().Len())
-
-					if mode == "expired" {
-						assert.Zero(t, afterCache.Expiry, "a valid key lifts the past expiry")
-					} else {
-						assert.Equal(t, beforeCache.Expiry, afterCache.Expiry)
-					}
 				}
 			})
 		}
@@ -3147,7 +3150,8 @@ func TestUserKeyOnTaggedMachine(t *testing.T) {
 }
 
 // TestPAKWakeRechecksAuthorityAtMutation: another user's key that wakes a
-// live tagged node is refused when an expiry lands before the writer runs.
+// live tagged node is asked for a new node key when an expiry lands before
+// the writer runs.
 func TestPAKWakeRechecksAuthorityAtMutation(t *testing.T) {
 	s := newRetagTestState(t)
 	original, err := s.CreatePreAuthKey(nil, true, false, nil, []string{"tag:prod"})
@@ -3166,9 +3170,55 @@ func TestPAKWakeRechecksAuthorityAtMutation(t *testing.T) {
 
 	beforeCache, beforeDB := c.fields(t)
 	_, err = c.reregister(view, pak, c.regReq)
-	require.EqualError(t, err, "authkey cannot re-authenticate this node")
+	require.ErrorIs(t, err, ErrNodeKeyExpired)
 
 	afterCache, afterDB := c.fields(t)
 	assert.Equal(t, beforeCache, afterCache)
 	assert.Equal(t, beforeDB, afterDB)
 }
+
+// TestExpiredNodeSameKeyLoginAsksForNewKey: an expired node that logs in
+// under its old node key is asked for a new one and changes nothing, even
+// with a valid key; the login with the new key then renews it.
+func TestExpiredNodeSameKeyLoginAsksForNewKey(t *testing.T) {
+	for _, tagged := range []bool{false, true} {
+		t.Run(fmt.Sprintf("tagged=%t", tagged), func(t *testing.T) {
+			s := newRetagTestState(t)
+
+			var (
+				owner *types.UserID
+				tags  []string
+			)
+
+			if tagged {
+				tags = []string{"tag:prod"}
+			} else {
+				owner = s.CreateUserForTest("expired-user").TypedID()
+			}
+
+			pak, err := s.CreatePreAuthKey(owner, true, false, nil, tags)
+			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)
+
+			beforeCache, beforeDB := c.fields(t)
+
+			_, _, err = s.HandleNodeFromPreAuthKey(c.regReq, c.machineKey)
+			require.ErrorIs(t, err, ErrNodeKeyExpired)
+
+			afterCache, afterDB := c.fields(t)
+			assert.Equal(t, beforeCache, afterCache)
+			assert.Equal(t, beforeDB, afterDB)
+
+			req := c.regReq
+			req.NodeKey = key.NewNode().Public()
+			node, _, err := s.HandleNodeFromPreAuthKey(req, c.machineKey)
+			require.NoError(t, err)
+			assert.Equal(t, c.node.ID(), node.ID())
+			assert.False(t, node.IsExpired())
+		})
+	}
+}

hscontrol/state/state.go

@@ -126,6 +126,10 @@ var nodeUpdateColumns = []string{
 // ErrRegistrationExpired is returned when a registration has expired.
 var ErrRegistrationExpired = errors.New("registration expired")
 
+// ErrNodeKeyExpired is returned when an expired node logs in again under its
+// old node key; the client answers it by rotating the key.
+var ErrNodeKeyExpired = errors.New("node key expired")
+
 // ErrNodeKeyInUse is returned when a registration or re-auth claims a NodeKey
 // already bound to a different machine, enforcing the 1:1 NodeKey<->MachineKey
 // binding.
@@ -2871,6 +2875,12 @@ func pakSkipsValidation(n types.NodeView, pak *types.PreAuthKey, nodeKey key.Nod
 // pakReregister distinguishes re-authentication with a valid key from a
 // wake-up that only refreshes a live node's reported metadata.
 func pakReregister(n types.NodeView, pak *types.PreAuthKey, nodeKey key.NodePublic, now time.Time) (bool, error) {
+	// As on Tailscale SaaS, an expired node is asked for a new node key before
+	// any key is judged.
+	if n.NodeKey() == nodeKey && n.IsExpiredAt(now) {
+		return false, ErrNodeKeyExpired
+	}
+
 	err := pak.ValidAt(now)
 	if err == nil {
 		if pak.CanReauthenticate(n) {
@@ -2952,7 +2962,7 @@ func (s *State) HandleNodeFromPreAuthKey(
 	}
 
 	// A wake-up writes no tags, so the tags it asks for are not refused.
-	if tagsErr != nil && (reauth || err != nil) {
+	if tagsErr != nil && !errors.Is(err, ErrNodeKeyExpired) && (reauth || err != nil) {
 		return types.NodeView{}, s.policyChangeSince(genBefore), tagsErr
 	}
 
@@ -3230,9 +3240,9 @@ func (s *State) reregisterNodeWithPAK(
 			if node.IsExpiredAt(now) {
 				node.Expiry = nil
 			}
-		case prior.NodeKey == regReq.NodeKey && !prior.IsExpiredAt(now):
-			// A live node that keeps its node key keeps its expiry, as on
-			// Tailscale SaaS: renewal comes with a new node key.
+		case prior.NodeKey == regReq.NodeKey:
+			// A node that keeps its node key keeps its expiry, as on Tailscale
+			// SaaS: renewal comes with a new node key.
 		case !regReq.Expiry.IsZero():
 			node.Expiry = &regReq.Expiry
 		case s.cfg.Node.Expiry > 0: