From 9351eec3e1e23ef56123ac3b3f196a22e90cf395 Mon Sep 17 00:00:00 2001 From: James Tucker Date: Tue, 21 May 2024 12:18:24 -0700 Subject: [PATCH] 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 --- cmd/tailscale/cli/netcheck.go | 1 - net/netcheck/netcheck.go | 120 ++----------------------- net/netcheck/netcheck_test.go | 151 +++----------------------------- wgengine/magicsock/magicsock.go | 1 - 4 files changed, 17 insertions(+), 256 deletions(-) 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,