From 588a2f934cebfaedcc484d3c099cd52a863612b9 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 21 Oct 2026 20:00:12 +0000 Subject: [PATCH] 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 +- hscontrol/auth_compat_test.go | 7 +- hscontrol/auth_test.go | 3 +- hscontrol/servertest/reauth_compat_test.go | 12 --- hscontrol/state/auth_tagged_expiry_test.go | 108 +++++++++++++++------ hscontrol/state/state.go | 18 +++- 6 files changed, 104 insertions(+), 53 deletions(-) diff --git a/hscontrol/auth.go b/hscontrol/auth.go index b2b0c6e8f..7d42c3d7a 100644 --- a/hscontrol/auth.go +++ b/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) } diff --git a/hscontrol/auth_compat_test.go b/hscontrol/auth_compat_test.go index 233e4ee88..0eff5fdfc 100644 --- a/hscontrol/auth_compat_test.go +++ b/hscontrol/auth_compat_test.go @@ -30,12 +30,7 @@ import ( // pendingLoginSkipReasons lists checks where headscale differs from SaaS, // keyed ///. 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. diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 380ee1f83..50b00bf96 100644 --- a/hscontrol/auth_test.go +++ b/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 diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index c863cda27..5e655af4a 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/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" diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 1aa1abc8b..39f756456 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/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()) + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 1a96cfba9..f90057f91 100644 --- a/hscontrol/state/state.go +++ b/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) { @@ -2953,7 +2963,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 } @@ -3231,9 +3241,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 = ®Req.Expiry case s.cfg.Node.Expiry > 0: