diff --git a/hscontrol/auth.go b/hscontrol/auth.go index 7d42c3d7a..73bb3bad2 100644 --- a/hscontrol/auth.go +++ b/hscontrol/auth.go @@ -198,9 +198,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. @@ -236,22 +249,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), diff --git a/hscontrol/auth_tags_test.go b/hscontrol/auth_tags_test.go index da6ccf28d..33d64b22a 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/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 5e655af4a..f439e1dd4 100644 --- a/hscontrol/servertest/reauth_compat_test.go +++ b/hscontrol/servertest/reauth_compat_test.go @@ -47,11 +47,6 @@ var reauthSkipReasons = map[string]string{ "owner-routes/r-expired-retag/withdraw/enabled": withdrawKeepsAutoApproval, "owner-routes/u-convert/withdraw/enabled": withdrawKeepsAutoApproval, - "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, @@ -96,7 +91,6 @@ const ( ) const ( - 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/persist_test.go b/hscontrol/state/persist_test.go index c00fe84c3..fa71dd9a3 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -1801,3 +1801,46 @@ 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)) +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index f433beadc..7705b9a71 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -135,6 +135,10 @@ var ErrNodeKeyExpired = errors.New("node key expired") // 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") @@ -978,7 +982,32 @@ 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) { - var onlineChanged, expired bool + 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) { + var onlineChanged, expired, refused bool genBefore := s.polMan.NodesGeneration() @@ -987,6 +1016,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 @@ -1001,6 +1036,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() @@ -2847,9 +2886,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. diff --git a/integration/tags_test.go b/integration/tags_test.go index 5940187cd..43fd00e8f 100644 --- a/integration/tags_test.go +++ b/integration/tags_test.go @@ -3415,10 +3415,9 @@ func TestTagsAuthKeyConvertToUserViaCLIRegister(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. @@ -3479,15 +3478,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) @@ -3495,9 +3492,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"})