diff --git a/hscontrol/auth_test.go b/hscontrol/auth_test.go index 4ed95c89a..bbf0d2ba2 100644 --- a/hscontrol/auth_test.go +++ b/hscontrol/auth_test.go @@ -4108,6 +4108,76 @@ 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) + // Direct fixture writes bypass registration's policy-cache updates. + require.NoError(t, app.state.UpdatePolicyManagerUsersForTest()) + require.NoError(t, app.state.UpdatePolicyManagerNodesForTest()) + + 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/maprequest.go b/hscontrol/state/maprequest.go index 2035ab4e6..ea612c8eb 100644 --- a/hscontrol/state/maprequest.go +++ b/hscontrol/state/maprequest.go @@ -10,6 +10,7 @@ import ( "github.com/rs/zerolog" "github.com/rs/zerolog/log" "tailscale.com/tailcfg" + "tailscale.com/types/views" ) // mapRequestDelta carries the classified facts extracted from one MapRequest @@ -154,30 +155,56 @@ func hostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool { return oldCopy.Equal(&newCopy) } -// peerHostinfo keeps the Hostinfo fields another node's client reads from -// a peer: what tailscale status shows, PeerAPI services, SSH known hosts, -// exit-node location, app-connector eligibility, and the routes this server -// feeds into policy. Everything else is stored but never fanned out, and a -// peer's NetInfo is never read at all. -func peerHostinfo(hi *tailcfg.Hostinfo) *tailcfg.Hostinfo { - if hi == nil { - return nil +// peerHostinfoEqual reports whether a and b agree on the Hostinfo fields +// another node's client reads from a peer: what tailscale status shows, PeerAPI +// services, SSH known hosts, exit-node location, app-connector eligibility, and +// the routes this server feeds into policy. Everything else is stored but never +// fanned out, and a peer's NetInfo is never read at all. +func peerHostinfoEqual(a, b tailcfg.HostinfoView) bool { + if !a.Valid() || !b.Valid() { + return a.Valid() == b.Valid() } - return &tailcfg.Hostinfo{ - Hostname: hi.Hostname, - OS: hi.OS, - Services: hi.Services, - SSH_HostKeys: hi.SSH_HostKeys, - Location: hi.Location, - AppConnector: hi.AppConnector, - RoutableIPs: hi.RoutableIPs, - } + return a.Hostname() == b.Hostname() && + a.OS() == b.OS() && + servicesEqual(a.Services(), b.Services()) && + views.SliceEqual(a.SSH_HostKeys(), b.SSH_HostKeys()) && + locationEqual(a.Location(), b.Location()) && + a.AppConnector() == b.AppConnector() && + views.SliceEqual(a.RoutableIPs(), b.RoutableIPs()) } -// peerHostinfoEqual reports whether the fields peers read are unchanged. -func peerHostinfoEqual(oldHI, newHI *tailcfg.Hostinfo) bool { - return peerHostinfo(oldHI).Equal(peerHostinfo(newHI)) +// servicesEqual compares field by field, as [tailcfg.Service] is +// incomparable. +func servicesEqual(a, b views.Slice[tailcfg.Service]) bool { + if a.Len() != b.Len() { + return false + } + + for i := range a.Len() { + x, y := a.At(i), b.At(i) + if x.Proto != y.Proto || x.Port != y.Port || x.Description != y.Description { + return false + } + } + + return true +} + +// locationEqual compares field by field, as [tailcfg.LocationView] has no +// Equal. +func locationEqual(a, b tailcfg.LocationView) bool { + if !a.Valid() || !b.Valid() { + return a.Valid() == b.Valid() + } + + return a.Country() == b.Country() && + a.CountryCode() == b.CountryCode() && + a.City() == b.City() && + a.CityCode() == b.CityCode() && + a.Latitude() == b.Latitude() && + a.Longitude() == b.Longitude() && + a.Priority() == b.Priority() } // netInfoEqualIgnoringDERP compares two NetInfo values via diff --git a/hscontrol/state/maprequest_test.go b/hscontrol/state/maprequest_test.go index 00c3a1675..336903264 100644 --- a/hscontrol/state/maprequest_test.go +++ b/hscontrol/state/maprequest_test.go @@ -2,6 +2,7 @@ package state import ( "net/netip" + "reflect" "strings" "sync" "sync/atomic" @@ -858,12 +859,60 @@ func TestPeerHostinfoEqual(t *testing.T) { t.Run(tt.name, func(t *testing.T) { changed := base() tt.mutate(changed) - require.Equal(t, tt.want, peerHostinfoEqual(base(), changed)) + require.Equal(t, tt.want, peerHostinfoEqual(base().View(), changed.View())) }) } - require.True(t, peerHostinfoEqual(nil, nil)) - require.False(t, peerHostinfoEqual(nil, base())) + require.True(t, peerHostinfoEqual(tailcfg.HostinfoView{}, tailcfg.HostinfoView{})) + require.False(t, peerHostinfoEqual(tailcfg.HostinfoView{}, base().View())) +} + +// TestPeerHostinfoEqualSeesEveryField fails when tailscale adds a field to +// [tailcfg.Service] or [tailcfg.Location] that servicesEqual or locationEqual +// does not compare. +func TestPeerHostinfoEqualSeesEveryField(t *testing.T) { + set := func(t *testing.T, f reflect.Value) { + t.Helper() + + switch { + case f.Kind() == reflect.String: + f.SetString("x") + case f.CanInt(): + f.SetInt(1) + case f.CanUint(): + f.SetUint(1) + case f.CanFloat(): + f.SetFloat(1) + default: + t.Fatalf("field kind %s not handled; compare it", f.Kind()) + } + } + + for _, field := range reflect.VisibleFields(reflect.TypeFor[tailcfg.Service]()) { + if !field.IsExported() { + continue + } + + t.Run("Service."+field.Name, func(t *testing.T) { + var svc tailcfg.Service + set(t, reflect.ValueOf(&svc).Elem().FieldByIndex(field.Index)) + + a := &tailcfg.Hostinfo{Services: []tailcfg.Service{{}}} + b := &tailcfg.Hostinfo{Services: []tailcfg.Service{svc}} + assert.False(t, peerHostinfoEqual(a.View(), b.View())) + }) + } + + for _, field := range reflect.VisibleFields(reflect.TypeFor[tailcfg.Location]()) { + t.Run("Location."+field.Name, func(t *testing.T) { + var loc tailcfg.Location + set(t, reflect.ValueOf(&loc).Elem().FieldByIndex(field.Index)) + + a := &tailcfg.Hostinfo{Location: &tailcfg.Location{}} + b := &tailcfg.Hostinfo{Location: &loc} + assert.False(t, peerHostinfoEqual(a.View(), b.View())) + }) + } } func TestNetInfoEqualIgnoringDERP(t *testing.T) { diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index a216c969c..5d9bd6d5e 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -198,6 +198,9 @@ type work struct { // prober applying multiple probe results at once) cannot have a // partial snapshot published between the updates. multiUpdates map[types.NodeID]UpdateNodeFunc + // pre, when non-nil, receives each updated node as it was before its + // update function ran. + pre map[types.NodeID]types.NodeView } // updateChanges reports whether an in-place update moved a peer-visibility @@ -277,6 +280,27 @@ func (s *NodeStore) UpdateNode(nodeID types.NodeID, updateFn UpdateNodeFunc) (ty return s.GetNode(nodeID) } +// UpdateNodeDiff is [NodeStore.UpdateNode] that also returns, first, the node +// as it was before updateFn ran, so callers can tell what the update changed +// without cloning the node themselves. That view is invalid when the node does +// not exist. +func (s *NodeStore) UpdateNodeDiff( + nodeID types.NodeID, + updateFn UpdateNodeFunc, +) (types.NodeView, types.NodeView, bool) { + timer := prometheus.NewTimer(nodeStoreOperationDuration.WithLabelValues("update")) + defer timer.ObserveDuration() + + pre := make(map[types.NodeID]types.NodeView, 1) + s.updateNodes(map[types.NodeID]UpdateNodeFunc{nodeID: updateFn}, pre) + + nodeStoreOperations.WithLabelValues("update").Inc() + + after, ok := s.GetNode(nodeID) + + return pre[nodeID], after, ok +} + // UpdateNodes applies per-node update functions in a single atomic // batch. The election that recomputes primary routes runs once, after // every update has landed, so callers cannot observe an intermediate @@ -285,6 +309,15 @@ func (s *NodeStore) UpdateNode(nodeID types.NodeID, updateFn UpdateNodeFunc) (ty // would change the election outcome — e.g. the HA prober applying // concurrent probe-timeout results. func (s *NodeStore) UpdateNodes(updates map[types.NodeID]UpdateNodeFunc) { + s.updateNodes(updates, nil) +} + +// updateNodes queues updates as one batch entry and waits for it, filling pre +// when it is non-nil. +func (s *NodeStore) updateNodes( + updates map[types.NodeID]UpdateNodeFunc, + pre map[types.NodeID]types.NodeView, +) { timer := prometheus.NewTimer(nodeStoreOperationDuration.WithLabelValues("update_multi")) defer timer.ObserveDuration() @@ -295,6 +328,7 @@ func (s *NodeStore) UpdateNodes(updates map[types.NodeID]UpdateNodeFunc) { w := work{ op: updateMulti, multiUpdates: updates, + pre: pre, result: make(chan struct{}), } @@ -510,6 +544,10 @@ func (s *NodeStore) applyBatch(batch []work) { nodes[id] = n + if w.pre != nil { + w.pre[id] = pre.View() + } + relation, election := updateChanges(pre, &n) relationChanged = relationChanged || relation electionChanged = electionChanged || election diff --git a/hscontrol/state/node_store_test.go b/hscontrol/state/node_store_test.go index 12945eb72..1a1c77e37 100644 --- a/hscontrol/state/node_store_test.go +++ b/hscontrol/state/node_store_test.go @@ -1667,6 +1667,28 @@ func TestUpdateNodeRecomputesPeersOnlyForRelationInputs(t *testing.T) { } } +// TestUpdateNodeDiff proves the update returns the node the writer replaced +// alongside the result, and an invalid before for a missing node. +func TestUpdateNodeDiff(t *testing.T) { + node := createTestNode(1, 1, "user1", "node1") + oldKey := node.NodeKey + + store := NewNodeStore(types.Nodes{&node}, allowAllPeersFunc, TestBatchSize, TestBatchTimeout) + store.Start() + + defer store.Stop() + + newKey := key.NewNode().Public() + before, after, ok := store.UpdateNodeDiff(1, func(n *types.Node) { n.NodeKey = newKey }) + require.True(t, ok) + assert.Equal(t, oldKey, before.NodeKey()) + assert.Equal(t, newKey, after.NodeKey()) + + before, _, ok = store.UpdateNodeDiff(99, func(*types.Node) {}) + assert.False(t, ok) + assert.False(t, before.Valid()) +} + // TestListPeersExcludesSelf proves a node is never returned among its own // peers, on both the snapshot path and the explicit peer-ID path. // diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index d726d2240..c76baccf3 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -315,7 +315,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(), @@ -375,7 +375,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(), @@ -395,7 +395,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 @@ -422,6 +422,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 bfd9472e1..4e416c785 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1679,8 +1679,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 { @@ -1721,7 +1723,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, @@ -1736,11 +1738,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 @@ -1831,9 +1833,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). @@ -1859,7 +1863,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 @@ -1873,7 +1877,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]. @@ -2277,12 +2281,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{}, change.Change{}, err } @@ -2290,7 +2297,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{}, change.Change{}, err } @@ -2344,10 +2351,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. @@ -2566,7 +2573,10 @@ func (s *State) HandleNodeFromPreAuthKey( Str(zf.UserName, pakUsername()). 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 @@ -2600,7 +2610,7 @@ func (s *State) HandleNodeFromPreAuthKey( priorNode := existingNodeSameUser.AsStruct() // 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) { node.NodeKey = regReq.NodeKey node.Hostname = hostname @@ -2730,6 +2740,7 @@ func (s *State) HandleNodeFromPreAuthKey( Msg("Node re-authorized") finalNode = updatedNodeView + keysOnly = onlyKeysChanged(before, updatedNodeView, time.Now()) } else { // Node does not exist for this user with this machine key. // For a user-owned key, check whether the machine key is already held @@ -2823,27 +2834,37 @@ func (s *State) HandleNodeFromPreAuthKey( policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - return finalNode, reauthChange(finalNode, existsSameUser, policyChanged), nil + return finalNode, reauthChange(finalNode, keysOnly, policyChanged), 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. @@ -3120,7 +3141,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest delta.hostinfoChanged = newHostinfo != nil && !hostinfoEqual(currentNode.Hostinfo, newHostinfo) delta.peerHostinfoChanged = newHostinfo != nil && - !peerHostinfoEqual(currentNode.Hostinfo, newHostinfo) + !peerHostinfoEqual(currentNode.Hostinfo.View(), newHostinfo.View()) delta.dnsMetadataChanged = newHostinfo != nil && (currentNode.Hostinfo == nil || currentNode.Hostinfo.Hostname != newHostinfo.Hostname || diff --git a/hscontrol/types/change/change.go b/hscontrol/types/change/change.go index 8737f5012..8fa72e7ec 100644 --- a/hscontrol/types/change/change.go +++ b/hscontrol/types/change/change.go @@ -508,12 +508,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/hscontrol/types/node.go b/hscontrol/types/node.go index 3d85d2a30..4d65e4bdf 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -214,6 +214,11 @@ func (ns Nodes) ViewSlice() views.Slice[NodeView] { // IsExpired returns whether the node registration has expired. func (node *Node) IsExpired() bool { + return node.IsExpiredAt(time.Now()) +} + +// IsExpiredAt reports whether the node registration has expired at now. +func (node *Node) IsExpiredAt(now time.Time) bool { // If Expiry is not set, the client has not indicated that // it wants an expiry time, it is therefore considered // to mean "not expired" @@ -221,7 +226,7 @@ func (node *Node) IsExpired() bool { return false } - return time.Since(*node.Expiry) > 0 + return now.After(*node.Expiry) } // Online reports the node's last known connectivity. Unknown counts as @@ -1016,6 +1021,15 @@ func (nv NodeView) IsExpired() bool { return nv.ж.IsExpired() } +// IsExpiredAt reports whether the node registration has expired at now. +func (nv NodeView) IsExpiredAt(now time.Time) bool { + if !nv.Valid() { + return true + } + + return nv.ж.IsExpiredAt(now) +} + // Online reports the node's last known connectivity. func (nv NodeView) Online() bool { if !nv.Valid() { diff --git a/hscontrol/types/node_test.go b/hscontrol/types/node_test.go index ee2950cee..9648ec31a 100644 --- a/hscontrol/types/node_test.go +++ b/hscontrol/types/node_test.go @@ -1095,3 +1095,33 @@ func TestHasPolicyChangeFields(t *testing.T) { }) } } + +func TestNodeIsExpiredAt(t *testing.T) { + now := time.Now() + zero := time.Time{} + justBefore := now.Add(-time.Nanosecond) + + tests := []struct { + name string + expiry *time.Time + want bool + }{ + {name: "nil expiry", expiry: nil, want: false}, + {name: "zero expiry", expiry: &zero, want: false}, + {name: "expiry equals now", expiry: &now, want: false}, + {name: "now one nanosecond after expiry", expiry: &justBefore, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + n := &Node{Expiry: tt.expiry} + require.Equal(t, tt.want, n.IsExpiredAt(now)) + require.Equal(t, tt.want, n.View().IsExpiredAt(now)) + }) + } + + t.Run("invalid view", func(t *testing.T) { + require.True(t, NodeView{}.IsExpiredAt(now), + "an invalid view counts as expired, like IsExpired") + }) +} diff --git a/integration/auth_web_flow_test.go b/integration/auth_web_flow_test.go index 752d78a31..a30332591 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.