net/netcheck: remove hairpin probes

Palo Alto reported interpreting hairpin probes as LAND attacks, and the
firewalls may be responding to this by shutting down otherwise in use NAT sessions
prematurely. We don't currently make use of the outcome of the hairpin
probes, and they contribute to other user confusion with e.g. the
AirPort Extreme hairpin session workaround. We decided in response to
remove the whole probe feature as a result.

Updates #188
Updates tailscale/corp#19106
Updates tailscale/corp#19116

Signed-off-by: James Tucker <james@tailscale.com>

cmd/tailscale/cli/netcheck.go

@@ -142,7 +142,6 @@ func printReport(dm *tailcfg.DERPMap, report *netcheck.Report) error {
 		printf("\t* IPv6: no, unavailable in OS\n")
 	}
 	printf("\t* MappingVariesByDestIP: %v\n", report.MappingVariesByDestIP)
-	printf("\t* HairPinning: %v\n", report.HairPinning)
 	printf("\t* PortMapping: %v\n", portMapping(report))
 	if report.CaptivePortal != "" {
 		printf("\t* CaptivePortal: %v\n", report.CaptivePortal)

net/netcheck/netcheck.go

@@ -64,9 +64,6 @@
 	// icmpProbeTimeout is the maximum amount of time netcheck will spend
 	// probing with ICMP packets.
 	icmpProbeTimeout = 1 * time.Second
-	// hairpinCheckTimeout is the amount of time we wait for a
-	// hairpinned packet to come back.
-	hairpinCheckTimeout = 100 * time.Millisecond
 	// defaultActiveRetransmitTime is the retransmit interval we use
 	// for STUN probes when we're in steady state (not in start-up),
 	// but don't have previous latency information for a DERP
@@ -96,11 +93,6 @@ type Report struct {
 	// STUN server you're talking to (on IPv4).
 	MappingVariesByDestIP opt.Bool
 
-	// HairPinning is whether the router supports communicating
-	// between two local devices through the NATted public IP address
-	// (on IPv4).
-	HairPinning opt.Bool
-
 	// UPnP is whether UPnP appears present on the LAN.
 	// Empty means not checked.
 	UPnP opt.Bool
@@ -287,23 +279,6 @@ func (c *Client) vlogf(format string, a ...any) {
 	}
 }
 
-// handleHairSTUN reports whether pkt (from src) was our magic hairpin
-// probe packet that we sent to ourselves.
-func (c *Client) handleHairSTUNLocked(pkt []byte, src netip.AddrPort) bool {
-	rs := c.curState
-	if rs == nil {
-		return false
-	}
-	if tx, err := stun.ParseBindingRequest(pkt); err == nil && tx == rs.hairTX {
-		select {
-		case rs.gotHairSTUN <- src:
-		default:
-		}
-		return true
-	}
-	return false
-}
-
 // MakeNextReportFull forces the next GetReport call to be a full
 // (non-incremental) probe of all DERP regions.
 func (c *Client) MakeNextReportFull() {
@@ -326,10 +301,6 @@ func (c *Client) ReceiveSTUNPacket(pkt []byte, src netip.AddrPort) {
 	}
 
 	c.mu.Lock()
-	if c.handleHairSTUNLocked(pkt, src) {
-		c.mu.Unlock()
-		return
-	}
 	rs := c.curState
 	c.mu.Unlock()
 
@@ -340,6 +311,8 @@ func (c *Client) ReceiveSTUNPacket(pkt []byte, src netip.AddrPort) {
 	tx, addrPort, err := stun.ParseResponse(pkt)
 	if err != nil {
 		if _, err := stun.ParseBindingRequest(pkt); err == nil {
+			// We no longer send hairpin checks, but perhaps we might catch a
+			// stray from earlier versions.
 			// This was probably our own netcheck hairpin
 			// check probe coming in late. Ignore.
 			return
@@ -565,20 +538,15 @@ type reportState struct {
 	c           *Client
 	start       time.Time
 	opts        *GetReportOpts
-	hairTX      stun.TxID
-	gotHairSTUN chan netip.AddrPort
-	hairTimeout chan struct{} // closed on timeout
-	pc4Hair     nettype.PacketConn
 	incremental bool // doing a lite, follow-up netcheck
 	stopProbeCh chan struct{}
 	waitPortMap sync.WaitGroup
 
-	mu            sync.Mutex
+	mu       sync.Mutex
-	sentHairCheck bool
+	report   *Report                            // to be returned by GetReport
-	report        *Report                            // to be returned by GetReport
+	inFlight map[stun.TxID]func(netip.AddrPort) // called without c.mu held
-	inFlight      map[stun.TxID]func(netip.AddrPort) // called without c.mu held
+	gotEP4   netip.AddrPort
-	gotEP4        netip.AddrPort
+	timers   []*time.Timer
-	timers        []*time.Timer
 }
 
 func (rs *reportState) anyUDP() bool {
@@ -628,50 +596,6 @@ func (rs *reportState) probeWouldHelp(probe probe, node *tailcfg.DERPNode) bool
 	return false
 }
 
-func (rs *reportState) startHairCheckLocked(dst netip.AddrPort) {
-	if rs.sentHairCheck || rs.incremental {
-		return
-	}
-	rs.sentHairCheck = true
-	rs.pc4Hair.WriteToUDPAddrPort(stun.Request(rs.hairTX), dst)
-	rs.c.vlogf("sent haircheck to %v", dst)
-	time.AfterFunc(hairpinCheckTimeout, func() { close(rs.hairTimeout) })
-}
-
-func (rs *reportState) waitHairCheck(ctx context.Context) {
-	rs.mu.Lock()
-	defer rs.mu.Unlock()
-	ret := rs.report
-	if rs.incremental {
-		if rs.c.last != nil {
-			ret.HairPinning = rs.c.last.HairPinning
-		}
-		return
-	}
-	if !rs.sentHairCheck {
-		return
-	}
-
-	// First, check whether we have a value before we check for timeouts.
-	select {
-	case <-rs.gotHairSTUN:
-		ret.HairPinning.Set(true)
-		return
-	default:
-	}
-
-	// Now, wait for a response or a timeout.
-	select {
-	case <-rs.gotHairSTUN:
-		ret.HairPinning.Set(true)
-	case <-rs.hairTimeout:
-		rs.c.vlogf("hairCheck timeout")
-		ret.HairPinning.Set(false)
-	case <-ctx.Done():
-		rs.c.vlogf("hairCheck context timeout")
-	}
-}
-
 func (rs *reportState) stopTimers() {
 	rs.mu.Lock()
 	defer rs.mu.Unlock()
@@ -720,7 +644,6 @@ func (rs *reportState) addNodeLatency(node *tailcfg.DERPNode, ipp netip.AddrPort
 		if !rs.gotEP4.IsValid() {
 			rs.gotEP4 = ipp
 			ret.GlobalV4 = ipp
-			rs.startHairCheckLocked(ipp)
 		} else {
 			if rs.gotEP4 != ipp {
 				ret.MappingVariesByDestIP.Set(true)
@@ -834,9 +757,6 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 		opts:        opts,
 		report:      newReport(),
 		inFlight:    map[stun.TxID]func(netip.AddrPort){},
-		hairTX:      stun.NewTxID(), // random payload
-		gotHairSTUN: make(chan netip.AddrPort, 1),
-		hairTimeout: make(chan struct{}),
 		stopProbeCh: make(chan struct{}, 1),
 	}
 	c.curState = rs
@@ -894,34 +814,11 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 		v6udp.Close()
 	}
 
-	// Create a UDP4 socket used for sending to our discovered IPv4 address.
-	rs.pc4Hair, err = nettype.MakePacketListenerWithNetIP(netns.Listener(c.logf, c.NetMon)).ListenPacket(ctx, "udp4", ":0")
-	if err != nil {
-		c.logf("udp4: %v", err)
-		return nil, err
-	}
-	defer rs.pc4Hair.Close()
-
 	if !c.SkipExternalNetwork && c.PortMapper != nil {
 		rs.waitPortMap.Add(1)
 		go rs.probePortMapServices()
 	}
 
-	// At least the Apple Airport Extreme doesn't allow hairpin
-	// sends from a private socket until it's seen traffic from
-	// that src IP:port to something else out on the internet.
-	//
-	// See https://github.com/tailscale/tailscale/issues/188#issuecomment-600728643
-	//
-	// And it seems that even sending to a likely-filtered RFC 5737
-	// documentation-only IPv4 range is enough to set up the mapping.
-	// So do that for now. In the future we might want to classify networks
-	// that do and don't require this separately. But for now help it.
-	const documentationIP = "203.0.113.1"
-	rs.pc4Hair.WriteToUDPAddrPort(
-		[]byte("tailscale netcheck; see https://github.com/tailscale/tailscale/issues/188"),
-		netip.AddrPortFrom(netip.MustParseAddr(documentationIP), 12345))
-
 	plan := makeProbePlan(dm, ifState, last)
 
 	// If we're doing a full probe, also check for a captive portal. We
@@ -999,8 +896,6 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 		captivePortalStop()
 	}
 
-	rs.waitHairCheck(ctx)
-	c.vlogf("hairCheck done")
 	if !c.SkipExternalNetwork && c.PortMapper != nil {
 		rs.waitPortMap.Wait()
 		c.vlogf("portMap done")
@@ -1369,7 +1264,6 @@ func (c *Client) logConciseReport(r *Report, dm *tailcfg.DERPMap) {
 			fmt.Fprintf(w, " v6os=%v", r.OSHasIPv6)
 		}
 		fmt.Fprintf(w, " mapvarydest=%v", r.MappingVariesByDestIP)
-		fmt.Fprintf(w, " hair=%v", r.HairPinning)
 		if r.AnyPortMappingChecked() {
 			fmt.Fprintf(w, " portmap=%v%v%v", conciseOptBool(r.UPnP, "U"), conciseOptBool(r.PMP, "M"), conciseOptBool(r.PCP, "C"))
 		} else {

net/netcheck/netcheck_test.go

@@ -20,142 +20,12 @@
 	"time"
 
 	"tailscale.com/net/netmon"
-	"tailscale.com/net/stun"
 	"tailscale.com/net/stun/stuntest"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
 	"tailscale.com/tstest/nettest"
 )
 
-func TestHairpinSTUN(t *testing.T) {
-	tx := stun.NewTxID()
-	c := &Client{
-		curState: &reportState{
-			hairTX:      tx,
-			gotHairSTUN: make(chan netip.AddrPort, 1),
-		},
-	}
-	req := stun.Request(tx)
-	if !stun.Is(req) {
-		t.Fatal("expected STUN message")
-	}
-	if !c.handleHairSTUNLocked(req, netip.AddrPort{}) {
-		t.Fatal("expected true")
-	}
-	select {
-	case <-c.curState.gotHairSTUN:
-	default:
-		t.Fatal("expected value")
-	}
-}
-
-func TestHairpinWait(t *testing.T) {
-	makeClient := func(t *testing.T) (*Client, *reportState) {
-		tx := stun.NewTxID()
-		c := &Client{}
-		req := stun.Request(tx)
-		if !stun.Is(req) {
-			t.Fatal("expected STUN message")
-		}
-
-		var err error
-		rs := &reportState{
-			c:           c,
-			hairTX:      tx,
-			gotHairSTUN: make(chan netip.AddrPort, 1),
-			hairTimeout: make(chan struct{}),
-			report:      newReport(),
-		}
-		rs.pc4Hair, err = net.ListenUDP("udp4", &net.UDPAddr{
-			IP:   net.ParseIP("127.0.0.1"),
-			Port: 0,
-		})
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		c.curState = rs
-		return c, rs
-	}
-
-	ll, err := net.ListenPacket("udp", "localhost:0")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer ll.Close()
-	dstAddr := netip.MustParseAddrPort(ll.LocalAddr().String())
-
-	t.Run("Success", func(t *testing.T) {
-		c, rs := makeClient(t)
-		req := stun.Request(rs.hairTX)
-
-		// Start a hairpin check to ourselves.
-		rs.startHairCheckLocked(dstAddr)
-
-		// Fake receiving the stun check from ourselves after some period of time.
-		src := netip.MustParseAddrPort(rs.pc4Hair.LocalAddr().String())
-		c.handleHairSTUNLocked(req, src)
-
-		rs.waitHairCheck(context.Background())
-
-		// Verify that we set HairPinning
-		if got := rs.report.HairPinning; !got.EqualBool(true) {
-			t.Errorf("wanted HairPinning=true, got %v", got)
-		}
-	})
-
-	t.Run("LateReply", func(t *testing.T) {
-		c, rs := makeClient(t)
-		req := stun.Request(rs.hairTX)
-
-		// Start a hairpin check to ourselves.
-		rs.startHairCheckLocked(dstAddr)
-
-		// Wait until we've timed out, to mimic the race in #1795.
-		<-rs.hairTimeout
-
-		// Fake receiving the stun check from ourselves after some period of time.
-		src := netip.MustParseAddrPort(rs.pc4Hair.LocalAddr().String())
-		c.handleHairSTUNLocked(req, src)
-
-		// Wait for a hairpin response
-		rs.waitHairCheck(context.Background())
-
-		// Verify that we set HairPinning
-		if got := rs.report.HairPinning; !got.EqualBool(true) {
-			t.Errorf("wanted HairPinning=true, got %v", got)
-		}
-	})
-
-	t.Run("Timeout", func(t *testing.T) {
-		_, rs := makeClient(t)
-
-		// Start a hairpin check to ourselves.
-		rs.startHairCheckLocked(dstAddr)
-
-		ctx, cancel := context.WithTimeout(context.Background(), hairpinCheckTimeout*50)
-		defer cancel()
-
-		// Wait in the background
-		waitDone := make(chan struct{})
-		go func() {
-			rs.waitHairCheck(ctx)
-			close(waitDone)
-		}()
-
-		// If we do nothing, then we time out; confirm that we set
-		// HairPinning to false in this case.
-		select {
-		case <-waitDone:
-			if got := rs.report.HairPinning; !got.EqualBool(false) {
-				t.Errorf("wanted HairPinning=false, got %v", got)
-			}
-		case <-ctx.Done():
-			t.Fatalf("timed out waiting for hairpin channel")
-		}
-	})
-}
-
 func newTestClient(t testing.TB) *Client {
 	c := &Client{
 		NetMon: netmon.NewStatic(),
@@ -211,10 +81,9 @@ func TestMultiGlobalAddressMapping(t *testing.T) {
 	}
 
 	rs := &reportState{
-		c:             c,
+		c:      c,
-		start:         time.Now(),
+		start:  time.Now(),
-		report:        newReport(),
+		report: newReport(),
-		sentHairCheck: true, // prevent hair check start, not relevant here
 	}
 	derpNode := &tailcfg.DERPNode{}
 	port1 := netip.MustParseAddrPort("127.0.0.1:1234")
@@ -784,12 +653,12 @@ func TestLogConciseReport(t *testing.T) {
 		{
 			name: "no_udp",
 			r:    &Report{},
-			want: "udp=false v4=false icmpv4=false v6=false mapvarydest= hair= portmap=? derp=0",
+			want: "udp=false v4=false icmpv4=false v6=false mapvarydest= portmap=? derp=0",
 		},
 		{
 			name: "no_udp_icmp",
 			r:    &Report{ICMPv4: true, IPv4: true},
-			want: "udp=false icmpv4=true v6=false mapvarydest= hair= portmap=? derp=0",
+			want: "udp=false icmpv4=true v6=false mapvarydest= portmap=? derp=0",
 		},
 		{
 			name: "ipv4_one_region",
@@ -804,7 +673,7 @@ func TestLogConciseReport(t *testing.T) {
 					1: 10 * ms,
 				},
 			},
-			want: "udp=true v6=false mapvarydest= hair= portmap=? derp=1 derpdist=1v4:10ms",
+			want: "udp=true v6=false mapvarydest= portmap=? derp=1 derpdist=1v4:10ms",
 		},
 		{
 			name: "ipv4_all_region",
@@ -823,7 +692,7 @@ func TestLogConciseReport(t *testing.T) {
 					3: 30 * ms,
 				},
 			},
-			want: "udp=true v6=false mapvarydest= hair= portmap=? derp=1 derpdist=1v4:10ms,2v4:20ms,3v4:30ms",
+			want: "udp=true v6=false mapvarydest= portmap=? derp=1 derpdist=1v4:10ms,2v4:20ms,3v4:30ms",
 		},
 		{
 			name: "ipboth_all_region",
@@ -848,7 +717,7 @@ func TestLogConciseReport(t *testing.T) {
 					3: 30 * ms,
 				},
 			},
-			want: "udp=true v6=true mapvarydest= hair= portmap=? derp=1 derpdist=1v4:10ms,1v6:10ms,2v4:20ms,2v6:20ms,3v4:30ms,3v6:30ms",
+			want: "udp=true v6=true mapvarydest= portmap=? derp=1 derpdist=1v4:10ms,1v6:10ms,2v4:20ms,2v6:20ms,3v4:30ms,3v6:30ms",
 		},
 		{
 			name: "portmap_all",
@@ -858,7 +727,7 @@ func TestLogConciseReport(t *testing.T) {
 				PMP:  "true",
 				PCP:  "true",
 			},
-			want: "udp=true v4=false v6=false mapvarydest= hair= portmap=UMC derp=0",
+			want: "udp=true v4=false v6=false mapvarydest= portmap=UMC derp=0",
 		},
 		{
 			name: "portmap_some",
@@ -868,7 +737,7 @@ func TestLogConciseReport(t *testing.T) {
 				PMP:  "false",
 				PCP:  "true",
 			},
-			want: "udp=true v4=false v6=false mapvarydest= hair= portmap=UC derp=0",
+			want: "udp=true v4=false v6=false mapvarydest= portmap=UC derp=0",
 		},
 	}
 	for _, tt := range tests {

wgengine/magicsock/magicsock.go

@@ -687,7 +687,6 @@ func (c *Conn) updateNetInfo(ctx context.Context) (*netcheck.Report, error) {
 	ni := &tailcfg.NetInfo{
 		DERPLatency:           map[string]float64{},
 		MappingVariesByDestIP: report.MappingVariesByDestIP,
-		HairPinning:           report.HairPinning,
 		UPnP:                  report.UPnP,
 		PMP:                   report.PMP,
 		PCP:                   report.PCP,