state: apply map requests only under the node key that authorised them

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)

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)
 }

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)
+		})
+	}
+}

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 ||

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
 	}