From 3cf284917db473553dde10eb1ade1631c36a4323 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 08:46:46 +0000 Subject: [PATCH] state: apply map requests only under the node key that authorised them --- hscontrol/poll_test.go | 8 ++++ hscontrol/state/maprequest.go | 6 +-- hscontrol/state/maprequest_test.go | 68 +++++++++++++++++++++++++++++- hscontrol/state/state.go | 19 +++++++-- hscontrol/types/node.go | 9 +--- 5 files changed, 93 insertions(+), 17 deletions(-) diff --git a/hscontrol/poll_test.go b/hscontrol/poll_test.go index e4d6df187..30dd85d7f 100644 --- a/hscontrol/poll_test.go +++ b/hscontrol/poll_test.go @@ -264,6 +264,7 @@ func TestServeLongPollWritesErrorWhenInitialMapFails(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) writer := &recordingResponseWriter{} session := app.newMapSession(ctx, tailcfg.MapRequest{ + NodeKey: node.NodeKey, Stream: true, Version: tailcfg.CapabilityVersion(100), }, writer, node.View()) @@ -345,6 +346,7 @@ func TestFailedReconnectDoesNotCancelEphemeralGC(t *testing.T) { writer := &recordingResponseWriter{} session := app.newMapSession(context.Background(), tailcfg.MapRequest{ + NodeKey: node.NodeKey, Stream: true, Version: tailcfg.CapabilityVersion(100), }, writer, node.View()) @@ -394,6 +396,7 @@ func TestEphemeralGCDoesNotDeleteReconnectedNode(t *testing.T) { node := nodeView.AsStruct() session := app.newMapSession(context.Background(), tailcfg.MapRequest{ + NodeKey: node.NodeKey, Stream: true, Version: tailcfg.CapabilityVersion(100), }, &recordingResponseWriter{}, node.View()) @@ -540,6 +543,7 @@ func TestGitHubIssue3129_BlockedWriteSessionLifecycle(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) writer := newDelayedSuccessResponseWriter(tt.writeDelay) session := app.newMapSession(ctx, tailcfg.MapRequest{ + NodeKey: node.NodeKey, Stream: true, Version: tailcfg.CapabilityVersion(100), }, writer, node.View()) @@ -673,6 +677,7 @@ func TestDeletedNodeEndsLongPoll(t *testing.T) { writer := newDelayedSuccessResponseWriter(0) session := app.newMapSession(ctx, tailcfg.MapRequest{ + NodeKey: node.NodeKey, Stream: true, Version: tailcfg.CapabilityVersion(100), }, writer, node.View()) @@ -762,6 +767,7 @@ func TestDeletedNodeEndsStreamCleanly(t *testing.T) { served := make(chan struct{}) srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { session := app.newMapSession(r.Context(), tailcfg.MapRequest{ + NodeKey: nodeView.NodeKey(), Stream: true, Version: tailcfg.CapabilityVersion(100), }, w, nodeView) @@ -856,6 +862,7 @@ func TestLongPollShutdownWaitsForCleanup(t *testing.T) { ctx, cancel := context.WithCancel(t.Context()) writer := newDelayedSuccessResponseWriter(0) session := app.newMapSession(ctx, tailcfg.MapRequest{ + NodeKey: node.NodeKey(), Stream: true, Version: tailcfg.CapabilityVersion(100), }, writer, node) @@ -953,6 +960,7 @@ func TestDeletedNodeInterruptsBlockedWrite(t *testing.T) { writer := newDeadlineResponseWriter() session := app.newMapSession(t.Context(), tailcfg.MapRequest{ + NodeKey: nodeView.NodeKey(), Stream: true, Version: tailcfg.CapabilityVersion(100), }, writer, nodeView) diff --git a/hscontrol/state/maprequest.go b/hscontrol/state/maprequest.go index 07ef695a1..edbe9cac8 100644 --- a/hscontrol/state/maprequest.go +++ b/hscontrol/state/maprequest.go @@ -58,10 +58,9 @@ type mapRequestDelta struct { // happens regardless; this gates only broadcast. endpointBroadcast bool - // keyChanged and discoKeyChanged report whether the node's wire keys - // changed. Key patches already carry the resulting endpoints/expiry, + // discoKeyChanged reports whether the node's discovery key changed. + // Key patches already carry the resulting endpoints/expiry, // so a key change subsumes endpoint/DERP patches. - keyChanged bool discoKeyChanged bool // persistWorthy reports whether the request carries data that should @@ -78,7 +77,6 @@ func (d mapRequestDelta) MarshalZerologObject(e *zerolog.Event) { Int("derp.old", int(d.oldDERP)). Int("derp.new", int(d.newDERP)). Bool("endpoint.broadcast", d.endpointBroadcast). - Bool("key.changed", d.keyChanged). Bool("disco_key.changed", d.discoKeyChanged). Bool("persist", d.persistWorthy) } diff --git a/hscontrol/state/maprequest_test.go b/hscontrol/state/maprequest_test.go index 7ee958053..e16fb35c5 100644 --- a/hscontrol/state/maprequest_test.go +++ b/hscontrol/state/maprequest_test.go @@ -618,7 +618,6 @@ func TestConcurrentMapRequestDERPUsesPersistedState(t *testing.T) { require.NoError(t, err) if rotateKey { - req.NodeKey = key.NewNode().Public() req.DiscoKey = key.NewDisco().Public() } @@ -1507,3 +1506,70 @@ func TestSetApprovedRoutesReportsVisibility(t *testing.T) { }) } } + +func TestMapRequestKeepsCurrentNodeKey(t *testing.T) { + for _, queued := range []bool{false, true} { + t.Run(fmt.Sprintf("queued=%t", queued), func(t *testing.T) { + s := newRetagTestState(t) + user := s.CreateUserForTest("map-key-user") + k, err := s.CreatePreAuthKey(user.TypedID(), true, false, nil, nil) + require.NoError(t, err) + c := registerForPAKRereg(t, s, k) + view, pak := c.lookup(t, k.Key) + req := c.regReq + req.NodeKey = key.NewNode().Public() + oldRequest := tailcfg.MapRequest{ + NodeKey: c.regReq.NodeKey, + Endpoints: []netip.AddrPort{netip.MustParseAddrPort("192.0.2.1:1234")}, + Hostinfo: &tailcfg.Hostinfo{Hostname: "stale-host"}, + } + + if queued { + entered, release := make(chan struct{}), make(chan struct{}) + + unblock := sync.OnceFunc(func() { close(release) }) + defer unblock() + + go s.nodeStore.UpdateNode(c.node.ID(), func(*types.Node) { + close(entered) + <-release + }) + + <-entered + + registered := make(chan error, 1) + + go func() { _, err := c.reregister(view, pak, req); registered <- err }() + + waitParkedOnWriteQueue(t, "reregisterNodeWithPAK") + + mapped := make(chan error, 1) + + go func() { _, err := s.UpdateNodeFromMapRequest(c.node.ID(), oldRequest); mapped <- err }() + + waitParkedOnWriteQueue(t, "UpdateNodeFromMapRequest") + unblock() + require.NoError(t, <-registered) + require.ErrorContains(t, <-mapped, "node key is no longer current") + } else { + _, err := c.reregister(view, pak, req) + require.NoError(t, err) + change, err := s.UpdateNodeFromMapRequest(c.node.ID(), oldRequest) + require.ErrorContains(t, err, "node key is no longer current") + require.True(t, change.IsEmpty()) + } + + current, ok := s.GetNodeByID(c.node.ID()) + require.True(t, ok) + + stored, err := s.db.GetNodeByID(c.node.ID()) + require.NoError(t, err) + require.Equal(t, req.NodeKey, current.NodeKey()) + require.Equal(t, req.NodeKey, stored.NodeKey) + require.Equal(t, "h1", current.Hostname()) + require.Equal(t, "h1", stored.Hostname) + require.Empty(t, current.Endpoints().AsSlice()) + require.Empty(t, stored.Endpoints) + }) + } +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index d9650f0f9..8955d54e2 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -132,6 +132,9 @@ var ErrRegistrationExpired = errors.New("registration expired") // binding. var ErrNodeKeyInUse = errors.New("node key already in use by another machine") +// ErrNodeKeyNotCurrent is returned when a map request uses a replaced node key. +var ErrNodeKeyNotCurrent = errors.New("node key is no longer current") + // ErrAmbiguousNodeOwnership is returned when a machine key maps to a set of // nodes from which the correct one to update or convert cannot be determined: // multiple user-owned candidates for a tagged conversion, or a tagged node and @@ -3618,6 +3621,7 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest routeChange bool needsRouteApproval bool autoApprovedRoutes []netip.Prefix + staleNodeKey bool ) // Snapshot the primary assignment so we can tell whether the // Hostinfo + auto-approval that follows shifted any prefix. @@ -3627,10 +3631,14 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest // We need to ensure we update the node as it is in the [NodeStore] at // the time of the request. updatedNode, ok := s.nodeStore.UpdateNode(id, func(currentNode *types.Node) { + if currentNode.NodeKey != req.NodeKey { + staleNodeKey = true + return + } + // Capture the raw wire-level peer change. LastSeen is always // stamped here, so classification tests must not rely on it. delta.peerChange = currentNode.PeerChangeFromMapRequest(req) - delta.keyChanged = delta.peerChange.Key != nil delta.discoKeyChanged = delta.peerChange.DiscoKey != nil // Normalize before classifying. A nil req.Hostinfo means the client @@ -3785,6 +3793,10 @@ func (s *State) UpdateNodeFromMapRequest(id types.NodeID, req tailcfg.MapRequest return change.Change{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, id) } + if staleNodeKey { + return change.Change{}, ErrNodeKeyNotCurrent + } + if delta.dnsMetadataChanged { s.selfRefreshMu.Lock() s.selfRefresh = append(s.selfRefresh, id) @@ -3960,7 +3972,7 @@ func buildMapRequestChangeResponse( } } - if delta.keyChanged || delta.discoKeyChanged { + if delta.discoKeyChanged { c := change.NodeKeyRotated(node) if delta.derpChanged { c.PeerPatches[0].DERPRegion = currentDERP @@ -4008,8 +4020,7 @@ func routesChanged(oldNode types.NodeView, newHI *tailcfg.Hostinfo) bool { // [Node.PeerChangeFromMapRequest] always stamps: a keepalive that only bumps // LastSeen should not trigger a full-row UPDATE and policy rescan. func peerChangePersistWorthy(peerChange tailcfg.PeerChange) bool { - return peerChange.Key != nil || - peerChange.DiscoKey != nil || + return peerChange.DiscoKey != nil || peerChange.Online != nil || peerChange.Endpoints != nil || peerChange.DERPRegion != 0 || diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index b890632cd..01eba8dd7 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -822,10 +822,6 @@ func (node *Node) PeerChangeFromMapRequest(req tailcfg.MapRequest) tailcfg.PeerC NodeID: tailcfg.NodeID(node.ID), //nolint:gosec // NodeID is bounded } - if node.NodeKey.String() != req.NodeKey.String() { - ret.Key = &req.NodeKey - } - if node.DiscoKey.String() != req.DiscoKey.String() { ret.DiscoKey = &req.DiscoKey } @@ -857,11 +853,8 @@ func EndpointsChanged(oldEndpoints, newEndpoints []netip.AddrPort) bool { } // ApplyPeerChange takes a [tailcfg.PeerChange] struct and updates the node. +// Key is ignored: only registration rotates the node key. func (node *Node) ApplyPeerChange(change *tailcfg.PeerChange) { - if change.Key != nil { - node.NodeKey = *change.Key - } - if change.DiscoKey != nil { node.DiscoKey = *change.DiscoKey }