diff --git a/hscontrol/auth.go b/hscontrol/auth.go index beb22e4b6..7af5eed92 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -203,9 +203,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 } @@ -216,9 +218,22 @@ func (h *Headscale) handleLogout( // that the node is not attempting to tamper/extend their expiry. // If it is not, we will expire the node or in the case of an ephemeral node, delete it. - // The client is trying to extend their key, this is not allowed. + // A client may bring its expiry forward, as `tailscale debug set-expire` + // does and Tailscale SaaS stores, which turns expiry on for a node without + // one. It may not push the expiry back. if req.Expiry.After(time.Now()) { - return nil, NewHTTPError(http.StatusBadRequest, "extending key is not allowed", nil) + updatedNode, c, err := h.state.BringNodeExpiryForward(node.ID(), req.NodeKey, req.Expiry) + h.Change(c) + + if errors.Is(err, state.ErrExpiryExtension) { + return nil, NewHTTPError(http.StatusBadRequest, "extending key is not allowed", nil) + } + + if err != nil { + return nil, fmt.Errorf("setting node expiry: %w", err) + } + + return nodeToRegisterResponse(updatedNode), nil } // If the request expiry is in the past, we consider it a logout. @@ -254,22 +269,9 @@ func (h *Headscale) handleLogout( Msg("Node is not ephemeral, setting expiry instead of deleting") } - // Tagged nodes have key expiry permanently disabled (they are owned by - // their tags, not a user, and never expire - KB 1068). Logging one out has - // no expiry semantics, so do not stamp an expiry on it: doing so leaves the - // node IsExpired() forever and it can never re-authenticate (#3371). The - // admin path `headscale nodes expire` remains free to set a deliberate - // expiry via SetNodeExpiry; only the logout path is guarded here. - if node.IsTagged() { - log.Debug(). - EmbedObject(node). - Msg("Tagged node logout: not stamping expiry (tagged nodes never expire)") - - return nodeToRegisterResponse(node), nil - } - // Update the internal state with the nodes new expiry, meaning it is - // logged out. + // logged out. A tagged node is expired too, as on Tailscale SaaS; a valid + // key under a new node key brings it back (#3371). // // Clamp the client-supplied value to now: Tailscale sends the // sentinel time.Unix(123, 0) on logout (controlclient/direct.go), @@ -476,6 +478,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_tags_test.go b/hscontrol/auth_tags_test.go index f313a53ec..f39d748f9 100644 --- a/hscontrol/auth_tags_test.go +++ b/hscontrol/auth_tags_test.go @@ -1353,11 +1353,10 @@ func tsLogoutSentinelExpiry() time.Time { // register/logout HTTP-handler path (handleRegister -> handleLogout -> // handleRegister), not by poking SetNodeExpiry directly. // -// Root cause (a): `tailscale logout` sends a past expiry; handleLogout stamps -// it on the node via SetNodeExpiry with no IsTagged guard, so a tagged node — -// which must have key-expiry disabled — becomes Expired. +// `tailscale logout` sends a past expiry, which expires the tagged node as on +// Tailscale SaaS. The relogin must bring it back. // -// Root cause (b): on the next `tailscale up --auth-key `, +// Root cause: on the next `tailscale up --auth-key `, // HandleNodeFromPreAuthKey sees an expired node, takes the expired-node // validation path, consumes the fresh single-use key on the in-place // re-registration, yet leaves the node expired (the expiry-refresh block is @@ -1411,14 +1410,9 @@ func TestIssue3371_TaggedNodeLogoutReloginSingleUseKey(t *testing.T) { _, err = app.handleRegister(context.Background(), logoutReq, machineKey.Public()) require.NoError(t, err) - // A tagged node must NOT be expired by logout — tagged nodes never expire. - // This is root cause (a); it fails before the fix. nodeAfterLogout, found := app.state.GetNodeByNodeKey(nodeKey.Public()) require.True(t, found) - assert.False(t, nodeAfterLogout.IsExpired(), - "issue #3371 root cause (a): logout must not expire a tagged node") - assert.False(t, nodeAfterLogout.Expiry().Valid(), - "issue #3371 root cause (a): tagged node must keep key-expiry disabled after logout") + assert.True(t, nodeAfterLogout.IsExpired(), "logout expires a tagged node") // `tailscale up --auth-key $KEY2`: a BRAND NEW single-use key, and the client // rotates its node key (as the real client does on relogin). @@ -1518,14 +1512,9 @@ func TestIssue3371_TaggedNodeLogoutReloginReusableKey(t *testing.T) { "issue #3371: tagged node must have key-expiry disabled after reusable-key relogin") } -// TestIssue3371_TaggedNodeLogoutDoesNotSetExpiry pins the deepest root cause -// (a) in isolation: `tailscale logout` on a tagged node must not stamp an -// expiry at all. This is the assertion PR #3372 does not make — it leaves -// handleLogout expiring tagged nodes and only unwinds the damage on the next -// registration. Keeping this separate from the relogin tests means a -// regression that re-introduces logout-sets-expiry is caught even if the -// re-registration cleanup masks it. -func TestIssue3371_TaggedNodeLogoutDoesNotSetExpiry(t *testing.T) { +// TestTaggedNodeLogoutExpiresIt: `tailscale logout` expires a tagged node, as +// on Tailscale SaaS, and the expiry reaches the database. +func TestTaggedNodeLogoutExpiresIt(t *testing.T) { app := createTestApp(t) user := app.state.CreateUserForTest("tag-logout-noexpiry") @@ -1556,18 +1545,51 @@ func TestIssue3371_TaggedNodeLogoutDoesNotSetExpiry(t *testing.T) { nodeAfterLogout, found := app.state.GetNodeByNodeKey(nodeKey.Public()) require.True(t, found) assert.True(t, nodeAfterLogout.IsTagged(), "node stays tagged through logout") - assert.False(t, nodeAfterLogout.Expiry().Valid(), - "issue #3371 root cause (a): logout must not set an expiry on a tagged node") - assert.False(t, nodeAfterLogout.IsExpired(), - "issue #3371 root cause (a): tagged node must not be expired by logout") + assert.True(t, nodeAfterLogout.IsExpired(), "logout expires a tagged node") - // The database column must be NULL, not a clamped 'now' timestamp — a - // persisted expiry survives restart and re-triggers the lockout. var dbNode types.Node require.NoError(t, app.state.DB().DB.First(&dbNode, nodeAfterLogout.ID().Uint64()).Error) - assert.Nil(t, dbNode.Expiry, - "issue #3371 root cause (a): tagged node's DB expiry must remain NULL after logout") + require.NotNil(t, dbNode.Expiry) + assert.True(t, dbNode.IsExpired(), "the logout expiry is stored") +} + +// TestClientBringsExpiryForward: a client may ask for an earlier expiry, which +// turns expiry on for a node without one, but may not push it back. +func TestClientBringsExpiryForward(t *testing.T) { + app := createTestApp(t) + + pak, err := app.state.CreatePreAuthKey(nil, true, false, nil, []string{"tag:tag1"}) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + + _, err = app.handleRegister(context.Background(), tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{AuthKey: pak.Key}, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{Hostname: "set-expire"}, + }, machineKey.Public()) + require.NoError(t, err) + + soon := time.Now().Add(time.Hour).Truncate(time.Second) + resp, err := app.handleRegister(context.Background(), tailcfg.RegisterRequest{ + NodeKey: nodeKey.Public(), + Expiry: soon, + }, machineKey.Public()) + require.NoError(t, err) + assert.False(t, resp.NodeKeyExpired) + + node, found := app.state.GetNodeByNodeKey(nodeKey.Public()) + require.True(t, found) + require.True(t, node.Expiry().Valid(), "the earlier expiry turns expiry on") + assert.True(t, soon.Equal(node.Expiry().Get())) + + _, err = app.handleRegister(context.Background(), tailcfg.RegisterRequest{ + NodeKey: nodeKey.Public(), + Expiry: soon.Add(time.Hour), + }, machineKey.Public()) + require.ErrorContains(t, err, "extending key is not allowed") } // TestIssue3371_UserOwnedNodeLogoutStillExpires is the guard rail: the fix for diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 8f14abfc5..0e0f64951 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -605,10 +605,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/expiry_compat_test.go b/hscontrol/servertest/expiry_compat_test.go index c16226a0e..2d8a24144 100644 --- a/hscontrol/servertest/expiry_compat_test.go +++ b/hscontrol/servertest/expiry_compat_test.go @@ -42,16 +42,11 @@ var expirySkipReasons = map[string]string{ "expiry/C4-expire-r2/r2/paths-r1": expiredPeerPaths, "expiry/C4-expire-r2/r2/self": selfExpiredFlags, - "expiry/D2-set-expire/subject/key-expiry": clientExpiryRefused, - "expiry/D2-set-expire/subject/outcome": clientExpiryRefused, - - "expiry/D3-past-expiry/subject/expired-observer": clientExpiryRefused, - "expiry/D3-past-expiry/subject/key-expiry": clientExpiryRefused, - "expiry/D3-past-expiry/subject/paths-observer": clientExpiryRefused, - "expiry/D3-past-expiry/subject/self": clientExpiryRefused, - - "expiry/D5-logout-tagged/tagged/key-expiry": taggedLogoutNotExpired, - "expiry/D5-logout-tagged/tagged/outcome": taggedLogoutNotExpired, + // A client asked for this expiry; once it passes the node is expired + // like any other. + "expiry/D3-past-expiry/subject/online-observer": expiredPeerOffline, + "expiry/D3-past-expiry/subject/paths-observer": expiredPeerPaths, + "expiry/D3-past-expiry/subject/self": selfExpiredFlags, "expiry/A2-expire/laptop/online-observer": expiredPeerOffline, "expiry/A2-expire/laptop/online-peer1": expiredPeerOffline, @@ -111,7 +106,6 @@ var expirySkipReasons = map[string]string{ const ( selfExpiredFlags = "SELF_EXPIRED_FLAGS: headscale sends an expired node its own entry with MachineAuthorized false and Expired set; SaaS keeps MachineAuthorized true and leaves Expired out" expiredPeerPaths = "EXPIRED_PEER_PATHS: headscale sends peers an expired node's endpoints and home DERP; SaaS leaves both out" - clientExpiryRefused = "CLIENT_EXPIRY_REFUSED: headscale refuses a client's request for an earlier key expiry; SaaS stores it, and the node expires then" nameserversFromConfig = "NAMESERVERS_FROM_CONFIG: headscale reads nameservers from its config and cannot change them while running; SaaS pushes a change to every node, an expired one too" ) @@ -153,6 +147,27 @@ func runNodeExpiryCompat(t *testing.T, id, observer string) { r := newStepReplay(t, c, opts...) r.paths = true + // The test server runs no scheduled tasks. Run headscale's expiry pass on + // a short timer, so peers learn when an expiry a client asked for passes. + go func() { + tick := time.NewTicker(250 * time.Millisecond) + defer tick.Stop() + + last := time.Unix(0, 0) + + for { + select { + case <-t.Context().Done(): + return + case <-tick.C: + next, changes, _ := r.srv.State().ExpireExpiredNodes(last) + last = next + + r.srv.App.Change(changes...) + } + } + }() + // A change is judged against the step before only, so both sides // compare the same span. prevCaps := c.Captures diff --git a/hscontrol/servertest/lifecycle_compat_test.go b/hscontrol/servertest/lifecycle_compat_test.go index 9ccb50250..321b643f8 100644 --- a/hscontrol/servertest/lifecycle_compat_test.go +++ b/hscontrol/servertest/lifecycle_compat_test.go @@ -56,10 +56,6 @@ var lifecycleSkipReasons = map[string]string{ "ephemeral-lifecycle/collection/perm-to-eph/left": ephemeralFollowsKey, "ephemeral-lifecycle/collection/perm-to-eph/removed-observer": ephemeralFollowsKey, "ephemeral-lifecycle/collection/perm-to-eph/present-observer": ephemeralFollowsKey, - - "ephemeral-lifecycle/E5-mem-shutdown/mem/expiry": taggedLogoutNotExpired, - "ephemeral-lifecycle/E5-mem-shutdown/mem/outcome": taggedLogoutNotExpired, - "ephemeral-lifecycle/E5-mem-shutdown/mem/peer-observer": taggedLogoutNotExpired, } // SaaS runs auto-approval when a router's announced routes change and on a diff --git a/hscontrol/servertest/reauth_compat_test.go b/hscontrol/servertest/reauth_compat_test.go index c863cda27..f439e1dd4 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,19 +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, - "reauth-foreign/t-logout-relogin/t-logout/peer-obs": taggedLogoutNotExpired, - "identity/w-tagreq-relogin-tags/w-tagreq/owner": keyLoginTagsRefused, "identity/w-tagreq-relogin-tags/w-tagreq/outcome": keyLoginTagsRefused, "identity/w-tagreq-relogin-tags/w-tagreq/node-key": keyLoginTagsRefused, @@ -101,14 +86,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" refusalText = "REFUSAL_TEXT: SaaS refuses a key by naming its ID; headscale says why the key is refused" diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 3eefdada0..28b575d8c 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/persist_test.go b/hscontrol/state/persist_test.go index c6124890e..d3f0a60be 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1896,6 +1896,49 @@ func reopenLabels(t *testing.T, s *State) map[types.NodeID]string { return out } +// TestBringNodeExpiryForwardDecidesInWriter: a client's earlier expiry is +// checked against the node as the writer holds it, so an admin expiry that +// lands after the caller read the node is not pushed back, and a node key +// the node no longer holds changes nothing. +func TestBringNodeExpiryForwardDecidesInWriter(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + read, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + _, _, err := s.SetNodeExpiry(nodeID, nil) + require.NoError(t, err) + + soon := time.Now().Add(time.Hour).Truncate(time.Second) + node, _, err := s.BringNodeExpiryForward(nodeID, read.NodeKey(), soon) + require.NoError(t, err) + require.True(t, node.Expiry().Valid(), "an earlier expiry turns expiry on") + assert.True(t, soon.Equal(node.Expiry().Get())) + + _, _, err = s.BringNodeExpiryForward(nodeID, read.NodeKey(), soon.Add(time.Minute)) + require.ErrorIs(t, err, ErrExpiryExtension, "a later expiry") + + _, _, err = s.BringNodeExpiryForward(nodeID, key.NewNode().Public(), soon.Add(-time.Minute)) + require.ErrorIs(t, err, ErrExpiryExtension, "another node key") + + past := time.Now().Add(-time.Minute).Truncate(time.Second) + _, _, err = s.SetNodeExpiry(nodeID, &past) + require.NoError(t, err) + + _, _, err = s.BringNodeExpiryForward(nodeID, read.NodeKey(), soon.Add(-time.Minute)) + require.ErrorIs(t, err, ErrExpiryExtension, "the admin expiry is earlier") + + current, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.True(t, current.IsExpired(), "the admin expiry stands") + + stored, err := s.db.GetNodeByID(nodeID) + require.NoError(t, err) + require.NotNil(t, stored.Expiry) + assert.True(t, past.Equal(*stored.Expiry)) +} + // TestSetNodeExpiryRejectsUnencodableExpiry proves an admin cannot store a key // expiry that every map carrying the node would fail to encode. func TestSetNodeExpiryRejectsUnencodableExpiry(t *testing.T) { diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 29e7848e4..b1afdcfe4 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -140,11 +140,19 @@ var ErrAuthCacheClosed = errors.New("authentication cache closed") // ErrAuthRequestFinished prevents reusing an already resolved verdict channel. var ErrAuthRequestFinished = errors.New("authentication request already finished") +// 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. var ErrNodeKeyInUse = errors.New("node key already in use by another machine") +// ErrExpiryExtension is returned when a client asks for an expiry no earlier +// than its node's, or under a node key the node no longer holds. +var ErrExpiryExtension = errors.New("extending key is not allowed") + // ErrNodeKeyNotCurrent is returned when a map request uses a replaced node key. var ErrNodeKeyNotCurrent = errors.New("node key is no longer current") @@ -1079,6 +1087,31 @@ func (s *State) ListEphemeralNodes() views.Slice[types.NodeView] { // SetNodeExpiry updates the expiration time for a node. // If expiry is nil, the node's expiry is disabled (node will never expire). func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.NodeView, change.Change, error) { + return s.setNodeExpiry(nodeID, expiry, nil) +} + +// BringNodeExpiryForward stores the expiry a client asks for when the node +// still holds nodeKey and has no expiry or a later one. The NodeStore writer +// decides, so an expiry that lands meanwhile, such as an admin's, is never +// pushed back. +func (s *State) BringNodeExpiryForward( + nodeID types.NodeID, + nodeKey key.NodePublic, + expiry time.Time, +) (types.NodeView, change.Change, error) { + return s.setNodeExpiry(nodeID, &expiry, func(node *types.Node) bool { + return node.NodeKey == nodeKey && + (node.Expiry == nil || node.Expiry.IsZero() || expiry.Before(*node.Expiry)) + }) +} + +// setNodeExpiry sets the node's expiry. A non-nil allow decides on the node +// as the writer holds it; a refusal writes nothing. +func (s *State) setNodeExpiry( + nodeID types.NodeID, + expiry *time.Time, + allow func(*types.Node) bool, +) (types.NodeView, change.Change, error) { if expiry != nil { utc, err := types.ValidateExpiry(*expiry) if err != nil { @@ -1088,7 +1121,7 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.Nod expiry = &utc } - var onlineChanged, expired bool + var onlineChanged, expired, refused bool genBefore := s.polMan.NodesGeneration() @@ -1097,6 +1130,12 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.Nod // must make the exact same change. If the database update fails, the [NodeStore] // change will remain, and the change describing it is returned with the error. n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) { + if allow != nil && !allow(node) { + refused = true + + return + } + wasOnline := node.Online() node.Expiry = expiry // Control stays connected in NeedsLogin so an expiry extension can @@ -1111,6 +1150,10 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry *time.Time) (types.Nod return types.NodeView{}, change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, nodeID) } + if refused { + return types.NodeView{}, change.Change{}, ErrExpiryExtension + } + if expired { // Expiry revokes the human check even if a pre-auth key renews the node. s.sshCheckMu.Lock() @@ -3012,9 +3055,8 @@ func loginNode(all map[types.UserID]types.NodeView, userID types.UserID) (types. // --authkey=KEY") re-presents a spent key legitimately. Anything that // authorises something new must present a valid key: // - a node key rotation; -// - an expired node. Tagged nodes can carry an administrative expiry or -// retain their expiry after an admin tags them. Legacy logout stamps -// were cleared by migration; logout no longer expires tagged nodes; +// - an expired node. Tagged nodes expire at logout, by an administrative +// expiry, or keep the expiry they had when an admin tagged them; // - 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. @@ -3040,6 +3082,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) { @@ -3122,7 +3170,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 } @@ -3400,9 +3448,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: diff --git a/integration/tags_test.go b/integration/tags_test.go index 4becf7a70..b3b845d0e 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -3419,10 +3419,9 @@ func TestTagsAuthKeyWithoutUserCLIRegisterAddsUserDevice(t *testing.T) { // TestTaggedNodeLogoutReloginSingleUseKeyOnline reproduces issue #3371 // end-to-end with a real tailscale client: a tagged node registered with a // single-use key logs out (`tailscale logout`) and re-authenticates with a -// FRESH single-use tagged key. Tagged nodes never expire (KB 1068), so logout -// must not stamp an expiry; before the fix the node was left permanently -// expired and the fresh key was consumed on a re-registration that still -// reported NodeKeyExpired, locking the node out forever. +// FRESH single-use tagged key. Logout expires the tagged node, as on Tailscale +// SaaS; before the fix the fresh key was consumed on a re-registration that +// still reported NodeKeyExpired, locking the node out forever. // // The observable proof at the integration level is that after relogin the node // is back online with a NULL expiry and the same node ID — not stuck expired. @@ -3483,15 +3482,13 @@ func TestTaggedNodeLogoutReloginSingleUseKeyOnline(t *testing.T) { } }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "waiting for initial registration") - // `tailscale logout`. A tagged node must not be expired by this. + // `tailscale logout` expires the tagged node. err = client.Logout() require.NoError(t, err) err = client.WaitForNeedsLogin(integrationutil.ScaledTimeout(60 * time.Second)) require.NoError(t, err) - // The node must remain in the DB, tagged, and crucially NOT carry a - // stale expiry. This is the #3371 root cause (a) surface. assert.EventuallyWithT(t, func(c *assert.CollectT) { nodes, err := headscale.ListNodes() assert.NoError(c, err) @@ -3499,9 +3496,9 @@ func TestTaggedNodeLogoutReloginSingleUseKeyOnline(t *testing.T) { if len(nodes) == 1 { assertNodeHasTagsWithCollect(c, nodes[0], []string{"tag:valid-owned"}) - assert.Nil(c, nodes[0].Expiry, "#3371: logout must not stamp expiry on a tagged node") + assert.NotNil(c, nodes[0].Expiry, "logout expires a tagged node") } - }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "tagged node must survive logout without expiry") + }, integrationutil.StatusReadyTimeout, integrationutil.SlowPoll, "tagged node must survive logout, expired") // KEY2: a FRESH single-use tagged key. Relogin. key2, err := scenario.CreatePreAuthKeyWithTags(userID, false, false, []string{"tag:valid-owned"})