Don't hole punch if either peer is behind a Symmetric NAT

go.mod

@@ -3,7 +3,7 @@ module github.com/libp2p/go-libp2p
 go 1.12
 
 require (
-	github.com/gogo/protobuf v1.3.1
+	github.com/gogo/protobuf v1.3.2
 	github.com/ipfs/go-cid v0.0.7
 	github.com/ipfs/go-datastore v0.4.5
 	github.com/ipfs/go-detect-race v0.0.1
@@ -23,7 +23,7 @@ require (
 	github.com/libp2p/go-libp2p-mplex v0.4.1
 	github.com/libp2p/go-libp2p-nat v0.0.6
 	github.com/libp2p/go-libp2p-netutil v0.1.0
-	github.com/libp2p/go-libp2p-noise v0.1.1
+	github.com/libp2p/go-libp2p-noise v0.2.0
 	github.com/libp2p/go-libp2p-peerstore v0.2.6
 	github.com/libp2p/go-libp2p-swarm v0.4.3
 	github.com/libp2p/go-libp2p-testing v0.4.0
@@ -39,7 +39,7 @@ require (
 	github.com/multiformats/go-multiaddr v0.3.1
 	github.com/multiformats/go-multiaddr-dns v0.2.0
 	github.com/multiformats/go-multistream v0.2.1
-	github.com/stretchr/testify v1.6.1
+	github.com/stretchr/testify v1.7.0
 	github.com/whyrusleeping/mdns v0.0.0-20190826153040-b9b60ed33aa9
 	go.uber.org/zap v1.16.0 // indirect
 )

p2p/host/basic/basic_host.go

@@ -212,6 +212,8 @@ func NewHost(ctx context.Context, n network.Network, opts *HostOpts) (*BasicHost
 		h.mux = opts.MultistreamMuxer
 	}
 
+	h.Peerstore().Put(h.ID(), identify.UDPNATDeviceTypeKey, network.NATDeviceTypeUnknown)
+	h.Peerstore().Put(h.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeUnknown)
 	// we can't set this as a default above because it depends on the *BasicHost.
 	if h.disableSignedPeerRecord {
 		h.ids, err = identify.NewIDService(h, identify.UserAgent(opts.UserAgent), identify.DisableSignedPeerRecord())

p2p/protocol/holepunch/coordination.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	logging "github.com/ipfs/go-log"
+	"github.com/libp2p/go-libp2p-core/event"
 	"github.com/libp2p/go-libp2p-core/host"
 	"github.com/libp2p/go-libp2p-core/network"
 	"github.com/libp2p/go-libp2p-core/peer"
@@ -25,6 +26,8 @@ var (
 	Protocol protocol.ID = "/libp2p/holepunch/1.0.0"
 	// HolePunchTimeout is the timeout for the hole punch protocol stream.
 	HolePunchTimeout = 1 * time.Minute
+	// ErrNATHolePunchingUnsupported means hole punching is NOT supported by the NAT on either or both peers.
+	ErrNATHolePunchingUnsupported = errors.New("NAT does NOT support hole punching")
 
 	maxMsgSize  = 4 * 1024 // 4K
 	dialTimeout = 5 * time.Second
@@ -75,11 +78,55 @@ func NewHolePunchService(h host.Host, ids *identify.IDService, isTest bool) (*Ho
 		isTest:    isTest,
 	}
 
+	sub, err := h.EventBus().Subscribe(new(event.EvtNATDeviceTypeChanged))
+	if err != nil {
+		return nil, err
+	}
+
 	h.SetStreamHandler(Protocol, hs.handleNewStream)
 	h.Network().Notify((*netNotifiee)(hs))
+
+	hs.refCount.Add(1)
+	go hs.loop(sub)
+
 	return hs, nil
 }
 
+func (hs *HolePunchService) loop(sub event.Subscription) {
+	defer hs.refCount.Done()
+	defer sub.Close()
+
+	for {
+		select {
+		// Our local NAT device types are intialized in the peerstore when the Host is created
+		//	and updated in the peerstore by the Observed Address Manager.
+		case _, ok := <-sub.Out():
+			if !ok {
+				return
+			}
+
+			if hs.PeerSupportsHolePunching(hs.host.ID(), hs.host.Addrs()) {
+				hs.host.SetStreamHandler(Protocol, hs.handleNewStream)
+			} else {
+				hs.host.RemoveStreamHandler(Protocol)
+			}
+
+		case <-hs.ctx.Done():
+			return
+		}
+	}
+}
+
+func hasProtoAddr(protocCode int, addrs []ma.Multiaddr) bool {
+	for _, a := range addrs {
+		if _, err := a.ValueForProtocol(protocCode); err == nil {
+			return true
+		}
+	}
+
+	return false
+}
+
 // Close closes the Hole Punch Service.
 func (hs *HolePunchService) Close() error {
 	hs.closeSync.Do(func() {
@@ -108,6 +155,12 @@ func (hs *HolePunchService) HolePunch(rp peer.ID) error {
 		}
 	}
 
+	// return if either peer does NOT support hole punching
+	if !hs.PeerSupportsHolePunching(rp, hs.host.Peerstore().Addrs(rp)) ||
+		!hs.PeerSupportsHolePunching(hs.host.ID(), hs.host.Addrs()) {
+		return ErrNATHolePunchingUnsupported
+	}
+
 	// hole punch
 	hpCtx := network.WithUseTransient(hs.ctx, "hole-punch")
 	sCtx := network.WithNoDial(hpCtx, "hole-punch")
@@ -203,6 +256,40 @@ func (hs *HolePunchService) appendHandlerErr(err error) {
 	}
 }
 
+// PeerSupportsHolePunching returns true if the given peer with the given addresses supports hole punching.
+// It uses the peer's NAT device type detected via Identify.
+// We can hole punch with a peer ONLY if it is NOT behind a symmetric NAT for all the transport protocol it supports.
+func (hs *HolePunchService) PeerSupportsHolePunching(p peer.ID, addrs []ma.Multiaddr) bool {
+	udpSupported := hasProtoAddr(ma.P_UDP, addrs)
+	tcpSupported := hasProtoAddr(ma.P_TCP, addrs)
+
+	if udpSupported {
+		udpNAT, err := hs.host.Peerstore().Get(p, identify.UDPNATDeviceTypeKey)
+		if err != nil {
+			return false
+		}
+		udpNatType := udpNAT.(network.NATDeviceType)
+
+		if udpNatType == network.NATDeviceTypeCone || udpNatType == network.NATDeviceTypeUnknown {
+			return true
+		}
+	}
+
+	if tcpSupported {
+		tcpNAT, err := hs.host.Peerstore().Get(p, identify.TCPNATDeviceTypeKey)
+		if err != nil {
+			return false
+		}
+
+		tcpNATType := tcpNAT.(network.NATDeviceType)
+		if tcpNATType == network.NATDeviceTypeCone || tcpNATType == network.NATDeviceTypeUnknown {
+			return true
+		}
+	}
+
+	return false
+}
+
 func (hs *HolePunchService) handleNewStream(s network.Stream) {
 	log.Infof("got hole punch request from peer %s", s.Conn().RemotePeer().Pretty())
 	_ = s.SetDeadline(time.Now().Add(HolePunchTimeout))

p2p/protocol/holepunch/coordination_test.go

@@ -8,14 +8,14 @@ import (
 
 	"github.com/libp2p/go-libp2p"
 	circuit "github.com/libp2p/go-libp2p-circuit"
+	"github.com/libp2p/go-libp2p-core/event"
 	"github.com/libp2p/go-libp2p-core/host"
 	"github.com/libp2p/go-libp2p-core/network"
 	"github.com/libp2p/go-libp2p-core/peer"
 	"github.com/libp2p/go-libp2p-core/peerstore"
 	"github.com/libp2p/go-libp2p/p2p/protocol/holepunch"
 	"github.com/libp2p/go-libp2p/p2p/protocol/holepunch/pb"
 	"github.com/libp2p/go-libp2p/p2p/protocol/identify"
-	identify_pb "github.com/libp2p/go-libp2p/p2p/protocol/identify/pb"
 	"github.com/libp2p/go-msgio/protoio"
 	ma "github.com/multiformats/go-multiaddr"
 	manet "github.com/multiformats/go-multiaddr/net"
@@ -229,59 +229,154 @@ func TestFailuresOnResponder(t *testing.T) {
 	}
 }
 
-func TestObservedAddressesAreExchanged(t *testing.T) {
+func TestHolePunchingFailsOnSymmetricNAT(t *testing.T) {
 	ctx := context.Background()
 
-	obsAddrs1 := ma.StringCast("/ip4/8.8.8.8/tcp/1234")
-	obsAddrs2 := ma.StringCast("/ip4/9.8.8.8/tcp/1234")
-
 	h1, h1ps := mkHostWithHolePunchSvc(t, ctx)
 	h2, _ := mkHostWithHolePunchSvc(t, ctx)
+	// connect and wait for identify to exchange NAT types
+	connect(t, ctx, h1, h2)
 
-	// modify identify handlers to send our fake observed addresses
-	h1.SetStreamHandler(identify.ID, func(s network.Stream) {
-		writer := protoio.NewDelimitedWriter(s)
-		msg := new(identify_pb.Identify)
-		msg.ObservedAddr = obsAddrs2.Bytes()
-		writer.WriteMsg(msg)
-		s.Close()
-	})
-
-	h2.SetStreamHandler(identify.ID, func(s network.Stream) {
-		writer := protoio.NewDelimitedWriter(s)
-		msg := new(identify_pb.Identify)
-		msg.ObservedAddr = obsAddrs1.Bytes()
-		writer.WriteMsg(msg)
-		s.Close()
-	})
+	// self peer is behind a Symmetric NAT and remote peer is behind a Cone NAT
+	h1.Peerstore().Put(h1.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeSymmetric)
+	h1.Peerstore().Put(h2.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeCone)
+	require.Equal(t, holepunch.ErrNATHolePunchingUnsupported, h1ps.HolePunch(h2.ID()))
 
-	connect(t, ctx, h1, h2)
+	// remote peer is behind a Symmetric NAT and self is behind a Cone NAT
+	h1.Peerstore().Put(h1.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeCone)
+	h1.Peerstore().Put(h2.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeSymmetric)
+	require.Equal(t, holepunch.ErrNATHolePunchingUnsupported, h1ps.HolePunch(h2.ID()))
+}
 
-	// hole punch so both peers exchange each other's observed addresses and save to peerstore
-	require.NoError(t, h1ps.HolePunch(h2.ID()))
+func TestProtocolHandlerOnNATType(t *testing.T) {
+	ctx := context.Background()
 
+	// Unknown NAT -> Protocol is supported
+	h, _ := mkHostWithHolePunchSvc(t, ctx)
+	em, err := h.EventBus().Emitter(new(event.EvtNATDeviceTypeChanged))
+	require.NoError(t, err)
+	require.Contains(t, h.Mux().Protocols(), string(holepunch.Protocol))
+
+	// Symmetric NAT -> Protocol is NOT supported
+	h.Peerstore().Put(h.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeSymmetric)
+
+	require.NoError(t, em.Emit(event.EvtNATDeviceTypeChanged{
+		TransportProtocol: network.NATTransportTCP,
+		NatDeviceType:     network.NATDeviceTypeSymmetric,
+	}))
 	require.Eventually(t, func() bool {
-		h2Addrs := h1.Peerstore().Addrs(h2.ID())
-		h1Addrs := h2.Peerstore().Addrs(h1.ID())
-
-		b1 := false
-		b2 := false
-		for _, a := range h1Addrs {
-			if a.Equal(obsAddrs1) {
-				b1 = true
-				break
+		for _, p := range h.Mux().Protocols() {
+			if p == string(holepunch.Protocol) {
+				return false
 			}
 		}
+		return true
+	}, 5*time.Second, 200*time.Millisecond)
 
-		for _, a := range h2Addrs {
-			if a.Equal(obsAddrs2) {
-				b2 = true
-				break
+	// Cone NAT -> Protocol is supported
+	h.Peerstore().Put(h.ID(), identify.TCPNATDeviceTypeKey, network.NATDeviceTypeCone)
+
+	require.NoError(t, em.Emit(event.EvtNATDeviceTypeChanged{
+		TransportProtocol: network.NATTransportTCP,
+		NatDeviceType:     network.NATDeviceTypeCone,
+	}))
+	require.Eventually(t, func() bool {
+		for _, p := range h.Mux().Protocols() {
+			if p == string(holepunch.Protocol) {
+				return true
 			}
 		}
+		return false
+	}, 5*time.Second, 200*time.Millisecond)
+}
+
+func TestPeerSupportsHolePunching(t *testing.T) {
+	ctx := context.Background()
+
+	tcs := map[string]struct {
+		udpSupported         bool
+		tcpSupported         bool
+		udpNat               network.NATDeviceType
+		tcpNat               network.NATDeviceType
+		supportsHolePunching bool
+	}{
+		"udp/tcp supported and symmetric for both -> no hole punching": {
+			udpSupported:         true,
+			tcpSupported:         true,
+			udpNat:               network.NATDeviceTypeSymmetric,
+			tcpNat:               network.NATDeviceTypeSymmetric,
+			supportsHolePunching: false,
+		},
+		"tcp supported and symmetric -> no hole punching": {
+			tcpSupported:         true,
+			tcpNat:               network.NATDeviceTypeSymmetric,
+			udpNat:               network.NATDeviceTypeCone,
+			supportsHolePunching: false,
+		},
+		"udp supported and symmetric -> no hole punching": {
+			udpSupported:         true,
+			udpNat:               network.NATDeviceTypeSymmetric,
+			tcpNat:               network.NATDeviceTypeCone,
+			supportsHolePunching: false,
+		},
+		"udp/tcp supported and cone for both -> hole punching possible": {
+			udpSupported:         true,
+			tcpSupported:         true,
+			udpNat:               network.NATDeviceTypeCone,
+			tcpNat:               network.NATDeviceTypeCone,
+			supportsHolePunching: true,
+		},
+		"tcp supported and cone -> hole punching possible": {
+			tcpSupported:         true,
+			tcpNat:               network.NATDeviceTypeCone,
+			udpNat:               network.NATDeviceTypeSymmetric,
+			supportsHolePunching: true,
+		},
+		"udp supported and cone -> hole punching possible": {
+			udpSupported:         true,
+			udpNat:               network.NATDeviceTypeCone,
+			tcpNat:               network.NATDeviceTypeSymmetric,
+			supportsHolePunching: true,
+		},
+		"udp/tcp supported and unknown for both -> hole punching possible": {
+			udpSupported:         true,
+			tcpSupported:         true,
+			udpNat:               network.NATDeviceTypeUnknown,
+			tcpNat:               network.NATDeviceTypeUnknown,
+			supportsHolePunching: true,
+		},
+		"tcp supported and unknown -> hole punching possible": {
+			tcpSupported:         true,
+			tcpNat:               network.NATDeviceTypeUnknown,
+			udpNat:               network.NATDeviceTypeSymmetric,
+			supportsHolePunching: true,
+		},
+		"udp supported and unknown -> hole punching possible": {
+			udpSupported:         true,
+			udpNat:               network.NATDeviceTypeUnknown,
+			tcpNat:               network.NATDeviceTypeSymmetric,
+			supportsHolePunching: true,
+		},
+	}
 
-		return b1 && b2
-	}, 2*time.Second, 100*time.Millisecond)
+	for name, tc := range tcs {
+		t.Run(name, func(t *testing.T) {
+			h, hps := mkHostWithHolePunchSvc(t, ctx)
+			var addrs []ma.Multiaddr
+			if tc.udpSupported {
+				addrs = append(addrs, ma.StringCast("/ip4/8.8.8.8/udp/1234/quic"))
+			}
+
+			if tc.tcpSupported {
+				addrs = append(addrs, ma.StringCast("/ip4/8.8.8.8/tcp/1234"))
+			}
+
+			h.Peerstore().Put(h.ID(), identify.TCPNATDeviceTypeKey, tc.tcpNat)
+			h.Peerstore().Put(h.ID(), identify.UDPNATDeviceTypeKey, tc.udpNat)
+
+			require.Equal(t, tc.supportsHolePunching, hps.PeerSupportsHolePunching(h.ID(), addrs))
+		})
+	}
 }
 
 func ensureNoHolePunchingStream(t *testing.T, h1, h2 host.Host) {
@@ -340,10 +435,6 @@ func ensureDirectConn(t *testing.T, h1, h2 host.Host) {
 	}, 5*time.Second, 200*time.Millisecond)
 }
 
-func TestNoHolePunchingIfDirectConnAlreadyExists(t *testing.T) {
-
-}
-
 func connect(t *testing.T, ctx context.Context, h1, h2 host.Host) network.Conn {
 	require.NoError(t, h1.Connect(ctx, peer.AddrInfo{
 		ID:    h2.ID(),
@@ -384,13 +475,13 @@ func mkHostWithStaticAutoRelay(t *testing.T, ctx context.Context, relay host.Hos
 }
 
 func mkRelay(t *testing.T, ctx context.Context) host.Host {
-	h, err := libp2p.New(ctx, libp2p.EnableRelay(circuit.OptHop))
+	h, err := libp2p.New(ctx, libp2p.EnableRelay(circuit.OptHop), libp2p.ForceReachabilityPrivate())
 	require.NoError(t, err)
 	return h
 }
 
 func mkHostWithHolePunchSvc(t *testing.T, ctx context.Context) (host.Host, *holepunch.HolePunchService) {
-	h, err := libp2p.New(ctx)
+	h, err := libp2p.New(ctx, libp2p.ForceReachabilityPrivate())
 	require.NoError(t, err)
 	ids, err := identify.NewIDService(h)
 	require.NoError(t, err)