From a5ef3aff15ab559e06f1553dcbe653a7b1e2d4f4 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 15 Jun 2026 08:18:59 +0000 Subject: [PATCH] state: patch relogins and gate endpoint broadcasts Relogin sent as a peer patch with endpoints preserved; endpoint-only deltas broadcast only on useful (non-STUN) changes. --- hscontrol/state/endpoint_test.go | 85 +++++++++++++++++++++++ hscontrol/state/persist_test.go | 90 ++++++++++++++++++++++++ hscontrol/state/state.go | 114 ++++++++++++++++++++++++++----- 3 files changed, 272 insertions(+), 17 deletions(-) diff --git a/hscontrol/state/endpoint_test.go b/hscontrol/state/endpoint_test.go index b8905ab7..21b83e5b 100644 --- a/hscontrol/state/endpoint_test.go +++ b/hscontrol/state/endpoint_test.go @@ -111,3 +111,88 @@ func TestEndpointStorageInNodeStore(t *testing.T) { } } } + +// TestEndpointBroadcastWorthy verifies the gate that decides whether an +// endpoint-only delta is worth fanning out to peers as an incremental +// PeersChangedPatch. A delta that only adds STUN-derived endpoints (or only +// removes endpoints) is suppressed: it is churny and unlikely to be useful, +// and disco's callMeMaybe re-derives STUN paths anyway. Only deltas that +// introduce a genuinely useful (non-STUN) endpoint are broadcast-worthy. +func TestEndpointBroadcastWorthy(t *testing.T) { + local := netip.MustParseAddrPort("192.168.1.5:41641") + local2 := netip.MustParseAddrPort("192.168.1.6:41641") + stun := netip.MustParseAddrPort("203.0.113.7:41641") + stun2 := netip.MustParseAddrPort("203.0.113.8:41641") + portmap := netip.MustParseAddrPort("198.51.100.9:41641") + + tests := []struct { + name string + stored []netip.AddrPort + newEPs []netip.AddrPort + newType []tailcfg.EndpointType + want bool + }{ + { + name: "adds only a STUN endpoint - suppress", + stored: []netip.AddrPort{local}, + newEPs: []netip.AddrPort{local, stun}, + newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN}, + want: false, + }, + { + name: "adds only STUN4LocalPort - suppress", + stored: []netip.AddrPort{local}, + newEPs: []netip.AddrPort{local, stun}, + newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN4LocalPort}, + want: false, + }, + { + name: "adds a useful local endpoint - broadcast", + stored: []netip.AddrPort{stun}, + newEPs: []netip.AddrPort{stun, local}, + newType: []tailcfg.EndpointType{tailcfg.EndpointSTUN, tailcfg.EndpointLocal}, + want: true, + }, + { + name: "adds a useful portmapped endpoint - broadcast", + stored: []netip.AddrPort{local}, + newEPs: []netip.AddrPort{local, portmap}, + newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointPortmapped}, + want: true, + }, + { + name: "pure shrink, no additions - suppress", + stored: []netip.AddrPort{local, local2}, + newEPs: []netip.AddrPort{local}, + newType: []tailcfg.EndpointType{tailcfg.EndpointLocal}, + want: false, + }, + { + name: "nil types (older client) adding endpoint - broadcast", + stored: []netip.AddrPort{local}, + newEPs: []netip.AddrPort{local, local2}, + want: true, + }, + { + name: "only STUN endpoints churn (replace one STUN with another) - suppress", + stored: []netip.AddrPort{local, stun}, + newEPs: []netip.AddrPort{local, stun2}, + newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN}, + want: false, + }, + { + name: "mixed add: one STUN and one useful - broadcast", + stored: []netip.AddrPort{local}, + newEPs: []netip.AddrPort{local, stun, local2}, + newType: []tailcfg.EndpointType{tailcfg.EndpointLocal, tailcfg.EndpointSTUN, tailcfg.EndpointLocal}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := endpointBroadcastWorthy(tt.stored, tt.newEPs, tt.newType) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 0adac6d5..ab35d9a4 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -325,3 +325,93 @@ func TestReauthRejectsNodeKeyClaimedByAnotherMachine(t *testing.T) { require.Error(t, err, "re-auth claiming a NodeKey bound to another machine must be rejected") } + +// TestReauthPreservesEndpointsWhenClientOmitsThem proves the re-auth/update +// path keeps a node's live WireGuard endpoints when the originating +// RegisterRequest carried none. Web/OIDC relogins report endpoints via +// MapRequest, not register, so RegData.Endpoints is empty; wiping the stored +// endpoints would advertise the re-keyed node to peers endpoint-less, which +// drives head/unstable tailscale clients into one-way disco-deafness. +func TestReauthPreservesEndpointsWhenClientOmitsThem(t *testing.T) { + dbPath := t.TempDir() + "/headscale.db" + cfg := persistTestConfig(dbPath) + + database, err := db.NewHeadscaleDatabase(cfg) + require.NoError(t, err) + + user := database.CreateUserForTest("user") + require.NoError(t, database.Close()) + + s, err := NewState(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = s.Close() }) + + machine := key.NewMachine() + endpoints := []netip.AddrPort{ + netip.MustParseAddrPort("192.168.1.5:41641"), + netip.MustParseAddrPort("10.0.0.5:41641"), + } + + // Node is registered and has reported live endpoints (as after its first + // MapRequest). + node, err := s.createAndSaveNewNode(newNodeParams{ + User: *user, + MachineKey: machine.Public(), + NodeKey: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + Hostname: "node", + Endpoints: endpoints, + RegisterMethod: util.RegisterMethodCLI, + }) + require.NoError(t, err) + require.Equal(t, endpoints, node.Endpoints().AsSlice(), + "precondition: node has live endpoints") + + // Node re-authenticates, rotating its NodeKey. The RegisterRequest carries + // no endpoints. + updated, err := s.applyAuthNodeUpdate(authNodeUpdateParams{ + ExistingNode: node, + RegData: &types.RegistrationData{ + MachineKey: machine.Public(), + NodeKey: key.NewNode().Public(), + Hostname: "node", + Hostinfo: &tailcfg.Hostinfo{}, + Endpoints: nil, + }, + ValidHostinfo: &tailcfg.Hostinfo{}, + Hostname: "node", + User: user, + RegisterMethod: util.RegisterMethodCLI, + }) + require.NoError(t, err) + + assert.Equal(t, endpoints, updated.Endpoints().AsSlice(), + "re-auth without reported endpoints must preserve the node's live endpoints") +} + +// TestReauthChange covers the decision both re-auth paths share: a same-user +// 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 +// recompute; a new node is a whole-node add. +func TestReauthChange(t *testing.T) { + n := types.Node{ + ID: 7, + NodeKey: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + } + node := n.View() + + relogin := reauthChange(node, true, false) + assert.Len(t, relogin.PeerPatches, 1, "relogin must be a peer patch") + assert.Empty(t, relogin.PeersChanged, "relogin must not be a whole-node add") + + added := reauthChange(node, false, false) + assert.Empty(t, added.PeerPatches) + assert.Len(t, added.PeersChanged, 1, "a new node must be a whole-node add") + + pol := reauthChange(node, true, true) + assert.Empty(t, pol.PeerPatches, "a policy change must not be a peer patch") + assert.Empty(t, pol.PeersChanged) + assert.False(t, pol.IsEmpty(), "a policy change must be non-empty") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index f7cfd686..d7ffc83d 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -1615,7 +1615,14 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView params.ValidHostinfo, ) - node.Endpoints = regData.Endpoints + // Preserve the node's live endpoints when the register request carried + // none. Web/OIDC relogins report endpoints via MapRequest, not register, + // so RegData.Endpoints is empty; clearing the stored set would advertise + // the re-keyed node with no way for peers to reach it. The first + // MapRequest restores the live set. + if len(regData.Endpoints) > 0 { + node.Endpoints = regData.Endpoints + } // Do NOT reset IsOnline here. Online status is managed exclusively by // [State.Connect]/[State.Disconnect] in the poll session lifecycle. // Resetting it during re-registration causes a false offline blip: the @@ -2115,14 +2122,12 @@ func (s *State) HandleNodeFromAuthPath( return finalNode, change.NodeAdded(finalNode.ID()), fmt.Errorf("updating policy manager nodes: %w", err) } - var c change.Change - if !usersChange.IsEmpty() || !nodesChange.IsEmpty() { - c = change.PolicyChange() - } else { - c = change.NodeAdded(finalNode.ID()) - } + policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - return finalNode, c, nil + // 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 } // createNewNodeFromAuth creates a new node during auth callback. @@ -2448,14 +2453,27 @@ func (s *State) HandleNodeFromPreAuthKey( return finalNode, change.NodeAdded(finalNode.ID()), fmt.Errorf("updating policy manager nodes: %w", err) } - var c change.Change - if !usersChange.IsEmpty() || !nodesChange.IsEmpty() { - c = change.PolicyChange() - } else { - c = change.NodeAdded(finalNode.ID()) - } + policyChanged := !usersChange.IsEmpty() || !nodesChange.IsEmpty() - return finalNode, c, nil + return finalNode, reauthChange(finalNode, existsSameUser, policyChanged), nil +} + +// reauthChange returns the [change.Change] to broadcast after an authentication +// that updated or created a node. +// +// 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 { + switch { + case policyChanged: + return change.PolicyChange() + case isRelogin: + return change.NodeKeyRotated(node) + default: + return change.NodeAdded(node.ID()) + } } // updatePolicyManagerUsers updates the policy manager with current users. @@ -2656,8 +2674,13 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest updatedNode, ok := s.nodeStore.UpdateNode(id, func(currentNode *types.Node) { peerChange := currentNode.PeerChangeFromMapRequest(req) - // Track what specifically changed - endpointChanged = peerChange.Endpoints != nil + // Track what specifically changed. An endpoint delta is only + // broadcast-worthy when it adds a useful (non-STUN) endpoint; + // STUN-only churn and pure shrinks are suppressed to reduce peer + // churn (see endpointBroadcastWorthy). The new set is still stored + // via ApplyPeerChange below regardless of this decision. + endpointChanged = peerChange.Endpoints != nil && + endpointBroadcastWorthy(currentNode.Endpoints, req.Endpoints, req.EndpointTypes) derpChanged = peerChange.DERPRegion != 0 hostinfoChanged = !hostinfoEqual(currentNode.View(), req.Hostinfo) @@ -2841,6 +2864,63 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest return buildMapRequestChangeResponse(id, updatedNode, hostinfoChanged, endpointChanged, derpChanged) } +// endpointBroadcastWorthy reports whether an endpoint-only delta is worth +// fanning out to peers as an incremental PeersChangedPatch. A delta that only +// adds STUN-derived endpoints — or only removes endpoints — is suppressed: +// bare STUN endpoints are unlikely to be open and churn a lot (the client +// re-derives those paths over disco anyway), and a pure shrink is not worth +// telling peers about. Suppressing this churn keeps peers' views stable. +// +// The decision is intentionally conservative: it gates the broadcast only, +// not storage. The node's full endpoint set (STUN included) is still stored +// and rides along the next substantive change or full MapResponse, so no +// reachable path is permanently hidden from peers. +// +// Limitation: headscale stores bare []netip.AddrPort with no per-endpoint +// type, so we can only classify the *new* request's endpoints (via the +// parallel newTypes slice). We therefore gate on whether any newly-added +// endpoint (present in new, absent from stored) is useful (non-STUN). When +// newTypes is absent or shorter than newEPs (older clients), the unknown +// endpoints are treated as useful, preserving the pre-existing always-broadcast +// behaviour and never hiding a genuinely new endpoint. +func endpointBroadcastWorthy( + stored, newEPs []netip.AddrPort, + newTypes []tailcfg.EndpointType, +) bool { + storedSet := make(map[netip.AddrPort]struct{}, len(stored)) + for _, ep := range stored { + storedSet[ep] = struct{}{} + } + + for i, ep := range newEPs { + if _, ok := storedSet[ep]; ok { + // Already known to peers; not a newly-added endpoint. + continue + } + + // A newly-added endpoint with no type information (older client) + // is treated as useful so we never hide a genuinely new endpoint. + t := tailcfg.EndpointUnknownType + if i < len(newTypes) { + t = newTypes[i] + } + + if isUsefulEndpointType(t) { + return true + } + } + + return false +} + +// isUsefulEndpointType reports whether an endpoint type is worth eagerly +// broadcasting to peers. STUN-derived endpoints are excluded because they are +// churny and unlikely to be directly reachable; magicsock's disco handles +// establishing those paths. +func isUsefulEndpointType(t tailcfg.EndpointType) bool { + return t != tailcfg.EndpointSTUN && t != tailcfg.EndpointSTUN4LocalPort +} + // buildMapRequestChangeResponse determines the appropriate response type for a [tailcfg.MapRequest] update. // Hostinfo changes require a full update, while endpoint/DERP changes can use lightweight patches. func buildMapRequestChangeResponse(