From a8d6f5be81e578a078001b908e41611ec03ce69e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20-nexus-=20Mlyn=C3=A1=C5=99?= Date: Thu, 1 Oct 2026 23:52:11 +0200 Subject: [PATCH] state: send a relogin as a whole node unless only its keys changed A PeerChange patch cannot clear a peer's Expired flag or carry the Hostinfo the relogin stored. Fixes #3531 --- .github/workflows/test-integration.yaml | 1 + CHANGELOG.md | 1 + hscontrol/auth_test.go | 67 ++++++++ hscontrol/servertest/lifecycle_test.go | 91 +++++++++++ hscontrol/state/auth_tagged_expiry_test.go | 3 +- hscontrol/state/persist_test.go | 47 +++++- hscontrol/state/state.go | 87 ++++++---- hscontrol/types/change/change.go | 9 +- integration/auth_web_flow_test.go | 176 +++++++++++++++++++++ 9 files changed, 443 insertions(+), 39 deletions(-) diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index 4c7413392..51bf27339 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -259,6 +259,7 @@ jobs: - TestOIDCReloginSameUserRoutesPreserved - TestAuthWebFlowAuthenticationPingAll - TestAuthWebFlowLogoutAndReloginSameUser + - TestAuthWebFlowReloginExpiredNode - TestAuthWebFlowLogoutAndReloginNewUser - TestApiKeyCommand - TestApiKeyCommandValidation diff --git a/CHANGELOG.md b/CHANGELOG.md index 91811c952..9b7e95bc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -152,6 +152,7 @@ clients, and how to run the same setup without Nix. - Fix policy `tests` and `sshTests` failing for a group that names an unknown user [#3516](https://github.com/juanfont/headscale/pull/3516) - Fix an unknown user in `nodeAttrs` rejecting the policy [#3516](https://github.com/juanfont/headscale/pull/3516) - Fix an ephemeral node being deleted while online, when it reconnected while its previous session was being marked offline [#3539](https://github.com/juanfont/headscale/pull/3539) +- Fix peers dropping traffic from a node that re-authenticates after its key expired [#3541](https://github.com/juanfont/headscale/pull/3541) - Fix an exit node or subnet router not seeing its own approved routes until it reconnected, so `tailscale status` did not show it offering an exit node [#3518](https://github.com/juanfont/headscale/pull/3518) diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 5ff089805..28cde5d62 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -4328,6 +4328,73 @@ func TestHandleNodeFromAuthPath_OldUserNil_NoPanic(t *testing.T) { assert.Equal(t, userB.ID, node.UserID().Get(), "new node belongs to userB") } +// TestHandleNodeFromAuthPath_ReloginWholeNode covers an interactive (web or +// OIDC) relogin that rotates the node key. A [tailcfg.PeerChange] patch can +// neither clear a peer's Expired flag nor carry Hostinfo, so a relogin that +// un-expires the node or changes Hostinfo peers read must be sent as a whole +// node. A relogin that only rotates keys keeps the key-rotation patch. +func TestHandleNodeFromAuthPath_ReloginWholeNode(t *testing.T) { + tests := []struct { + name string + expired bool + services []tailcfg.Service + wantPatch bool + }{ + {name: "keys only", wantPatch: true}, + {name: "expired", expired: true}, + { + name: "peer-visible hostinfo", + services: []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 4242}}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + app := createTestApp(t) + + user := app.state.CreateUserForTest("authpath-relogin") + node := app.state.CreateRegisteredNodeForTest(user, "authpath-relogin") + node.Hostinfo = &tailcfg.Hostinfo{Hostname: node.Hostname} + + if tt.expired { + node.Expiry = new(time.Now().Add(-time.Minute)) + } + + app.state.PutNodeInStoreForTest(*node) + + newNodeKey := key.NewNode() + authID := types.MustAuthID() + app.state.SetAuthCacheEntry(authID, types.NewRegisterAuthRequest(&types.RegistrationData{ + MachineKey: node.MachineKey, + NodeKey: newNodeKey.Public(), + Hostname: node.Hostname, + Hostinfo: &tailcfg.Hostinfo{ + Hostname: node.Hostname, + Services: tt.services, + }, + })) + + relogged, c, err := app.state.HandleNodeFromAuthPath( + authID, + types.UserID(user.ID), + nil, + "oidc", + ) + require.NoError(t, err) + require.Equal(t, node.ID, relogged.ID(), "relogin must update the existing node") + require.Equal(t, newNodeKey.Public(), relogged.NodeKey()) + + if tt.wantPatch { + assert.Empty(t, c.PeersChanged, "relogin must not be a whole-node add") + assert.Len(t, c.PeerPatches, 1, "relogin must be a peer patch") + } else { + assert.Empty(t, c.PeerPatches, "relogin must not be a peer patch") + assert.Contains(t, c.PeersChanged, node.ID, "relogin must be a whole-node add") + } + }) + } +} + // TestWaitForFollowupMachineKeyMismatch covers the followup poll in // [Headscale.waitForFollowup]. That poll is authenticated only by the auth ID // embedded in the followup URL, so without a machine-key check anyone who diff --git a/hscontrol/servertest/lifecycle_test.go b/hscontrol/servertest/lifecycle_test.go index eefc7e92a..b83bd1983 100644 --- a/hscontrol/servertest/lifecycle_test.go +++ b/hscontrol/servertest/lifecycle_test.go @@ -14,6 +14,8 @@ import ( "github.com/juanfont/headscale/hscontrol/types/change" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/control/controlclient" + "tailscale.com/tailcfg" "tailscale.com/types/netmap" ) @@ -270,6 +272,95 @@ func TestRestoredExpirySurvivesQueuedChanges(t *testing.T) { } } +// TestReloginOfExpiredNodeClearsPeerExpiry covers #3531. Peers hold an expired +// node with Expired=true, which only a whole node from control can clear: +// [tailcfg.PeerChange] has no Expired field. Hostinfo is identical across the +// relogin, so no Hostinfo-driven whole-node update can mask a missing one. +func TestReloginOfExpiredNodeClearsPeerExpiry(t *testing.T) { + t.Parallel() + + h := servertest.NewHarness(t, 2, + servertest.WithServerOptions(servertest.WithBatchDelay(10*time.Millisecond)), + ) + client, observer := h.Client(0), h.Client(1) + id := findNodeID(t, h.Server, client.Name) + node, ok := h.Server.State().GetNodeByID(id) + require.True(t, ok) + + oldKey := node.NodeKey() + + expiry := time.Now() + _, c, err := h.Server.State().SetNodeExpiry(id, &expiry) + require.NoError(t, err) + h.Server.App.Change(c) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + peer, found := observer.PeerByName(client.Name) + if assert.True(c, found) { + assert.True(c, peer.Expired()) + } + }, 5*time.Second, 10*time.Millisecond, "observer must see the node expired") + + // The server answers the expired key with NodeKeyExpired, so the client + // generates a new key, the way tailscaled re-authenticates. + client.Reconnect(t) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + current, found := h.Server.State().GetNodeByID(id) + if !assert.True(c, found) { + return + } + + assert.NotEqual(c, oldKey, current.NodeKey()) + assert.False(c, current.IsExpired()) + + peer, found := observer.PeerByName(client.Name) + if assert.True(c, found) { + assert.Equal(c, current.NodeKey(), peer.Key()) + assert.False(c, peer.Expired(), "relogin must clear the peer's expired flag") + } + }, 5*time.Second, 10*time.Millisecond, "observer must see the relogged node with its new key and not expired") +} + +// TestReloginPeerVisibleHostinfoReachesPeers covers the rest of the #3531 +// class: a relogin stores the RegisterRequest's Hostinfo, so the following +// MapRequest carries no Hostinfo delta, and a peer-visible change made at +// relogin reaches peers only if the relogin itself sends the whole node. +func TestReloginPeerVisibleHostinfoReachesPeers(t *testing.T) { + t.Parallel() + + h := servertest.NewHarness(t, 2, + servertest.WithServerOptions(servertest.WithBatchDelay(10*time.Millisecond)), + ) + client, observer := h.Client(0), h.Client(1) + + svc := tailcfg.Service{Proto: tailcfg.PeerAPI4, Port: 4242} + // Same Hostinfo as servertest.NewClient, plus a service. + hi := &tailcfg.Hostinfo{ + BackendLogID: "servertest-" + client.Name, + Hostname: client.Name, + Services: []tailcfg.Service{svc}, + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + // `tailscale up --force-reauth`: an interactive login rotates the key. + client.Disconnect(t) + client.Direct().SetHostinfo(hi) + _, err := client.Direct().TryLogin(ctx, controlclient.LoginInteractive) + require.NoError(t, err) + require.NoError(t, client.RestartPoll(ctx)) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + peer, found := observer.PeerByName(client.Name) + if assert.True(c, found) { + assert.Equal(c, []tailcfg.Service{svc}, peer.Hostinfo().Services().AsSlice(), + "peer must see the Hostinfo the node relogged in with") + } + }, 5*time.Second, 10*time.Millisecond, "observer must see the relogin's peer-visible Hostinfo") +} + func TestNodeExpiryRouteFailover(t *testing.T) { t.Parallel() diff --git a/hscontrol/state/auth_tagged_expiry_test.go b/hscontrol/state/auth_tagged_expiry_test.go index 4398e3085..c626fb009 100644 --- a/hscontrol/state/auth_tagged_expiry_test.go +++ b/hscontrol/state/auth_tagged_expiry_test.go @@ -1767,8 +1767,9 @@ func (c pakReregCase) reregister( regReq tailcfg.RegisterRequest, ) (types.NodeView, error) { hi := regReq.Hostinfo.Clone() + node, _, err := c.s.reregisterNodeWithPAK(view, pak, regReq, c.machineKey, hi.Hostname, hi) - return c.s.reregisterNodeWithPAK(view, pak, regReq, c.machineKey, hi.Hostname, hi) + return node, err } // pakNodeFields are the node fields a re-registration writes. Expiry is in diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 5d503e6b1..f0a7fbb41 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -354,7 +354,7 @@ func TestReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) { require.NoError(t, err) // Attacker re-authenticates its own node but supplies the victim's NodeKey. - _, err = s.applyAuthNodeUpdate(authNodeUpdateParams{ + _, _, err = s.applyAuthNodeUpdate(authNodeUpdateParams{ ExistingNode: attackerNode, RegData: &types.RegistrationData{ MachineKey: attackerMachine.Public(), @@ -414,7 +414,7 @@ func TestReauthPreservesEndpointsWhenClientOmitsThem(t *testing.T) { // Node re-authenticates, rotating its NodeKey. The RegisterRequest carries // no endpoints. - updated, err := s.applyAuthNodeUpdate(authNodeUpdateParams{ + updated, _, err := s.applyAuthNodeUpdate(authNodeUpdateParams{ ExistingNode: node, RegData: &types.RegistrationData{ MachineKey: machine.Public(), @@ -434,7 +434,7 @@ func TestReauthPreservesEndpointsWhenClientOmitsThem(t *testing.T) { "re-auth without reported endpoints must preserve the node's live endpoints") } -// TestReauthChange covers the decision both re-auth paths share: a same-user +// TestReauthChange covers the decision both re-auth paths share: a keys-only // relogin must be an incremental peer patch (so the tailscale client takes its // fast patch path), never a whole-node add (which strands a re-keyed, // momentarily-endpoint-less peer disco-deaf); a policy change forces a full @@ -461,6 +461,47 @@ func TestReauthChange(t *testing.T) { assert.False(t, pol.IsEmpty(), "a policy change must be non-empty") } +// TestOnlyKeysChanged covers which relogins may ride the key-rotation patch: +// one that flips Expired or changes Hostinfo peers read must not, as +// [tailcfg.PeerChange] carries neither (#3531). +func TestOnlyKeysChanged(t *testing.T) { + now := time.Now() + past, future := now.Add(-time.Minute), now.Add(time.Hour) + + node := func(expiry time.Time, hi tailcfg.Hostinfo) types.NodeView { + n := types.Node{ + NodeKey: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + Expiry: &expiry, + Hostinfo: &hi, + } + + return n.View() + } + + hi := tailcfg.Hostinfo{Hostname: "node", OS: "linux"} + withOSVersion, withService := hi, hi + withOSVersion.OSVersion = "6.1" + withService.Services = []tailcfg.Service{{Proto: tailcfg.PeerAPI4, Port: 4242}} + + tests := []struct { + name string + before, after types.NodeView + want bool + }{ + {name: "keys", before: node(future, hi), after: node(future, hi), want: true}, + {name: "hostinfo peers do not read", before: node(future, hi), after: node(future, withOSVersion), want: true}, + {name: "un-expired", before: node(past, hi), after: node(future, hi)}, + {name: "hostinfo peers read", before: node(future, hi), after: node(future, withService)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, onlyKeysChanged(tt.before, tt.after, now)) + }) + } +} + // TestPreAuthKeyReauthRejectsNodeKeyClaimedByAnotherMachine is the pre-auth-key // analogue of TestReauthRejectsNodeKeyClaimedByAnotherMachine: re-registering // via a pre-auth key must enforce the same 1:1 NodeKey<->MachineKey binding the diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 627425f94..04f2c8cde 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1859,8 +1859,10 @@ type authNodeUpdateParams struct { // applyAuthNodeUpdate applies common update logic for re-authenticating or converting // an existing node. It updates the node in [NodeStore], processes RequestTags, and -// persists changes to the database. -func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, error) { +// persists changes to the database. The bool reports whether peers read the +// node it replaced and the update the same, apart from its keys; see +// [onlyKeysChanged]. +func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView, bool, error) { regData := params.RegData // Log the operation type if params.IsConvertFromTag { @@ -1901,7 +1903,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView rejectedTags := s.validateRequestTagsForReauth(params.ExistingNode, authUser, requestTags) if len(rejectedTags) > 0 { - return types.NodeView{}, fmt.Errorf( + return types.NodeView{}, false, fmt.Errorf( "%w %v are invalid or not permitted", ErrRequestedTagsInvalidOrNotPermitted, rejectedTags, @@ -1916,11 +1918,11 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView // the NodeStore NodeKey index (denying the victim service). if existing, ok := s.nodeStore.GetNodeByNodeKey(regData.NodeKey); ok && existing.MachineKey() != regData.MachineKey { - return types.NodeView{}, ErrNodeKeyInUse + return types.NodeView{}, false, ErrNodeKeyInUse } // Update existing node in [NodeStore] - validation passed, safe to mutate - updatedNodeView, ok := s.nodeStore.UpdateNode(params.ExistingNode.ID(), func(node *types.Node) { + before, updatedNodeView, ok := s.nodeStore.UpdateNodeDiff(params.ExistingNode.ID(), func(node *types.Node) { node.NodeKey = regData.NodeKey node.DiscoKey = regData.DiscoKey node.Hostname = params.Hostname @@ -2011,9 +2013,11 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView }) if !ok { - return types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID()) + return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID()) } + keysOnly := onlyKeysChanged(before, updatedNodeView, time.Now()) + // Persist to database. // Explicitly select all node columns so GORM includes nil/zero-value fields // (see nodeUpdateColumns comment). @@ -2039,7 +2043,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView return nil, nil //nolint:nilnil // side-effect only write }) if err != nil { - return types.NodeView{}, err + return types.NodeView{}, false, err } // Log completion @@ -2053,7 +2057,7 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView Msg("Node re-authorized") } - return updatedNodeView, nil + return updatedNodeView, keysOnly, nil } // createAndSaveNewNode creates a new node, allocates IPs, saves to DB, and adds to [NodeStore]. @@ -2460,12 +2464,15 @@ func (s *State) HandleNodeFromAuthPath( RegisterMethod: registrationMethod, } - var finalNode types.NodeView + var ( + finalNode types.NodeView + keysOnly bool + ) if nodeExistsForSameUser { updateParams.ExistingNode = existingNodeSameUser - finalNode, err = s.applyAuthNodeUpdate(updateParams) + finalNode, keysOnly, err = s.applyAuthNodeUpdate(updateParams) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } @@ -2473,7 +2480,7 @@ func (s *State) HandleNodeFromAuthPath( updateParams.ExistingNode = taggedNode updateParams.IsConvertFromTag = true - finalNode, err = s.applyAuthNodeUpdate(updateParams) + finalNode, _, err = s.applyAuthNodeUpdate(updateParams) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } @@ -2527,10 +2534,10 @@ func (s *State) HandleNodeFromAuthPath( policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - // nodeExistsForSameUser is true only for a same-user relogin; a tag->user - // conversion is excluded, as it changes the peer's User — a structural - // change peers must see in full, not a key-rotation patch. - return finalNode, reauthChange(finalNode, nodeExistsForSameUser, policyChanged), nil + // keysOnly is set only for a same-user relogin; a tag->user conversion is + // excluded, as it changes the peer's User — a structural change peers must + // see in full, not a key-rotation patch. + return finalNode, reauthChange(finalNode, keysOnly, policyChanged), nil } // createNewNodeFromAuth creates a new node during auth callback. @@ -2753,14 +2760,17 @@ func (s *State) HandleNodeFromPreAuthKey( Str(zf.UserName, pak.Username()). Msg("Registering node with pre-auth key") - var finalNode types.NodeView + var ( + finalNode types.NodeView + keysOnly bool + ) // If this node exists for this user, update the node in place. For a // tags-only key (pak.User == nil) this is true when the machine already has // 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() { - finalNode, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo) + finalNode, keysOnly, err = s.reregisterNodeWithPAK(existingNodeSameUser, pak, regReq, machineKey, hostname, validHostinfo) if err != nil { return types.NodeView{}, s.policyChangeSince(genBefore), err } @@ -2857,7 +2867,7 @@ func (s *State) HandleNodeFromPreAuthKey( policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - return finalNode, reauthChange(finalNode, existsSameUser, policyChanged), nil + return finalNode, reauthChange(finalNode, keysOnly, policyChanged), nil } // reregisterNodeWithPAK updates an existing node in place for a pre-auth key @@ -2868,7 +2878,8 @@ func (s *State) HandleNodeFromPreAuthKey( // 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. +// for its ID and for logging. The bool reports whether peers read the node it +// replaced and the update the same, apart from its keys; see [onlyKeysChanged]. func (s *State) reregisterNodeWithPAK( existingNodeSameUser types.NodeView, pak *types.PreAuthKey, @@ -2876,7 +2887,7 @@ func (s *State) reregisterNodeWithPAK( machineKey key.MachinePublic, hostname string, validHostinfo *tailcfg.Hostinfo, -) (types.NodeView, error) { +) (types.NodeView, bool, error) { log.Trace(). Caller(). Str(zf.NodeName, existingNodeSameUser.Hostname()). @@ -2894,7 +2905,7 @@ func (s *State) reregisterNodeWithPAK( // NodeStore NodeKey index, denying the victim service. if existing, ok := s.nodeStore.GetNodeByNodeKey(regReq.NodeKey); ok && existing.MachineKey() != machineKey { - return types.NodeView{}, ErrNodeKeyInUse + return types.NodeView{}, false, ErrNodeKeyInUse } // prior is the node the writer replaced: the NetInfo source and the @@ -2905,7 +2916,7 @@ func (s *State) reregisterNodeWithPAK( ) // Update existing node - NodeStore first, then database - updatedNodeView, ok := s.nodeStore.UpdateNode(existingNodeSameUser.ID(), func(node *types.Node) { + before, updatedNodeView, ok := s.nodeStore.UpdateNodeDiff(existingNodeSameUser.ID(), func(node *types.Node) { // One clock read, so the whole mutation is decided at one instant. now := time.Now() @@ -2998,7 +3009,7 @@ func (s *State) reregisterNodeWithPAK( // 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()) + return types.NodeView{}, false, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, existingNodeSameUser.ID()) } if authErr != nil { @@ -3010,7 +3021,7 @@ func (s *State) reregisterNodeWithPAK( Err(authErr). Msg("Re-registration needs a valid auth key when it applies, rejecting") - return types.NodeView{}, authErr + return types.NodeView{}, false, authErr } _, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { @@ -3058,7 +3069,7 @@ func (s *State) reregisterNodeWithPAK( n.AuthKeyID = prior.AuthKeyID }) - return types.NodeView{}, fmt.Errorf("writing node to database: %w", err) + return types.NodeView{}, false, fmt.Errorf("writing node to database: %w", err) } log.Trace(). @@ -3070,27 +3081,37 @@ func (s *State) reregisterNodeWithPAK( Str(zf.UserName, pak.Username()). Msg("Node re-authorized") - return updatedNodeView, nil + return updatedNodeView, onlyKeysChanged(before, updatedNodeView, time.Now()), nil } // reauthChange returns the [change.Change] to broadcast after an authentication -// that updated or created a node. +// that updated or created a node. keysOnly marks a same-user relogin that +// changed nothing peers read besides its keys; see [onlyKeysChanged]. // -// A pure relogin (isRelogin: an existing node, same user, with only its NodeKey -// rotated) is sent as a minimal incremental peer patch via [change.NodeKeyRotated] -// rather than re-advertising the whole node. A policy change forces a full -// recompute; any other (new) node is a whole-node add. -func reauthChange(node types.NodeView, isRelogin, policyChanged bool) change.Change { +// Such a relogin is sent as the minimal peer patch [change.NodeKeyRotated]. +// Anything else is a whole-node add, and a policy change forces a full +// recompute. +func reauthChange(node types.NodeView, keysOnly, policyChanged bool) change.Change { switch { case policyChanged: return change.PolicyChange() - case isRelogin: + case keysOnly: return change.NodeKeyRotated(node) default: return change.NodeAdded(node.ID()) } } +// onlyKeysChanged reports whether peers read before and after the same, apart +// from what [change.NodeKeyRotated] patches. [tailcfg.PeerChange] carries +// neither [tailcfg.Node.Expired] nor Hostinfo: peers left holding an expired +// node drop its WireGuard handshakes, and the relogin stored the Hostinfo, so +// no later MapRequest shows the change. +func onlyKeysChanged(before, after types.NodeView, now time.Time) bool { + return before.IsExpiredAt(now) == after.IsExpiredAt(now) && + peerHostinfoEqual(before.Hostinfo(), after.Hostinfo()) +} + // updatePolicyManagerUsers pushes the current user list into the policy // manager, rebuilds peer adjacency when user identity changed, and returns // a PolicyChange when clients need a refresh. diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index a4f35c368..3a1a307dc 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -504,12 +504,17 @@ func EndpointOrDERPUpdate(id types.NodeID, patch *tailcfg.PeerChange) Change { // incremental [tailcfg.PeerChange] patch rather than re-advertising the whole // node — the smallest update that conveys the rotation, and the least // disruptive for peers reconciling it. +// +// Use it only when nothing else peers read changed: [tailcfg.PeerChange] has +// no field for Hostinfo or for [tailcfg.Node.Expired], which only control can +// clear, and only by sending the whole node. func NodeKeyRotated(node types.NodeView) Change { nk := node.NodeKey() dk := node.DiscoKey() - // KeyExpiry is always set: the zero value clears any prior expiry on the - // peer (un-expire), and a non-zero value carries the new expiry. + // KeyExpiry is always set: the zero value clears any prior expiry + // timestamp on the peer, and a non-zero value carries the new expiry. It + // does not clear [tailcfg.Node.Expired]. var expiry time.Time if e, ok := node.Expiry().GetOk(); ok { expiry = e diff --git a/integration/auth_web_flow_test.go b/integration/auth_web_flow_test.go index 26c23d06e..839031ca6 100644 --- a/integration/auth_web_flow_test.go +++ b/integration/auth_web_flow_test.go @@ -13,6 +13,7 @@ import ( "github.com/samber/lo" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "tailscale.com/types/key" ) func TestAuthWebFlowAuthenticationPingAll(t *testing.T) { @@ -197,6 +198,181 @@ func TestAuthWebFlowLogoutAndReloginSameUser(t *testing.T) { t.Logf("all clients IPs are the same") } +// peerAPISettleTimeout bounds the wait for an expired client to serve +// peerapi again; clients that do take about a second. +const peerAPISettleTimeout = 10 * time.Second + +// TestAuthWebFlowReloginExpiredNode expires one node at a time and logs it +// back in through the web flow, which rotates its node key. Peers hold the +// expired node with Expired=true, so the relogin must clear that flag on every +// peer that stayed connected. Otherwise the peers keep dropping the node's +// WireGuard handshakes while disco pings still succeed, which is why +// reachability is checked with TSMP pings rather than disco pings. +func TestAuthWebFlowReloginExpiredNode(t *testing.T) { + IntegrationSkip(t) + + spec := ScenarioSpec{ + NodesPerUser: len(MustTestVersions), + Users: []string{"user1"}, + } + + scenario, err := NewScenario(spec) + + require.NoError(t, err) + defer scenario.ShutdownAssertNoPanics(t) + + err = scenario.CreateHeadscaleEnvWithLoginURL( + nil, + hsic.WithTestName("webexpiredrelogin"), + ) + requireNoErrHeadscaleEnv(t, err) + + allClients, err := scenario.ListTailscaleClients() + requireNoErrListClients(t, err) + + allIps, err := scenario.ListTailscaleClientsIPs() + requireNoErrListClientIPs(t, err) + + err = scenario.WaitForTailscaleSync() + requireNoErrSync(t, err) + + allAddrs := lo.Map(allIps, func(x netip.Addr, index int) string { + return x.String() + }) + + assertPingAll(t, allClients, allAddrs) + + headscale, err := scenario.Headscale() + requireNoErrGetHeadscale(t, err) + + for _, target := range allClients { + t.Run(target.Hostname(), func(t *testing.T) { + var ( + selfID string + oldKey key.NodePublic + ) + + require.EventuallyWithT(t, func(c *assert.CollectT) { + status, err := target.Status() + if !assert.NoError(c, err) { + return + } + + selfID, oldKey = string(status.Self.ID), status.Self.PublicKey + }, integrationutil.StatusReadyTimeout, integrationutil.FastPoll, "client must report its own status before expiry") + + targetIP := target.MustIPv4().String() + + _, err := headscale.Execute([]string{ + "headscale", "nodes", "expire", "--identifier", selfID, + }) + require.NoError(t, err) + + for _, peer := range allClients { + if peer.Hostname() == target.Hostname() { + continue + } + + require.EventuallyWithT(t, func(c *assert.CollectT) { + status, err := peer.Status() + if !assert.NoError(c, err) { + return + } + + expired, found := status.Peer[oldKey] + if assert.True(c, found, "expired node must remain visible") { + assert.True(c, expired.Expired) + } + }, integrationutil.StatusReadyTimeout, integrationutil.FastPoll, "peer must see the node expired") + } + + require.EventuallyWithT(t, func(c *assert.CollectT) { + status, err := target.Status() + if !assert.NoError(c, err) { + return + } + + assert.Equal(c, "NeedsLogin", status.BackendState) + }, integrationutil.StatusReadyTimeout, integrationutil.FastPoll, "expired client must wait for login") + + // Some clients serve peerapi again while they wait for login. + // Relogging once they do makes them register with the Hostinfo + // they later report as running, so no Hostinfo change can carry + // the whole node and the relogin itself must clear the expired + // flag. Which clients do is up to the client, so it is observed + // rather than assumed; the others relog with changed Hostinfo. + settled := false + tick := time.NewTicker(integrationutil.FastPoll) + timeout := time.After(peerAPISettleTimeout) + + settle: + for { + status, err := target.Status() + if err == nil && len(status.Self.PeerAPIURL) > 0 { + settled = true + + break + } + + select { + case <-tick.C: + case <-timeout: + break settle + } + } + + tick.Stop() + + t.Logf("%s served peerapi while expired: %v", target.Version(), settled) + + loginURL, err := target.LoginWithURL(headscale.GetEndpoint()) + require.NoError(t, err) + body, err := doLoginURL(target.Hostname(), loginURL) + require.NoError(t, err) + require.NoError(t, scenario.runHeadscaleRegister("user1", body)) + require.NoError(t, target.WaitForRunning(integrationutil.PeerSyncTimeout())) + + var newKey key.NodePublic + + require.EventuallyWithT(t, func(c *assert.CollectT) { + status, err := target.Status() + if !assert.NoError(c, err) { + return + } + + newKey = status.Self.PublicKey + assert.NotEqual(c, oldKey, newKey, "relogin of an expired node must rotate its node key") + }, integrationutil.StatusReadyTimeout, integrationutil.FastPoll, "client must report its new node key") + + for _, peer := range allClients { + if peer.Hostname() == target.Hostname() { + continue + } + + require.EventuallyWithT(t, func(c *assert.CollectT) { + status, err := peer.Status() + if !assert.NoError(c, err) { + return + } + + relogged, found := status.Peer[newKey] + if assert.True(c, found, "peer must know the new node key") { + assert.False(c, relogged.Expired, "peer must clear the expired flag") + } + + stdout, _, err := peer.Execute([]string{ + "tailscale", "ping", "--tsmp", "--c=1", "--timeout=2s", targetIP, + }) + assert.NoError(c, err) + assert.Contains(c, stdout, "pong") + }, integrationutil.StatusReadyTimeout, integrationutil.FastPoll, "peer must reach the relogged node over WireGuard") + } + }) + } + + assertPingAll(t, allClients, allAddrs) +} + // TestAuthWebFlowLogoutAndReloginNewUser tests the scenario where multiple Tailscale clients // initially authenticate using the web-based authentication flow (where users visit a URL // in their browser to authenticate), then all clients log out and log back in as a different user.