wgengine/magicsock: prioritize trusted peer relay paths over untrusted (#16559)

A trusted peer relay path is always better than an untrusted direct or
peer relay path.

Updates tailscale/corp#30412

Signed-off-by: Jordan Whited <jordan@tailscale.com>

wgengine/magicsock/endpoint.go

@@ -106,24 +106,27 @@ type endpoint struct {
 func (de *endpoint) udpRelayEndpointReady(maybeBest addrQuality) {
 	de.mu.Lock()
 	defer de.mu.Unlock()
-
+	now := mono.Now()
-	if maybeBest.relayServerDisco.Compare(de.bestAddr.relayServerDisco) == 0 {
+	curBestAddrTrusted := now.Before(de.trustBestAddrUntil)
-		// TODO(jwhited): add some observability for this case, e.g. did we
+	sameRelayServer := de.bestAddr.vni.isSet() && maybeBest.relayServerDisco.Compare(de.bestAddr.relayServerDisco) == 0
-		//  flip transports during a de.bestAddr transition from untrusted to
+
-		//  trusted?
+	if !curBestAddrTrusted ||
+		sameRelayServer ||
+		betterAddr(maybeBest, de.bestAddr) {
+		// We must set maybeBest as de.bestAddr if:
+		//   1. de.bestAddr is untrusted. betterAddr does not consider
+		//      time-based trust.
+		//   2. maybeBest & de.bestAddr are on the same relay. If the maybeBest
+		//      handshake happened to use a different source address/transport,
+		//      the relay will drop packets from the 'old' de.bestAddr's.
+		//   3. maybeBest is a 'betterAddr'.
 		//
-		// If these are equal we must set maybeBest as bestAddr, otherwise we
+		// TODO(jwhited): add observability around !curBestAddrTrusted and sameRelayServer
-		// could leave a stale bestAddr if it goes over a different
+		// TODO(jwhited): collapse path change logging with endpoint.handlePongConnLocked()
-		// address family or src.
+		de.c.logf("magicsock: disco: node %v %v now using %v mtu=%v", de.publicKey.ShortString(), de.discoShort(), maybeBest.epAddr, maybeBest.wireMTU)
-	} else if !betterAddr(maybeBest, de.bestAddr) {
+		de.setBestAddrLocked(maybeBest)
-		return
+		de.trustBestAddrUntil = now.Add(trustUDPAddrDuration)
 	}
-
-	// Promote maybeBest to bestAddr.
-	// TODO(jwhited): collapse path change logging with endpoint.handlePongConnLocked()
-	de.c.logf("magicsock: disco: node %v %v now using %v mtu=%v", de.publicKey.ShortString(), de.discoShort(), maybeBest.epAddr, maybeBest.wireMTU)
-	de.setBestAddrLocked(maybeBest)
-	de.trustBestAddrUntil = mono.Now().Add(trustUDPAddrDuration)
 }
 
 func (de *endpoint) setBestAddrLocked(v addrQuality) {

wgengine/magicsock/endpoint_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"tailscale.com/tailcfg"
+	"tailscale.com/tstime/mono"
 	"tailscale.com/types/key"
 )
 
@@ -365,3 +366,91 @@ func Test_epAddr_isDirectUDP(t *testing.T) {
 		})
 	}
 }
+
+func Test_endpoint_udpRelayEndpointReady(t *testing.T) {
+	directAddrQuality := addrQuality{epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.1:7")}}
+	peerRelayAddrQuality := addrQuality{epAddr: epAddr{ap: netip.MustParseAddrPort("192.0.2.2:77")}, latency: time.Second}
+	peerRelayAddrQuality.vni.set(1)
+	peerRelayAddrQualityHigherLatencySameServer := addrQuality{
+		epAddr:  epAddr{ap: netip.MustParseAddrPort("192.0.2.3:77"), vni: peerRelayAddrQuality.vni},
+		latency: peerRelayAddrQuality.latency * 10,
+	}
+	peerRelayAddrQualityHigherLatencyDiffServer := addrQuality{
+		epAddr:           epAddr{ap: netip.MustParseAddrPort("192.0.2.3:77"), vni: peerRelayAddrQuality.vni},
+		latency:          peerRelayAddrQuality.latency * 10,
+		relayServerDisco: key.NewDisco().Public(),
+	}
+	peerRelayAddrQualityLowerLatencyDiffServer := addrQuality{
+		epAddr:           epAddr{ap: netip.MustParseAddrPort("192.0.2.4:77"), vni: peerRelayAddrQuality.vni},
+		latency:          peerRelayAddrQuality.latency / 10,
+		relayServerDisco: key.NewDisco().Public(),
+	}
+	peerRelayAddrQualityEqualLatencyDiffServer := addrQuality{
+		epAddr:           epAddr{ap: netip.MustParseAddrPort("192.0.2.4:77"), vni: peerRelayAddrQuality.vni},
+		latency:          peerRelayAddrQuality.latency,
+		relayServerDisco: key.NewDisco().Public(),
+	}
+	tests := []struct {
+		name               string
+		curBestAddr        addrQuality
+		trustBestAddrUntil mono.Time
+		maybeBest          addrQuality
+		wantBestAddr       addrQuality
+	}{
+		{
+			name:               "bestAddr trusted direct",
+			curBestAddr:        directAddrQuality,
+			trustBestAddrUntil: mono.Now().Add(1 * time.Hour),
+			maybeBest:          peerRelayAddrQuality,
+			wantBestAddr:       directAddrQuality,
+		},
+		{
+			name:               "bestAddr untrusted direct",
+			curBestAddr:        directAddrQuality,
+			trustBestAddrUntil: mono.Now().Add(-1 * time.Hour),
+			maybeBest:          peerRelayAddrQuality,
+			wantBestAddr:       peerRelayAddrQuality,
+		},
+		{
+			name:               "maybeBest same relay server higher latency bestAddr trusted",
+			curBestAddr:        peerRelayAddrQuality,
+			trustBestAddrUntil: mono.Now().Add(1 * time.Hour),
+			maybeBest:          peerRelayAddrQualityHigherLatencySameServer,
+			wantBestAddr:       peerRelayAddrQualityHigherLatencySameServer,
+		},
+		{
+			name:               "maybeBest diff relay server higher latency bestAddr trusted",
+			curBestAddr:        peerRelayAddrQuality,
+			trustBestAddrUntil: mono.Now().Add(1 * time.Hour),
+			maybeBest:          peerRelayAddrQualityHigherLatencyDiffServer,
+			wantBestAddr:       peerRelayAddrQuality,
+		},
+		{
+			name:               "maybeBest diff relay server lower latency bestAddr trusted",
+			curBestAddr:        peerRelayAddrQuality,
+			trustBestAddrUntil: mono.Now().Add(1 * time.Hour),
+			maybeBest:          peerRelayAddrQualityLowerLatencyDiffServer,
+			wantBestAddr:       peerRelayAddrQualityLowerLatencyDiffServer,
+		},
+		{
+			name:               "maybeBest diff relay server equal latency bestAddr trusted",
+			curBestAddr:        peerRelayAddrQuality,
+			trustBestAddrUntil: mono.Now().Add(1 * time.Hour),
+			maybeBest:          peerRelayAddrQualityEqualLatencyDiffServer,
+			wantBestAddr:       peerRelayAddrQuality,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			de := &endpoint{
+				c:                  &Conn{logf: func(msg string, args ...any) { return }},
+				bestAddr:           tt.curBestAddr,
+				trustBestAddrUntil: tt.trustBestAddrUntil,
+			}
+			de.udpRelayEndpointReady(tt.maybeBest)
+			if de.bestAddr != tt.wantBestAddr {
+				t.Errorf("de.bestAddr = %v, want %v", de.bestAddr, tt.wantBestAddr)
+			}
+		})
+	}
+}