ipn/ipnlocal,wgengine: move disco-key change detection to nodeBackend

Engine.Reconfig previously diffed cfg.Peers disco keys against the
previous config to find restarted peers and flush their WireGuard
sessions, with a TSMP-learned-key map to suppress resets for key
changes that arrived over a working session. That was the last
per-peer state computed from wgcfg.Config.Peers inside the engine,
and it only ran on full reconfigs, so incremental netmap deltas
never got session resets at all.

Move the detection into nodeBackend, which sees every peer change:
full netmaps in SetNetMap and incremental upserts in
UpdateNetmapDelta both now report which peers changed disco keys,
with the same TSMP suppression and mismatch accounting as before.
LocalBackend acts on the result via a new Engine.ResetDevicePeer
method, which just removes the peer from the WireGuard device and
lets the peer lookup func lazily re-create it with fresh state.

LocalBackend.PatchDiscoKey now records TSMP-learned keys in
nodeBackend instead of forwarding to the engine, so the engine's
PatchDiscoKey method and tsmpLearnedDisco map are gone. The
controlclient patchDiscoKeyer interface becomes the exported
DiscoKeyUpdater so LocalBackend can compile-time assert that it
implements it, alongside its NetmapDeltaUpdater friends, replacing
the test that asserted the same of the engine.

This is one of the last steps toward removing Peers from wgcfg.Config.

Updates #12542

Change-Id: I6b42e460f42924816beae89ca43731cb91b66054
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

ipn/ipnlocal/node_backend.go

@@ -32,6 +32,7 @@ import (
 	"tailscale.com/util/dnsname"
 	"tailscale.com/util/eventbus"
 	"tailscale.com/util/mak"
+	"tailscale.com/util/set"
 	"tailscale.com/util/slicesx"
 	"tailscale.com/util/testenv"
 	"tailscale.com/wgengine/filter"
@@ -166,6 +167,14 @@ type nodeBackend struct {
 	// nil in production, where no test installs a waiter.
 	keyWaitersForTest map[key.NodePublic]chan struct{}
 
+	// tsmpLearnedDisco records, per node key, a peer disco key that was
+	// learned via TSMP (that is, over an existing WireGuard session with
+	// that peer). When a netmap update later reports the same disco key
+	// change, the peer's WireGuard session does not need to be reset,
+	// because the change demonstrably arrived over a working session.
+	// See [nodeBackend.discoChangedLocked].
+	tsmpLearnedDisco map[key.NodePublic]key.DiscoPublic
+
 	// routeMgr tracks this node's view of which IPs route to which
 	// peers and publishes lock-free snapshots for the data plane and
 	// the OS router. It is initialized once and immutable, but its
@@ -579,7 +588,7 @@ func (nb *nodeBackend) netMapWithPeers() *netmap.NetworkMap {
 	return nm
 }
 
-func (nb *nodeBackend) SetNetMap(nm *netmap.NetworkMap) {
+func (nb *nodeBackend) SetNetMap(nm *netmap.NetworkMap) (discoChanged []key.NodePublic) {
 	nb.mu.Lock()
 	defer nb.mu.Unlock()
 	nb.netMap = nm
@@ -587,7 +596,7 @@ func (nb *nodeBackend) SetNetMap(nm *netmap.NetworkMap) {
 	nb.updateNodeByKeyLocked()
 	nb.updateNodeByStableIDLocked()
 	nb.updateNodeByNameLocked()
-	nb.updatePeersLocked()
+	discoChanged = nb.updatePeersLocked()
 	nb.signalKeyWaitersForTestLocked()
 	if nm != nil {
 		nb.userProfiles = maps.Clone(nm.UserProfiles)
@@ -600,6 +609,7 @@ func (nb *nodeBackend) SetNetMap(nm *netmap.NetworkMap) {
 		nb.packetFilter = nil
 		nb.derpMapViewPub.Publish(tailcfg.DERPMapView{})
 	}
+	return discoChanged
 }
 
 // AwaitNodeKeyForTest returns a channel that is closed once a peer with the
@@ -780,10 +790,19 @@ func (nb *nodeBackend) ExtraDNSByName(hostname string) (_ netip.Addr, ok bool) {
 	return ip, ok
 }
 
-func (nb *nodeBackend) updatePeersLocked() {
+func (nb *nodeBackend) updatePeersLocked() (discoChanged []key.NodePublic) {
 	nm := nb.netMap
 	oldIDs := slices.Collect(maps.Keys(nb.peers))
 
+	// Snapshot the previous disco keys (by node key) before the peers
+	// map is repopulated, to detect restarted peers below.
+	var prevDisco map[key.NodePublic]key.DiscoPublic
+	for _, p := range nb.peers {
+		if !p.DiscoKey().IsZero() {
+			mak.Set(&prevDisco, p.Key(), p.DiscoKey())
+		}
+	}
+
 	if nm == nil {
 		nb.peers = nil
 	} else {
@@ -794,6 +813,18 @@ func (nb *nodeBackend) updatePeersLocked() {
 		})
 	}
 
+	for _, p := range nb.peers {
+		if prev, ok := prevDisco[p.Key()]; ok && nb.discoChangedLocked(p.Key(), prev, p.DiscoKey()) {
+			discoChanged = append(discoChanged, p.Key())
+		}
+	}
+	// Drop TSMP-learned disco keys for peers no longer in the netmap.
+	for k := range nb.tsmpLearnedDisco {
+		if _, ok := nb.nodeByKey[k]; !ok {
+			delete(nb.tsmpLearnedDisco, k)
+		}
+	}
+
 	// Resync the route manager to the new peer set. Upserts of
 	// unchanged peers are cheap no-ops; this is a full-netmap event,
 	// so O(n) work is expected here.
@@ -807,6 +838,52 @@ func (nb *nodeBackend) updatePeersLocked() {
 		rt.UpsertPeer(p)
 	}
 	rt.Commit()
+	return discoChanged
+}
+
+// recordTSMPLearnedDisco notes that a peer's new disco key was learned via
+// TSMP, so the netmap update carrying the same change need not reset the
+// peer's WireGuard session. See the [nodeBackend.tsmpLearnedDisco] field doc.
+func (nb *nodeBackend) recordTSMPLearnedDisco(pub key.NodePublic, disco key.DiscoPublic) {
+	nb.mu.Lock()
+	defer nb.mu.Unlock()
+	mak.Set(&nb.tsmpLearnedDisco, pub, disco)
+}
+
+// discoChangedLocked reports whether a peer's disco key change from prev to
+// cur should reset the peer's WireGuard session. A changed disco key means
+// the peer restarted, so any existing session key material is dead weight;
+// resetting lets the handshake start over immediately. The exception is a
+// key change already learned via TSMP: that arrived over a working WireGuard
+// session with the peer, so the session is demonstrably fine and is kept.
+//
+// It consumes any [nodeBackend.tsmpLearnedDisco] entry for pub.
+// nb.mu must be held.
+func (nb *nodeBackend) discoChangedLocked(pub key.NodePublic, prev, cur key.DiscoPublic) bool {
+	if prev.IsZero() || cur.IsZero() || prev == cur {
+		return false
+	}
+	if discoTSMP, ok := nb.tsmpLearnedDisco[pub]; ok {
+		delete(nb.tsmpLearnedDisco, pub)
+		if discoTSMP == cur {
+			nb.logf("nodeBackend: skipping WireGuard session reset (TSMP key): %s changed from %q to %q",
+				pub.ShortString(), prev, cur)
+			return false
+		}
+		// The new disco key does not match what we received via
+		// TSMP for this peer. This is unexpected, though possible
+		// if processing a change in a large netmap ends up taking
+		// longer than the 2 second timeout in
+		// [controlclient.mapRoutineState.UpdateNetmapDelta], or if
+		// the context is cancelled mid update. Log the event, and reset
+		// the session as it is possibly a stale entry in the map
+		// instead of a TSMP disco key update that led us here.
+		nb.logf("nodeBackend: [unexpected] using TSMP key for %s (control stale): tsmp=%q control=%q old=%q",
+			pub.ShortString(), discoTSMP, cur, prev)
+		metricTSMPLearnedKeyMismatch.Add(1)
+	}
+	nb.logf("nodeBackend: peer %s disco key changed from %q to %q", pub.ShortString(), prev, cur)
+	return true
 }
 
 // routePrefs is the subset of routing-relevant prefs (and derived
@@ -901,16 +978,30 @@ func (nb *nodeBackend) mergeUserProfiles(profiles map[tailcfg.UserID]tailcfg.Use
 	}
 }
 
+// netmapDeltaResult describes the side effects of applying netmap
+// delta mutations that the caller must propagate.
+type netmapDeltaResult struct {
+	// ChangedAllowedIPs are the peers whose allowed source prefixes
+	// changed, as described by [routemanager.Result.AllowedIPs]; the
+	// caller syncs those peers to the WireGuard device. In
+	// particular, the value for a key will be nil when that peer was
+	// removed.
+	ChangedAllowedIPs map[key.NodePublic][]netip.Prefix
+
+	// DiscoChanged is the set of peers whose disco key changed in a
+	// way that requires a WireGuard session reset (see
+	// [nodeBackend.discoChangedLocked]).
+	DiscoChanged set.Set[key.NodePublic]
+}
+
 // UpdateNetmapDelta applies the given netmap mutations to the live
-// peer state. It returns the peers whose allowed source prefixes
-// changed (as described by [routemanager.Result.AllowedIPs]) so the
-// caller can sync those peers to the WireGuard device, and reports
-// whether it handled all of the mutations.
-func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (changedAllowedIPs map[key.NodePublic][]netip.Prefix, handled bool) {
+// peer state. It returns the side effects for the caller to
+// propagate, and reports whether it handled all of the mutations.
+func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (res netmapDeltaResult, handled bool) {
 	nb.mu.Lock()
 	defer nb.mu.Unlock()
 	if nb.netMap == nil {
-		return nil, false
+		return res, false
 	}
 
 	// Locally cloned mutable nodes, to avoid calling AsStruct (clone)
@@ -925,14 +1016,23 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (changedAll
 	// get a full netmap soon and reset all our state anyway.
 	rt := nb.routeMgr.Begin()
 	defer func() {
-		res := rt.Commit()
-		changedAllowedIPs = res.AllowedIPs
+		res.ChangedAllowedIPs = rt.Commit().AllowedIPs
 	}()
 
 	for _, m := range muts {
 		switch m := m.(type) {
 		case netmap.NodeMutationUpsert:
 			nid := m.Node.ID()
+			if old, ok := nb.peers[nid]; ok {
+				if old.Key() == m.Node.Key() {
+					if nb.discoChangedLocked(m.Node.Key(), old.DiscoKey(), m.Node.DiscoKey()) {
+						res.DiscoChanged.Make()
+						res.DiscoChanged.Add(m.Node.Key())
+					}
+				} else {
+					delete(nb.tsmpLearnedDisco, old.Key())
+				}
+			}
 			mak.Set(&nb.peers, nid, m.Node)
 			for _, ipp := range m.Node.Addresses().All() {
 				if ipp.IsSingleIP() {
@@ -956,6 +1056,7 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (changedAll
 				delete(nb.nodeByKey, old.Key())
 				delete(nb.nodeByWGString, old.Key().WireGuardGoString())
 				delete(nb.nodeByStableID, old.StableID())
+				delete(nb.tsmpLearnedDisco, old.Key())
 				nb.removeNodeNameLocked(old.Name())
 				delete(nb.peers, nid)
 				rt.RemovePeer(nid)
@@ -969,7 +1070,7 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (changedAll
 			nv, ok := nb.peers[nid]
 			if !ok {
 				// TODO(bradfitz): unexpected metric?
-				return nil, false
+				return res, false
 			}
 			n = nv.AsStruct()
 			mak.Set(&mutableNodes, nv.ID(), n)
@@ -980,7 +1081,7 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (changedAll
 		nb.peers[nid] = n.View()
 	}
 	nb.signalKeyWaitersForTestLocked()
-	return nil, true
+	return res, true
 }
 
 // unlockedNodesPermitted reports whether any peer with theUnsignedPeerAPIOnly bool set true has any of its allowed IPs