diff --git a/cmd/tailscale/cli/netcheck.go b/cmd/tailscale/cli/netcheck.go index 1a38870c7..f8bb906ef 100644 --- a/cmd/tailscale/cli/netcheck.go +++ b/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) diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index 396081739..6f8e08eff 100644 --- a/net/netcheck/netcheck.go +++ b/net/netcheck/netcheck.go @@ -64,9 +64,6 @@ const ( // 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 - sentHairCheck bool - report *Report // to be returned by GetReport - inFlight map[stun.TxID]func(netip.AddrPort) // called without c.mu held - gotEP4 netip.AddrPort - timers []*time.Timer + mu sync.Mutex + report *Report // to be returned by GetReport + inFlight map[stun.TxID]func(netip.AddrPort) // called without c.mu held + gotEP4 netip.AddrPort + 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 { diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go index 4cbb06be2..1e27ba12c 100644 --- a/net/netcheck/netcheck_test.go +++ b/net/netcheck/netcheck_test.go @@ -20,142 +20,12 @@ import ( "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, - start: time.Now(), - report: newReport(), - sentHairCheck: true, // prevent hair check start, not relevant here + c: c, + start: time.Now(), + report: newReport(), } 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 { diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 4f1617140..87f6ca164 100644 --- a/wgengine/magicsock/magicsock.go +++ b/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,