wgengine/magicsock: only accept pong sent by CLI ping

When sending a ping from the CLI, only accept a pong that is in reply
to the specific CLI ping we sent.

Updates #311

Signed-off-by: Val <valerie@tailscale.com>

wgengine/magicsock/endpoint.go

@@ -71,8 +71,6 @@ type endpoint struct {
 	endpointState      map[netip.AddrPort]*endpointState
 	isCallMeMaybeEP    map[netip.AddrPort]bool
 
-	pendingCLIPings []pendingCLIPing // any outstanding "tailscale ping" commands running
-
 	// The following fields are related to the new "silent disco"
 	// implementation that's a WIP as of 2022-10-20.
 	// See #540 for background.
@@ -82,11 +80,6 @@ type endpoint struct {
 	isWireguardOnly bool // whether the endpoint is WireGuard only
 }
 
-type pendingCLIPing struct {
-	res *ipnstate.PingResult
-	cb  func(*ipnstate.PingResult)
-}
-
 // endpointDisco is the current disco key and short string for an endpoint. This
 // structure is immutable.
 type endpointDisco struct {
@@ -99,6 +92,8 @@ type sentPing struct {
 	at      mono.Time
 	timer   *time.Timer // timeout timer
 	purpose discoPingPurpose
+	res     *ipnstate.PingResult       // nil unless CLI ping
+	cb      func(*ipnstate.PingResult) // nil unless CLI ping
 }
 
 // endpointState is some state and history for a specific endpoint of
@@ -363,7 +358,7 @@ func (de *endpoint) heartbeat() {
 	udpAddr, _, _ := de.addrForSendLocked(now)
 	if udpAddr.IsValid() {
 		// We have a preferred path. Ping that every 2 seconds.
-		de.startDiscoPingLocked(udpAddr, now, pingHeartbeat, 0)
+		de.startDiscoPingLocked(udpAddr, now, pingHeartbeat, 0, nil, nil)
 	}
 
 	if de.wantFullPingLocked(now) {
@@ -415,22 +410,21 @@ func (de *endpoint) cliPing(res *ipnstate.PingResult, size int, cb func(*ipnstat
 		return
 	}
 
-	de.pendingCLIPings = append(de.pendingCLIPings, pendingCLIPing{res, cb})
-
 	now := mono.Now()
 	udpAddr, derpAddr, _ := de.addrForSendLocked(now)
+
 	if derpAddr.IsValid() {
-		de.startDiscoPingLocked(derpAddr, now, pingCLI, size)
+		de.startDiscoPingLocked(derpAddr, now, pingCLI, size, res, cb)
 	}
 	if udpAddr.IsValid() && now.Before(de.trustBestAddrUntil) {
 		// Already have an active session, so just ping the address we're using.
 		// Otherwise "tailscale ping" results to a node on the local network
 		// can look like they're bouncing between, say 10.0.0.0/9 and the peer's
 		// IPv6 address, both 1ms away, and it's random who replies first.
-		de.startDiscoPingLocked(udpAddr, now, pingCLI, size)
+		de.startDiscoPingLocked(udpAddr, now, pingCLI, size, res, cb)
 	} else {
 		for ep := range de.endpointState {
-			de.startDiscoPingLocked(ep, now, pingCLI, size)
+			de.startDiscoPingLocked(ep, now, pingCLI, size, res, cb)
 		}
 	}
 	de.noteActiveLocked()
@@ -573,8 +567,10 @@ const (
 	pingCLI
 )
 
-// startDiscoPingLocked sends a disco ping to ep in a separate goroutine.
-func (de *endpoint) startDiscoPingLocked(ep netip.AddrPort, now mono.Time, purpose discoPingPurpose, size int) {
+// startDiscoPingLocked sends a disco ping to ep in a separate
+// goroutine. res and cb are for returning the results of CLI pings,
+// otherwise they are nil.
+func (de *endpoint) startDiscoPingLocked(ep netip.AddrPort, now mono.Time, purpose discoPingPurpose, size int, res *ipnstate.PingResult, cb func(*ipnstate.PingResult)) {
 	if runtime.GOOS == "js" {
 		return
 	}
@@ -599,7 +595,10 @@ func (de *endpoint) startDiscoPingLocked(ep netip.AddrPort, now mono.Time, purpo
 		at:      now,
 		timer:   time.AfterFunc(pingTimeoutDuration, func() { de.discoPingTimeout(txid) }),
 		purpose: purpose,
+		res:     res,
+		cb:      cb,
 	}
+
 	logLevel := discoLog
 	if purpose == pingHeartbeat {
 		logLevel = discoVerboseLog
@@ -630,7 +629,7 @@ func (de *endpoint) sendDiscoPingsLocked(now mono.Time, sendCallMeMaybe bool) {
 			de.c.dlogf("[v1] magicsock: disco: send, starting discovery for %v (%v)", de.publicKey.ShortString(), de.discoShort())
 		}
 
-		de.startDiscoPingLocked(ep, now, pingDiscovery, 0)
+		de.startDiscoPingLocked(ep, now, pingDiscovery, 0, nil, nil)
 	}
 	derpAddr := de.derpAddr
 	if sentAny && sendCallMeMaybe && derpAddr.IsValid() {
@@ -915,11 +914,13 @@ func (de *endpoint) handlePongConnLocked(m *disco.Pong, di *discoInfo, src netip
 		}))
 	}
 
-	for _, pp := range de.pendingCLIPings {
-		de.c.populateCLIPingResponseLocked(pp.res, latency, sp.to)
-		go pp.cb(pp.res)
+	// Currently only CLI ping uses this callback.
+	if sp.cb != nil {
+		if sp.purpose == pingCLI {
+			de.c.populateCLIPingResponseLocked(sp.res, latency, sp.to)
+		}
+		go sp.cb(sp.res)
 	}
-	de.pendingCLIPings = nil
 
 	// Promote this pong response to our current best address if it's lower latency.
 	// TODO(bradfitz): decide how latency vs. preference order affects decision
@@ -1134,7 +1135,6 @@ func (de *endpoint) stopAndReset() {
 		de.heartBeatTimer.Stop()
 		de.heartBeatTimer = nil
 	}
-	de.pendingCLIPings = nil
 }
 
 // resetLocked clears all the endpoint's p2p state, reverting it to a

wgengine/magicsock/magicsock.go

@@ -1270,7 +1270,7 @@ func (c *Conn) sendDiscoMessage(dst netip.AddrPort, dstKey key.NodePublic, dstDi
 		// Can't send. (e.g. no IPv6 locally)
 	} else {
 		if !c.networkDown() {
-			c.logf("magicsock: disco: failed to send %T to %v: %v", m, dst, err)
+			c.logf("magicsock: disco: failed to send %v to %v: %v", disco.MessageSummary(m), dst, err)
 		}
 	}
 	return sent, err