diff --git a/control/controlclient/auto.go b/control/controlclient/auto.go index 542426f38..137b8a39b 100644 --- a/control/controlclient/auto.go +++ b/control/controlclient/auto.go @@ -575,13 +575,13 @@ func (mrs mapRoutineState) UpdateUserProfiles(profiles map[tailcfg.UserID]tailcf } } -var _ patchDiscoKeyer = mapRoutineState{} +var _ DiscoKeyUpdater = mapRoutineState{} func (mrs mapRoutineState) PatchDiscoKey(pub key.NodePublic, disco key.DiscoPublic) { c := mrs.c c.mu.Lock() goodState := c.loggedIn && c.inMapPoll - dun, ok := c.observer.(patchDiscoKeyer) + dun, ok := c.observer.(DiscoKeyUpdater) mapCtx := c.mapCtx c.mu.Unlock() diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 062af8dd5..8ac635616 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -286,10 +286,10 @@ type UserProfileUpdater interface { UpdateUserProfiles(profiles map[tailcfg.UserID]tailcfg.UserProfileView) bool } -// patchDiscoKeyer is an optional interface that can be implemented by an [Observer] to be +// DiscoKeyUpdater is an optional interface that can be implemented by an [Observer] to be // notified about node disco keys received out-of-band from control, via // existing connection state. -type patchDiscoKeyer interface { +type DiscoKeyUpdater interface { // PatchDiscoKey reports to the receiver that the specified disco key // for node was obtained out-of-band from control. PatchDiscoKey(key.NodePublic, key.DiscoPublic) diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 0236c3fc0..a601c8579 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -362,7 +362,7 @@ func (ms *mapSession) handleNonKeepAliveMapResponse(ctx context.Context, resp *t } func (ms *mapSession) tryMarkDiscoAsLearnedFromTSMP(res *tailcfg.MapResponse) { - dun, ok := ms.netmapUpdater.(patchDiscoKeyer) + dun, ok := ms.netmapUpdater.(DiscoKeyUpdater) if !ok { return } diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index 057e0b4c8..3367f7809 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -34,9 +34,7 @@ "tailscale.com/util/eventbus/eventbustest" "tailscale.com/util/mak" "tailscale.com/util/must" - "tailscale.com/util/usermetric" "tailscale.com/util/zstdframe" - "tailscale.com/wgengine" ) func eps(s ...string) []netip.AddrPort { @@ -1949,20 +1947,6 @@ func TestLearnZstdOfKeepAlive(t *testing.T) { } } -func TestPathDiscokeyerImplementations(t *testing.T) { - bus := eventbustest.NewBus(t) - ht := health.NewTracker(bus) - reg := new(usermetric.Registry) - e, err := wgengine.NewFakeUserspaceEngine(t.Logf, 0, ht, reg, bus) - if err != nil { - t.Fatal(err) - } - t.Cleanup(e.Close) - if _, ok := e.(patchDiscoKeyer); !ok { - t.Error("wgengine.userspaceEngine must implement patchDiscoKeyer") - } -} - func TestPeerIDAndKeyByTailscaleIP(t *testing.T) { peerKey1 := key.NewNode().Public() peerKey2 := key.NewNode().Public() diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index c4c761bae..c4bb293ec 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2144,16 +2144,11 @@ func (b *LocalBackend) setControlClientStatusLocked(c controlclient.Client, st c b.authReconfigLocked() } +// PatchDiscoKey records 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. It implements [controlclient.DiscoKeyUpdater]. func (b *LocalBackend) PatchDiscoKey(pub key.NodePublic, disco key.DiscoPublic) { - // PatchDiscoKey mirrors the implementation of [controlclient.patchDiscoKeyer]. - // It is implemented here to avoid the dependency edge to controlclient, but must be kept - // in sync with the original implementation. - type patchDiscoKeyer interface { - PatchDiscoKey(key.NodePublic, key.DiscoPublic) - } - if e, ok := b.e.(patchDiscoKeyer); ok { - e.PatchDiscoKey(pub, disco) - } + b.currentNode().recordTSMPLearnedDisco(pub, disco) } type preferencePolicyInfo struct { @@ -2436,6 +2431,7 @@ func (b *LocalBackend) sysPolicyChangedForSession(sess *watchSession) { _ controlclient.NetmapDeltaUpdater = (*LocalBackend)(nil) _ controlclient.PacketFilterUpdater = (*LocalBackend)(nil) _ controlclient.UserProfileUpdater = (*LocalBackend)(nil) + _ controlclient.DiscoKeyUpdater = (*LocalBackend)(nil) ) // UpdateNetmapDelta implements controlclient.NetmapDeltaUpdater. @@ -2464,7 +2460,7 @@ func (b *LocalBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo muts = b.tkaFilterDeltaMutsLocked(muts) needsAuthReconfig := netmapDeltaNeedsAuthReconfig(cn, muts) - _, changedAllowedIPs := cn.UpdateNetmapDelta(muts) + _, changedAllowedIPs, discoChanged := cn.UpdateNetmapDelta(muts) if buildfeatures.HasDrive { // Drive's lazy remotes-source caches its rebuild keyed by this // generation, so any delta — peer add/remove, address change, @@ -2515,6 +2511,13 @@ func (b *LocalBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo b.e.SyncDevicePeer(k) } + // Reset the WireGuard session for peers whose disco key changed in + // a way that indicates a restart, flushing their dead session keys; + // each such peer is lazily re-created on demand with current state. + for _, k := range discoChanged { + b.e.ResetDevicePeer(k) + } + // Force a full authReconfig + SetSelfNode on any peer add or // remove. The WireGuard device itself no longer depends on this: // the SyncDevicePeer calls above keep its peer set delta-correct, @@ -7302,7 +7305,7 @@ func (b *LocalBackend) setNetMapLocked(nm *netmap.NetworkMap) { if nm != nil { login = cmp.Or(profileFromView(nm.UserProfiles[nm.User()]).LoginName, "") } - b.currentNode().SetNetMap(nm) + discoChanged := b.currentNode().SetNetMap(nm) if ms, ok := b.sys.MagicSock.GetOK(); ok { if nm != nil { if nm.Cached { @@ -7314,6 +7317,12 @@ func (b *LocalBackend) setNetMapLocked(nm *netmap.NetworkMap) { ms.SetNetworkMap(tailcfg.NodeView{}, nil) } } + // Reset the WireGuard session for peers whose disco key changed in + // a way that indicates a restart, flushing their dead session keys; + // each such peer is lazily re-created on demand with current state. + for _, k := range discoChanged { + b.e.ResetDevicePeer(k) + } if login != b.activeLogin { b.logf("active login: %v", login) b.activeLogin = login @@ -9187,6 +9196,11 @@ func maybeUsernameOf(actor ipnauth.Actor) string { metricNetmapDeltaPeerPatched = clientmetric.NewCounter("localbackend_netmap_delta_peer_patched") metricUpdatePacketFilter = clientmetric.NewCounter("localbackend_update_packet_filter") metricUpdateUserProfiles = clientmetric.NewCounter("localbackend_update_user_profiles") + + // metricTSMPLearnedKeyMismatch counts netmap updates carrying a peer + // disco key that doesn't match the one previously learned via TSMP + // for the same peer. See [nodeBackend.discoChangedLocked]. + metricTSMPLearnedKeyMismatch = clientmetric.NewCounter("magicsock_tsmp_learned_key_mismatch") ) func (b *LocalBackend) stateEncrypted() opt.Bool { diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 86ddfd006..753744ba7 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -2622,7 +2622,7 @@ func TestNotifyForSessionUserProfilesDedupResetsOnSelfChange(t *testing.T) { // tests LocalBackend.updateNetmapDeltaLocked func TestUpdateNetmapDelta(t *testing.T) { b := newTestLocalBackend(t) - if handled, _ := b.currentNode().UpdateNetmapDelta(nil); handled { + if handled, _, _ := b.currentNode().UpdateNetmapDelta(nil); handled { t.Errorf("updateNetmapDeltaLocked() = true, want false with nil netmap") } @@ -2661,7 +2661,7 @@ func TestUpdateNetmapDelta(t *testing.T) { t.Fatal("netmap.MutationsFromMapResponse failed") } - if handled, _ := b.currentNode().UpdateNetmapDelta(muts); !handled { + if handled, _, _ := b.currentNode().UpdateNetmapDelta(muts); !handled { t.Fatalf("updateNetmapDeltaLocked() = false, want true with new netmap") } diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index 85c56cc7e..3e4c228a6 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/ipn/ipnlocal/node_backend.go @@ -166,6 +166,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 +587,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 +595,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 +608,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 +789,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 +812,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 +837,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 @@ -926,12 +1002,14 @@ func (nb *nodeBackend) mergeUserProfiles(profiles map[tailcfg.UserID]tailcfg.Use // peer state. It reports whether it handled all of them, and 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. -func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bool, changedAllowedIPs map[key.NodePublic][]netip.Prefix) { +// peers to the WireGuard device, plus the peers whose disco key +// changed in a way that requires a WireGuard session reset (see +// [nodeBackend.discoChangedLocked]). +func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bool, changedAllowedIPs map[key.NodePublic][]netip.Prefix, discoChanged []key.NodePublic) { nb.mu.Lock() defer nb.mu.Unlock() if nb.netMap == nil { - return false, nil + return false, nil, nil } // Locally cloned mutable nodes, to avoid calling AsStruct (clone) @@ -954,6 +1032,15 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo 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()) { + discoChanged = append(discoChanged, 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() { @@ -977,6 +1064,7 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo 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) @@ -990,7 +1078,7 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo nv, ok := nb.peers[nid] if !ok { // TODO(bradfitz): unexpected metric? - return false, nil + return false, nil, discoChanged } n = nv.AsStruct() mak.Set(&mutableNodes, nv.ID(), n) @@ -1001,7 +1089,7 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo nb.peers[nid] = n.View() } nb.signalKeyWaitersForTestLocked() - return true, nil + return true, nil, discoChanged } // unlockedNodesPermitted reports whether any peer with theUnsignedPeerAPIOnly bool set true has any of its allowed IPs diff --git a/ipn/ipnlocal/node_backend_test.go b/ipn/ipnlocal/node_backend_test.go index 94c12121c..21ec40955 100644 --- a/ipn/ipnlocal/node_backend_test.go +++ b/ipn/ipnlocal/node_backend_test.go @@ -346,7 +346,7 @@ func TestNodeBackendRouteManager(t *testing.T) { // Incremental deltas: add peer 3, remove peer 1. p3 := mkPeer(3, "stable3", "100.64.0.3/32") - handled, changed := nb.UpdateNetmapDelta([]netmap.NodeMutation{ + handled, changed, _ := nb.UpdateNetmapDelta([]netmap.NodeMutation{ netmap.NodeMutationUpsert{Node: p3}, netmap.MakeNodeMutationRemove(1), }) @@ -420,3 +420,142 @@ func TestNodeBackendRouteManagerExtras(t *testing.T) { t.Errorf("Outbound still routes %v after extras cleared", transit.Addr()) } } + +// TestNodeBackendDiscoChanged exercises the full-netmap disco change +// detection: a peer whose disco key changes has restarted and needs its +// WireGuard session reset, unless the new key was already learned over +// TSMP (that is, over a working WireGuard session with the peer). +func TestNodeBackendDiscoChanged(t *testing.T) { + nb := newNodeBackend(t.Context(), tstest.WhileTestRunningLogger(t), eventbus.New()) + + nk := key.NewNode().Public() + mkNetMap := func(disco key.DiscoPublic) *netmap.NetworkMap { + n := &tailcfg.Node{ + ID: 1, + Key: nk, + DiscoKey: disco, + HomeDERP: 1, + } + return &netmap.NetworkMap{Peers: []tailcfg.NodeView{n.View()}} + } + newDisco := func() key.DiscoPublic { return key.NewDisco().Public() } + + // A brand-new peer is not a disco change. + d1 := newDisco() + if got := nb.SetNetMap(mkNetMap(d1)); len(got) != 0 { + t.Errorf("SetNetMap(new peer) discoChanged = %v; want none", got) + } + + // A changed disco key requires a session reset. + d2 := newDisco() + if got := nb.SetNetMap(mkNetMap(d2)); !slices.Contains(got, nk) { + t.Errorf("SetNetMap(changed disco) discoChanged = %v; want %v", got, nk) + } + + // An unchanged disco key does not. + if got := nb.SetNetMap(mkNetMap(d2)); len(got) != 0 { + t.Errorf("SetNetMap(same disco) discoChanged = %v; want none", got) + } + + // A change already learned via TSMP is suppressed... + d3 := newDisco() + nb.recordTSMPLearnedDisco(nk, d3) + if got := nb.SetNetMap(mkNetMap(d3)); len(got) != 0 { + t.Errorf("SetNetMap(TSMP-learned disco) discoChanged = %v; want none", got) + } + + // ...but the TSMP entry is consumed, so the next change resets again. + d4 := newDisco() + if got := nb.SetNetMap(mkNetMap(d4)); !slices.Contains(got, nk) { + t.Errorf("SetNetMap(after TSMP entry consumed) discoChanged = %v; want %v", got, nk) + } + + // A TSMP-learned key that doesn't match the netmap's new key still + // resets the session and bumps the mismatch metric. + before := metricTSMPLearnedKeyMismatch.Value() + nb.recordTSMPLearnedDisco(nk, newDisco()) + d5 := newDisco() + if got := nb.SetNetMap(mkNetMap(d5)); !slices.Contains(got, nk) { + t.Errorf("SetNetMap(TSMP mismatch) discoChanged = %v; want %v", got, nk) + } + if delta := metricTSMPLearnedKeyMismatch.Value() - before; delta != 1 { + t.Errorf("metricTSMPLearnedKeyMismatch delta = %d; want 1", delta) + } + + // Removing the peer garbage-collects its TSMP entry: after the peer + // comes back, a change to the once-recorded key is a normal reset. + d6 := newDisco() + nb.recordTSMPLearnedDisco(nk, d6) + nb.SetNetMap(&netmap.NetworkMap{}) + nb.SetNetMap(mkNetMap(d5)) + if got := nb.SetNetMap(mkNetMap(d6)); !slices.Contains(got, nk) { + t.Errorf("SetNetMap(after TSMP entry GC) discoChanged = %v; want %v", got, nk) + } + + // Transitions to or from a zero disco key never reset. + if got := nb.SetNetMap(mkNetMap(key.DiscoPublic{})); len(got) != 0 { + t.Errorf("SetNetMap(to zero disco) discoChanged = %v; want none", got) + } + if got := nb.SetNetMap(mkNetMap(d1)); len(got) != 0 { + t.Errorf("SetNetMap(from zero disco) discoChanged = %v; want none", got) + } +} + +// TestNodeBackendDiscoChangedDelta is like TestNodeBackendDiscoChanged +// but for the incremental path: disco changes arriving as +// [netmap.NodeMutationUpsert] deltas. +func TestNodeBackendDiscoChangedDelta(t *testing.T) { + nb := newNodeBackend(t.Context(), tstest.WhileTestRunningLogger(t), eventbus.New()) + + mkNode := func(k key.NodePublic, disco key.DiscoPublic) tailcfg.NodeView { + return (&tailcfg.Node{ID: 1, Key: k, DiscoKey: disco, HomeDERP: 1}).View() + } + newDisco := func() key.DiscoPublic { return key.NewDisco().Public() } + apply := func(muts ...netmap.NodeMutation) []key.NodePublic { + t.Helper() + handled, _, discoChanged := nb.UpdateNetmapDelta(muts) + if !handled { + t.Fatal("UpdateNetmapDelta not handled") + } + return discoChanged + } + + nk := key.NewNode().Public() + d1 := newDisco() + nb.SetNetMap(&netmap.NetworkMap{Peers: []tailcfg.NodeView{mkNode(nk, d1)}}) + + // An upserted peer with a changed disco key needs a session reset. + d2 := newDisco() + if got := apply(netmap.NodeMutationUpsert{Node: mkNode(nk, d2)}); !slices.Contains(got, nk) { + t.Errorf("upsert(changed disco) discoChanged = %v; want %v", got, nk) + } + + // An unchanged disco key does not. + if got := apply(netmap.NodeMutationUpsert{Node: mkNode(nk, d2)}); len(got) != 0 { + t.Errorf("upsert(same disco) discoChanged = %v; want none", got) + } + + // A change already learned via TSMP is suppressed. + d3 := newDisco() + nb.recordTSMPLearnedDisco(nk, d3) + if got := apply(netmap.NodeMutationUpsert{Node: mkNode(nk, d3)}); len(got) != 0 { + t.Errorf("upsert(TSMP-learned disco) discoChanged = %v; want none", got) + } + + // A node key rotation replaces the WireGuard peer outright, so no + // disco-based reset is reported. + nk2 := key.NewNode().Public() + if got := apply(netmap.NodeMutationUpsert{Node: mkNode(nk2, newDisco())}); len(got) != 0 { + t.Errorf("upsert(rotated node key) discoChanged = %v; want none", got) + } + + // Removing the peer garbage-collects its TSMP entry: after the peer + // comes back, a change to the once-recorded key is a normal reset. + d4 := newDisco() + nb.recordTSMPLearnedDisco(nk2, d4) + apply(netmap.MakeNodeMutationRemove(1)) + apply(netmap.NodeMutationUpsert{Node: mkNode(nk2, newDisco())}) + if got := apply(netmap.NodeMutationUpsert{Node: mkNode(nk2, d4)}); !slices.Contains(got, nk2) { + t.Errorf("upsert(after TSMP entry GC) discoChanged = %v; want %v", got, nk2) + } +} diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index b40e18853..47cf595ed 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -2014,7 +2014,8 @@ func (e *mockEngine) SetPeerByIPPacketFunc(func(netip.Addr) (_ key.NodePublic, o func (e *mockEngine) SetPeerForIPFunc(func(netip.Addr) (_ wgengine.PeerForIP, ok bool)) {} func (e *mockEngine) SetPeerConfigFunc(func(key.NodePublic) (allowedIPs []netip.Prefix, ok bool)) { } -func (e *mockEngine) SyncDevicePeer(key.NodePublic) {} +func (e *mockEngine) SyncDevicePeer(key.NodePublic) {} +func (e *mockEngine) ResetDevicePeer(key.NodePublic) {} func (e *mockEngine) SetPeerSessionStateFunc(func(key.NodePublic, wgengine.PeerWireGuardState)) { } func (e *mockEngine) SetNetLogSource(wgengine.NetLogSource) {} diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 5406fbf86..c9b66f07d 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -50,7 +50,6 @@ "tailscale.com/util/eventbus" "tailscale.com/util/execqueue" "tailscale.com/util/mak" - "tailscale.com/util/set" "tailscale.com/util/singleflight" "tailscale.com/util/testenv" "tailscale.com/util/usermetric" @@ -89,8 +88,7 @@ type userspaceEngine struct { birdClient BIRDClient // or nil controlKnobs *controlknobs.Knobs // or nil - testMaybeReconfigHook func() // for tests; if non-nil, fires if maybeReconfigWireguardLocked called - testDiscoChangedHook func(map[key.NodePublic]bool) // for tests; if non-nil, fires after assembling discoChanged map + testMaybeReconfigHook func() // for tests; if non-nil, fires if maybeReconfigWireguardLocked called // isLocalAddr reports the whether an IP is assigned to the local // tunnel interface. It's used to reflect local packets @@ -161,10 +159,6 @@ type userspaceEngine struct { // rewritten. wgPeerLookup syncs.AtomicValue[func(wgString string) (tsString string, ok bool)] - // tsmpLearnedDisco tracks per node key if a peer disco key was learned via TSMP. - // wgLock must be held when using this map. - tsmpLearnedDisco map[key.NodePublic]key.DiscoPublic - // Lock ordering: magicsock.Conn.mu, wgLock, then mu. } @@ -763,6 +757,13 @@ func (e *userspaceEngine) SyncDevicePeer(k key.NodePublic) { } } +// ResetDevicePeer implements [Engine.ResetDevicePeer]. +func (e *userspaceEngine) ResetDevicePeer(k key.NodePublic) { + e.wgLock.Lock() + defer e.wgLock.Unlock() + e.wgdev.RemovePeer(k.Raw32()) +} + // SetPeerByIPPacketFunc installs a callback used by wireguard-go to look up // which peer should handle an outbound packet by destination IP. // @@ -848,12 +849,6 @@ func (e *userspaceEngine) ResetAndStop() (*Status, error) { return e.getStatus() } -func (e *userspaceEngine) PatchDiscoKey(pub key.NodePublic, disco key.DiscoPublic) { - e.wgLock.Lock() - defer e.wgLock.Unlock() - mak.Set(&e.tsmpLearnedDisco, pub, disco) -} - func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config) error { if routerCfg == nil { panic("routerCfg must not be nil") @@ -868,11 +863,6 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, defer e.wgLock.Unlock() e.tundev.SetWGConfig(cfg) - peerSet := make(set.Set[key.NodePublic], len(cfg.Peers)) - for _, p := range cfg.Peers { - peerSet.Add(p.PublicKey) - } - e.mu.Lock() self := e.selfNode e.mu.Unlock() @@ -925,63 +915,6 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, e.isDNSIPOverTailscale.Store(ipset.NewContainsIPFunc(views.SliceOf(dnsIPsOverTailscale(dnsCfg, routerCfg)))) } - // See if any peers have changed disco keys, which means they've restarted. - // If so, remove the peer from wireguard-go to flush its session key, - // then let the PeerLookupFunc re-create it on demand. - discoChanged := make(map[key.NodePublic]bool) - if engineChanged { - prevEP := make(map[key.NodePublic]key.DiscoPublic) - for i := range e.lastCfgFull.Peers { - if p := &e.lastCfgFull.Peers[i]; !p.DiscoKey.IsZero() { - prevEP[p.PublicKey] = p.DiscoKey - } - } - for i := range cfg.Peers { - p := &cfg.Peers[i] - if p.DiscoKey.IsZero() { - continue - } - - pub := p.PublicKey - - if old, ok := prevEP[pub]; ok && old != p.DiscoKey { - // If the disco key was learned via TSMP, we do not need to reset the - // wireguard config as the new key was received over an existing wireguard - // connection. - if discoTSMP, okTSMP := e.tsmpLearnedDisco[p.PublicKey]; okTSMP { - // Key matches, remove entry from map. - delete(e.tsmpLearnedDisco, p.PublicKey) - if discoTSMP == p.DiscoKey { - e.logf("wgengine: Skipping reconfig (TSMP key): %s changed from %q to %q", - pub.ShortString(), old, p.DiscoKey) - // Skip session clear. - continue - } - - // 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 connection as it is possibly a stale entry in the map - // instead of a TSMP disco key update that led us here. - e.logf("wgengine: [unexpected] Reconfig: using TSMP key for %s (control stale): tsmp=%q control=%q old=%q", - pub.ShortString(), discoTSMP, p.DiscoKey, old) - metricTSMPLearnedKeyMismatch.Add(1) - } - - discoChanged[pub] = true - e.logf("wgengine: Reconfig: %s changed from %q to %q", pub.ShortString(), old, p.DiscoKey) - } - } - } - - // For tests, what disco connections needs to be changed. - if e.testDiscoChangedHook != nil { - e.testDiscoChangedHook(discoChanged) - } - if !e.lastCfgFull.PrivateKey.Equal(cfg.PrivateKey) { // Tell magicsock about the new (or initial) private key // (which is needed by DERP) before wgdev gets it, as wgdev @@ -1005,19 +938,6 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, if err := e.maybeReconfigWireguardLocked(); err != nil { return err } - // Now that we've reconfigured wireguard-go, remove any peers with - // changed disco keys to flush their session keys, and let them be - // re-created on demand by the PeerLookupFunc. - for pub := range discoChanged { - e.wgdev.RemovePeer(pub.Raw32()) - } - } - - // Cleanup map of tsmp marks for peers that no longer exists in config. - for nodeKey := range e.tsmpLearnedDisco { - if !peerSet.Contains(nodeKey) { - delete(e.tsmpLearnedDisco, nodeKey) - } } if routerChanged { @@ -1689,8 +1609,6 @@ func (ls fwdDNSLinkSelector) PickLink(ip netip.Addr) (linkName string) { metricTSMPDiscoKeyAdvertisementSent = clientmetric.NewCounter("magicsock_tsmp_disco_key_advertisement_sent") metricTSMPDiscoKeyAdvertisementError = clientmetric.NewCounter("magicsock_tsmp_disco_key_advertisement_error") - - metricTSMPLearnedKeyMismatch = clientmetric.NewCounter("magicsock_tsmp_learned_key_mismatch") ) func (e *userspaceEngine) InstallCaptureHook(cb packet.CaptureCallback) { diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 95fbf5a75..890a9cb0b 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -125,167 +125,6 @@ func TestUserspaceEngineReconfig(t *testing.T) { } } -func TestUserspaceEngineTSMPLearned(t *testing.T) { - bus := eventbustest.NewBus(t) - - ht := health.NewTracker(bus) - reg := new(usermetric.Registry) - e, err := NewFakeUserspaceEngine(t.Logf, 0, ht, reg, bus) - if err != nil { - t.Fatal(err) - } - t.Cleanup(e.Close) - ue := e.(*userspaceEngine) - - discoChangedChan := make(chan map[key.NodePublic]bool, 1) - ue.testDiscoChangedHook = func(m map[key.NodePublic]bool) { - discoChangedChan <- m - } - - routerCfg := &router.Config{} - - keyChanges := []struct { - tsmp bool - inMap bool - }{ - {tsmp: false, inMap: false}, - {tsmp: true, inMap: false}, - {tsmp: false, inMap: true}, - } - - nkHex := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - for _, change := range keyChanges { - oldDisco := key.NewDisco() - nm := &netmap.NetworkMap{ - Peers: nodeViews([]*tailcfg.Node{ - { - ID: 1, - Key: nkFromHex(nkHex), - DiscoKey: oldDisco.Public(), - }, - }), - } - nk, err := key.ParseNodePublicUntyped(mem.S(nkHex)) - if err != nil { - t.Fatal(err) - } - e.SetSelfNode(nm.SelfNode) - - newDisco := key.NewDisco() - cfg := &wgcfg.Config{ - Peers: []wgcfg.Peer{ - { - PublicKey: nk, - DiscoKey: newDisco.Public(), - }, - }, - } - - if change.tsmp { - ue.PatchDiscoKey(nk, newDisco.Public()) - } - err = e.Reconfig(cfg, routerCfg, &dns.Config{}) - if err != nil { - t.Fatal(err) - } - - changeMap := <-discoChangedChan - - if _, ok := changeMap[nk]; ok != change.inMap { - t.Fatalf("expect key %v in map %v to be %t, got %t", nk, changeMap, - change.inMap, ok) - } - } -} - -func TestUserspaceEngineTSMPLearnedMismatch(t *testing.T) { - bus := eventbustest.NewBus(t) - - ht := health.NewTracker(bus) - reg := new(usermetric.Registry) - e, err := NewFakeUserspaceEngine(t.Logf, 0, ht, reg, bus) - if err != nil { - t.Fatal(err) - } - t.Cleanup(e.Close) - ue := e.(*userspaceEngine) - - discoChangedChan := make(chan map[key.NodePublic]bool, 1) - ue.testDiscoChangedHook = func(m map[key.NodePublic]bool) { - discoChangedChan <- m - } - - routerCfg := &router.Config{} - var metricValue int64 = 0 - - keyChanges := []struct { - tsmp bool - inMap bool - wrongKey bool - }{ - {tsmp: false, inMap: false, wrongKey: false}, - {tsmp: true, inMap: false, wrongKey: false}, - {tsmp: true, inMap: true, wrongKey: true}, - {tsmp: false, inMap: true, wrongKey: false}, - } - - nkHex := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" - for _, change := range keyChanges { - oldDisco := key.NewDisco() - nm := &netmap.NetworkMap{ - Peers: nodeViews([]*tailcfg.Node{ - { - ID: 1, - Key: nkFromHex(nkHex), - DiscoKey: oldDisco.Public(), - }, - }), - } - nk, err := key.ParseNodePublicUntyped(mem.S(nkHex)) - if err != nil { - t.Fatal(err) - } - e.SetSelfNode(nm.SelfNode) - - newDisco := key.NewDisco() - cfg := &wgcfg.Config{ - Peers: []wgcfg.Peer{ - { - PublicKey: nk, - DiscoKey: newDisco.Public(), - }, - }, - } - - tsmpKey := newDisco.Public() - if change.tsmp { - if change.wrongKey { - tsmpKey = key.NewDisco().Public() - } - ue.PatchDiscoKey(nk, tsmpKey) - } - err = e.Reconfig(cfg, routerCfg, &dns.Config{}) - if err != nil { - t.Fatal(err) - } - - changeMap := <-discoChangedChan - - if _, ok := changeMap[nk]; ok != change.inMap { - t.Fatalf("expect key %v in map %v to be %t, got %t", nk, changeMap, - change.inMap, ok) - } - - metric := metricTSMPLearnedKeyMismatch.Value() - delta := metric - metricValue - metricValue = metric - - if change.wrongKey && delta != 1 { - t.Fatalf("expected a delta of 1, got %d", delta) - } - } -} - func TestUserspaceEnginePortReconfig(t *testing.T) { flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/2855") const defaultPort = 49983 diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index 25c1de5cc..774e1c1aa 100644 --- a/wgengine/wgengine.go +++ b/wgengine/wgengine.go @@ -198,6 +198,16 @@ type Engine interface { // It is a no-op if no config source is installed. SyncDevicePeer(key.NodePublic) + // ResetDevicePeer removes the peer from the WireGuard device, + // discarding any session key material and in-flight handshake + // state. If the peer is still known to the config source installed + // via [Engine.SetPeerConfigFunc], it is lazily re-created on demand + // with fresh state. + // + // LocalBackend calls it when a peer's disco key changes, which + // means the peer restarted and its old sessions are dead. + ResetDevicePeer(key.NodePublic) + // SetNetLogSource installs the [NetLogSource] consulted by the // engine's network flow logger for node lookups and the current // audit logging identity.