From 016a3768e1159da96bd2eb531f4e974f66a356ff Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 12 Jun 2026 21:06:51 +0000 Subject: [PATCH] wgengine/magicsock: bootstrap disco for a re-added peer with no path UpsertPeer kicks disco when a peer has no trusted path; a DERP-only peer also solicits callMeMaybe. No-ops for peers with an active trusted path. --- wgengine/magicsock/endpoint.go | 52 ++++++++++++++- wgengine/magicsock/endpoint_test.go | 100 ++++++++++++++++++++++++++++ wgengine/magicsock/magicsock.go | 10 +++ 3 files changed, 161 insertions(+), 1 deletion(-) diff --git a/wgengine/magicsock/endpoint.go b/wgengine/magicsock/endpoint.go index 0513c113a..5249a209a 100644 --- a/wgengine/magicsock/endpoint.go +++ b/wgengine/magicsock/endpoint.go @@ -969,6 +969,48 @@ func (de *endpoint) noteTxActivityExtTriggerLocked(now mono.Time) { } } +// maybeKickDiscoBootstrap starts disco (pinging known endpoints and soliciting +// callMeMaybe over DERP) for a peer that has no trusted path yet, without +// waiting for outbound WireGuard traffic. +// +// Disco is normally armed only off an [endpoint.send], which calls +// [endpoint.sendDiscoPingsLocked] and arms the heartbeat when there is no +// trusted best path (endpoint.go: `!udpAddr.isDirect() || +// now.After(de.trustBestAddrUntil)`). A peer added or re-keyed via the delta +// path ([Conn.UpsertPeer]) with no traffic flowing toward it never reaches that +// send, so disco never starts: the node keeps answering pings but never +// initiates, leaving it one-way dead until traffic happens to flow +// +// +// It only fires for peers that genuinely need bootstrapping — disco-capable, +// not expired, with something to probe (a DERP home or known UDP candidates) +// and no currently-trusted path. A peer that already has a trusted path is +// skipped, so this neither spams active sessions nor re-arms a working path. +func (de *endpoint) maybeKickDiscoBootstrap() { + de.mu.Lock() + defer de.mu.Unlock() + + if de.isWireguardOnly || de.expired { + return + } + if de.disco.Load() == nil { + return + } + if !de.derpAddr.IsValid() && len(de.endpointState) == 0 { + // Nothing to probe toward. + return + } + now := mono.Now() + if de.bestAddr.epAddr.ap.IsValid() && now.Before(de.trustBestAddrUntil) { + // Already have a trusted path; disco isn't needed. + return + } + // Arm the heartbeat so disco keeps re-driving if the first round doesn't + // establish a path, then ping known endpoints and solicit callMeMaybe now. + de.noteTxActivityExtTriggerLocked(now) + de.sendDiscoPingsLocked(now, true) +} + // MaxDiscoPingSize is the largest useful ping message size that we // can send - the maximum packet size minus the IPv4 and UDP headers. var MaxDiscoPingSize = tstun.MaxPacketSize - 20 - 8 @@ -1388,7 +1430,15 @@ func (de *endpoint) sendDiscoPingsLocked(now mono.Time, sendCallMeMaybe bool) { de.startDiscoPingLocked(epAddr{ap: ep}, now, pingDiscovery, 0, nil) } derpAddr := de.derpAddr - if sentAny && sendCallMeMaybe && derpAddr.IsValid() { + // Send a CallMeMaybe over DERP when we either pinged a UDP candidate + // (sentAny) or have no UDP candidates at all but do know a DERP home. The + // latter is the bootstrap case for a peer whose endpoints aren't known yet + // (e.g. mid-STUN after a relogin): without soliciting the reverse + // direction it can never establish a path, leaving the node one-way dead + // This is narrower than the reverted #19878: + // when UDP candidates exist we still require sentAny, preserving #20088's + // anti-spam property. + if sendCallMeMaybe && derpAddr.IsValid() && (sentAny || len(de.endpointState) == 0) { // Have our magicsock.Conn figure out its STUN endpoint (if // it doesn't know already) and then send a CallMeMaybe // message to our peer via DERP informing them that we've diff --git a/wgengine/magicsock/endpoint_test.go b/wgengine/magicsock/endpoint_test.go index 593cf1455..cb998c2c2 100644 --- a/wgengine/magicsock/endpoint_test.go +++ b/wgengine/magicsock/endpoint_test.go @@ -687,3 +687,103 @@ func Test_endpoint_handlePongConnLocked(t *testing.T) { }) } } + +// TestDERPOnlyPeerSolicitsCallMeMaybe proves a peer reachable only via DERP +// (no UDP endpoints known yet, e.g. mid-STUN after a relogin) still solicits +// the reverse direction with callMeMaybe. Without this, such a peer can never +// bootstrap a path — magicsock_disco_sent_callmemaybe stays frozen and the +// node is one-way dead — and a control server can strand the client by +// delivering a re-keyed peer endpoint-less. +func TestDERPOnlyPeerSolicitsCallMeMaybe(t *testing.T) { + conn := newTestConn(t) + defer conn.Close() + + de := &endpoint{ + c: conn, + publicKey: key.NewNode().Public(), + sentPing: make(map[stun.TxID]sentPing), + endpointState: make(map[netip.AddrPort]*endpointState), // no UDP candidates + debugUpdates: ringlog.New[EndpointChange](10), + } + de.derpAddr = netip.AddrPortFrom(tailcfg.DerpMagicIPAddr, 1) // reachable only via DERP + de.updateDiscoKey(key.NewDisco().Public()) + + // enqueueCallMeMaybe records onEndpointRefreshed[de] only when the local + // endpoints are stale; otherwise it tries to send immediately over DERP, + // which the test harness can't do. The harness's startup STUN keeps + // refreshing lastEndpointsTime, so backdate it and re-drive the trigger in + // a loop: one iteration where the goroutine sees stale endpoints wins and + // records the callback, proving the callMeMaybe gate let the DERP-only peer + // through. stopPeriodicReSTUNTimerLocked quiets the periodic refresher. + conn.mu.Lock() + conn.stopPeriodicReSTUNTimerLocked() + conn.mu.Unlock() + + var solicited bool + for range 200 { + conn.mu.Lock() + conn.lastEndpointsTime = time.Now().Add(-2 * endpointsFreshEnoughDuration) + conn.mu.Unlock() + + de.mu.Lock() + de.lastFullPing = 0 // allow re-driving past discoPingInterval + de.sendDiscoPingsLocked(mono.Now(), true) + de.mu.Unlock() + + conn.mu.Lock() + _, solicited = conn.onEndpointRefreshed[de] + conn.mu.Unlock() + if solicited { + break + } + time.Sleep(5 * time.Millisecond) + } + if !solicited { + t.Fatal("DERP-only peer with no UDP endpoints did not solicit callMeMaybe") + } +} + +// TestUpsertDERPOnlyPeerKicksDisco proves that adding a peer via the delta path +// (UpsertPeer) with a DERP home but no known UDP endpoints initiates disco +// solicitation without waiting for outbound WireGuard traffic. Previously disco +// was only armed off a wireguard endpoint.send(); a re-keyed, endpoint-less peer +// delivered via UpsertPeer therefore never solicited callMeMaybe and stayed +// one-way dead until traffic happened to flow. +func TestUpsertDERPOnlyPeerKicksDisco(t *testing.T) { + conn := newTestConn(t) + defer conn.Close() + conn.SetPrivateKey(key.NewNode()) + + nv := (&tailcfg.Node{ + ID: 1, + Key: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + HomeDERP: 1, // reachable via DERP + Endpoints: nil, // but no UDP candidates known yet + }).View() + + // The real trigger: nothing else, no manual sendDiscoPingsLocked call. + conn.UpsertPeer(nv) + + conn.mu.Lock() + de, ok := conn.peerMap.endpointForNodeID(nv.ID()) + conn.mu.Unlock() + if !ok { + t.Fatal("UpsertPeer did not create an endpoint") + } + + // sendDiscoPingsLocked sets lastFullPing and noteTxActivityExtTriggerLocked + // arms heartBeatTimer; both being set proves disco was kicked purely from + // the add, with no outbound WireGuard traffic. These reads are deterministic + // and independent of DERP/STUN being functional in the test harness. + de.mu.Lock() + pinged := !de.lastFullPing.IsZero() + armed := de.heartBeatTimer != nil + de.mu.Unlock() + if !pinged { + t.Fatal("UpsertPeer of a DERP-only peer did not kick sendDiscoPingsLocked (lastFullPing still zero)") + } + if !armed { + t.Fatal("UpsertPeer of a DERP-only peer did not arm the heartbeat") + } +} diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 398803faf..7aa9280ce 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -3322,6 +3322,16 @@ func (c *Conn) UpsertPeer(n tailcfg.NodeView) { mak.Set(&c.peersByID, n.ID(), n) c.upsertPeerLocked(n, flags, debugRingBufferSize(len(c.peersByID))) + // A peer added or re-keyed via this delta path won't have disco armed if no + // traffic is flowing toward it yet (arming normally happens off an outbound + // endpoint.send). Kick disco here for a peer that lacks a trusted path so it + // starts initiating instead of staying one-way dead until traffic flows + // maybeKickDiscoBootstrap no-ops for peers that + // already have a trusted path. + if ep, ok := c.peerMap.endpointForNodeID(n.ID()); ok { + ep.maybeKickDiscoBootstrap() + } + var relayUpsert candidatePeerRelay relayQualifies := false if c.relayClientEnabled {