Fix address advertisement bugs (#974)

* fix: use all interface addresses when we can't find the default route
* fix: don't add advertise unspecified addresses
* fix: resolve addresses before looking up observed addresses
* fix: only advertise global unicast
* fix: filter link-local addresses from advertisement
* test: fix basic host addr tests

p2p/host/basic/basic_host.go

@@ -103,9 +103,9 @@ type BasicHost struct {
 
 	addrChangeChan chan struct{}
 
-	lipMu         sync.RWMutex
+	addrMu                 sync.RWMutex
-	localIPv4Addr ma.Multiaddr
+	filteredInterfaceAddrs []ma.Multiaddr
-	localIPv6Addr ma.Multiaddr
+	allInterfaceAddrs      []ma.Multiaddr
 
 	disableSignedPeerRecord bool
 	signKey                 crypto.PrivKey
@@ -254,27 +254,68 @@ func NewHost(ctx context.Context, n network.Network, opts *HostOpts) (*BasicHost
 }
 
 func (h *BasicHost) updateLocalIpAddr() {
-	h.lipMu.Lock()
+	h.addrMu.Lock()
-	defer h.lipMu.Unlock()
+	defer h.addrMu.Unlock()
+
+	h.filteredInterfaceAddrs = nil
+	h.allInterfaceAddrs = nil
+
+	// Try to use the default ipv4/6 addresses.
 
 	if r, err := netroute.New(); err != nil {
-		log.Debugw("failed to build Router for kernel's routing table", "err", err)
+		log.Debugw("failed to build Router for kernel's routing table", "error", err)
 	} else {
 		if _, _, localIPv4, err := r.Route(net.IPv4zero); err != nil {
-			log.Debugw("failed to fetch local IPv4 address", "err", err)
+			log.Debugw("failed to fetch local IPv4 address", "error", err)
-		} else {
+		} else if localIPv4.IsGlobalUnicast() {
 			maddr, err := manet.FromIP(localIPv4)
 			if err == nil {
-				h.localIPv4Addr = maddr
+				h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, maddr)
 			}
 		}
 
-		if _, _, localIpv6, err := r.Route(net.IPv6unspecified); err != nil {
+		if _, _, localIPv6, err := r.Route(net.IPv6unspecified); err != nil {
-			log.Debugw("failed to fetch local IPv6 address", "err", err)
+			log.Debugw("failed to fetch local IPv6 address", "error", err)
-		} else {
+		} else if localIPv6.IsGlobalUnicast() {
-			maddr, err := manet.FromIP(localIpv6)
+			maddr, err := manet.FromIP(localIPv6)
 			if err == nil {
-				h.localIPv6Addr = maddr
+				h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, maddr)
+			}
+		}
+	}
+
+	// Resolve the interface addresses
+	ifaceAddrs, err := manet.InterfaceMultiaddrs()
+	if err != nil {
+		// This usually shouldn't happen, but we could be in some kind
+		// of funky restricted environment.
+		log.Errorw("failed to resolve local interface addresses", "error", err)
+
+		// Add the loopback addresses to the filtered addrs and use them as the non-filtered addrs.
+		// Then bail. There's nothing else we can do here.
+		h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, manet.IP4Loopback, manet.IP6Loopback)
+		h.allInterfaceAddrs = h.filteredInterfaceAddrs
+		return
+	}
+
+	for _, addr := range ifaceAddrs {
+		// Skip link-local addrs, they're mostly useless.
+		if !manet.IsIP6LinkLocal(addr) {
+			h.allInterfaceAddrs = append(h.allInterfaceAddrs, addr)
+		}
+	}
+
+	// If netroute failed to get us any interface addresses, use all of
+	// them.
+	if len(h.filteredInterfaceAddrs) == 0 {
+		// Add all addresses.
+		h.filteredInterfaceAddrs = h.allInterfaceAddrs
+	} else {
+		// Only add loopback addresses. Filter these because we might
+		// not _have_ an IPv6 loopback address.
+		for _, addr := range h.allInterfaceAddrs {
+			if manet.IsIPLoopback(addr) {
+				h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, addr)
 			}
 		}
 	}
@@ -747,35 +788,21 @@ func dedupAddrs(addrs []ma.Multiaddr) (uniqueAddrs []ma.Multiaddr) {
 // AllAddrs returns all the addresses of BasicHost at this moment in time.
 // It's ok to not include addresses if they're not available to be used now.
 func (h *BasicHost) AllAddrs() []ma.Multiaddr {
-	h.lipMu.RLock()
+	h.addrMu.RLock()
-	localIPv4Addr := h.localIPv4Addr
+	filteredIfaceAddrs := h.filteredInterfaceAddrs
-	localIPv6Addr := h.localIPv6Addr
+	allIfaceAddrs := h.allInterfaceAddrs
-	h.lipMu.RUnlock()
+	h.addrMu.RUnlock()
-
-	ifaceAddrs := []ma.Multiaddr{manet.IP4Loopback, manet.IP6Loopback}
-	if localIPv4Addr != nil {
-		ifaceAddrs = append(ifaceAddrs, localIPv4Addr)
-	}
-	if localIPv6Addr != nil {
-		ifaceAddrs = append(ifaceAddrs, localIPv6Addr)
-	}
 
 	// Iterate over all _unresolved_ listen addresses, resolving our primary
 	// interface only to avoid advertising too many addresses.
 	listenAddrs := h.Network().ListenAddresses()
 	var finalAddrs []ma.Multiaddr
-	for _, addr := range listenAddrs {
+	if resolved, err := addrutil.ResolveUnspecifiedAddresses(listenAddrs, filteredIfaceAddrs); err != nil {
-		if !manet.IsIPUnspecified(addr) {
+		// This can happen if we're listening on no addrs, or listening
-			finalAddrs = append(finalAddrs, addr)
+		// on IPv6 addrs, but only have IPv4 interface addrs.
-			continue
+		log.Debugw("failed to resolve listen addrs", "error", err)
-		}
+	} else {
-
+		finalAddrs = append(finalAddrs, resolved...)
-		resolved, err := addrutil.ResolveUnspecifiedAddress(addr, ifaceAddrs)
-		if err == nil {
-			for _, r := range resolved {
-				finalAddrs = append(finalAddrs, r)
-			}
-		}
 	}
 
 	finalAddrs = dedupAddrs(finalAddrs)
@@ -873,8 +900,13 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
 				extMaddr = ma.Join(extMaddr, rest)
 			}
 
-			// Add in the mapped addr.
+			// if the router reported a sane address
-			finalAddrs = append(finalAddrs, extMaddr)
+			if !manet.IsIPUnspecified(extMaddr) {
+				// Add in the mapped addr.
+				finalAddrs = append(finalAddrs, extMaddr)
+			} else {
+				log.Warn("NAT device reported an unspecified IP as it's external address")
+			}
 
 			// Did the router give us a routable public addr?
 			if manet.IsPublicAddr(mappedMaddr) {
@@ -883,29 +915,37 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
 			}
 
 			// No.
-			// in case router give us a wrong address.
+			// in case the router gives us a wrong address or we're behind a double-NAT.
 			// also add observed addresses
-
+			resolved, err := addrutil.ResolveUnspecifiedAddress(listen, allIfaceAddrs)
-			// Now, check if we have any observed addresses that
+			if err != nil {
-			// differ from the one reported by the router. Routers
+				// This can happen if we try to resolve /ip6/::/...
-			// don't always give the most accurate information.
+				// without any IPv6 interface addresses.
-			observed := h.ids.ObservedAddrsFor(listen)
-
-			if len(observed) == 0 {
 				continue
 			}
 
-			// Drop the IP from the external maddr
+			for _, addr := range resolved {
-			_, extMaddrNoIP := ma.SplitFirst(mappedMaddr)
+				// Now, check if we have any observed addresses that
+				// differ from the one reported by the router. Routers
+				// don't always give the most accurate information.
+				observed := h.ids.ObservedAddrsFor(addr)
 
-			for _, obsMaddr := range observed {
+				if len(observed) == 0 {
-				// Extract a public observed addr.
-				ip, _ := ma.SplitFirst(obsMaddr)
-				if ip == nil || !manet.IsPublicAddr(ip) {
 					continue
 				}
 
-				finalAddrs = append(finalAddrs, ma.Join(ip, extMaddrNoIP))
+				// Drop the IP from the external maddr
+				_, extMaddrNoIP := ma.SplitFirst(mappedMaddr)
+
+				for _, obsMaddr := range observed {
+					// Extract a public observed addr.
+					ip, _ := ma.SplitFirst(obsMaddr)
+					if ip == nil || !manet.IsPublicAddr(ip) {
+						continue
+					}
+
+					finalAddrs = append(finalAddrs, ma.Join(ip, extMaddrNoIP))
+				}
 			}
 		}
 	} else {

p2p/host/basic/basic_host_test.go

@@ -25,7 +25,6 @@ import (
 
 	ma "github.com/multiformats/go-multiaddr"
 	madns "github.com/multiformats/go-multiaddr-dns"
-	manet "github.com/multiformats/go-multiaddr-net"
 	"github.com/stretchr/testify/require"
 )
 
@@ -209,18 +208,20 @@ func TestLocalIPChangesWhenListenAddrChanges(t *testing.T) {
 	h := New(swarmt.GenSwarm(t, ctx, swarmt.OptDialOnly))
 	defer h.Close()
 
-	h.lipMu.Lock()
+	h.addrMu.Lock()
-	h.localIPv4Addr = nil
+	h.filteredInterfaceAddrs = nil
-	h.lipMu.Unlock()
+	h.allInterfaceAddrs = nil
+	h.addrMu.Unlock()
 
 	// change listen addrs and veify local IP addr is not nil again
 	require.NoError(t, h.Network().Listen(ma.StringCast("/ip4/0.0.0.0/tcp/0")))
 	h.SignalAddressChange()
 	time.Sleep(1 * time.Second)
 
-	h.lipMu.RLock()
+	h.addrMu.RLock()
-	h.lipMu.RUnlock()
+	defer h.addrMu.RUnlock()
-	require.NotNil(t, h.localIPv4Addr)
+	require.NotEmpty(t, h.filteredInterfaceAddrs)
+	require.NotEmpty(t, h.allInterfaceAddrs)
 }
 
 func TestAllAddrs(t *testing.T) {
@@ -231,27 +232,24 @@ func TestAllAddrs(t *testing.T) {
 	defer h.Close()
 	require.Nil(t, h.AllAddrs())
 
-	h.lipMu.RLock()
+	// listen on loopback
-	localIPv4Addr := h.localIPv4Addr
+	laddr := ma.StringCast("/ip4/127.0.0.1/tcp/0")
-	h.lipMu.RUnlock()
-
-	// listen on private IP address and see it's available on the address
-	laddr := localIPv4Addr.Encapsulate(ma.StringCast("/tcp/0"))
 	require.NoError(t, h.Network().Listen(laddr))
 	require.Len(t, h.AllAddrs(), 1)
-	addr := ma.Split(h.AllAddrs()[0])
+	firstAddr := h.AllAddrs()[0]
-	require.Equal(t, localIPv4Addr.String(), addr[0].String())
+	require.Equal(t, "/ip4/127.0.0.1", ma.Split(firstAddr)[0].String())
 
 	// listen on IPv4 0.0.0.0
 	require.NoError(t, h.Network().Listen(ma.StringCast("/ip4/0.0.0.0/tcp/0")))
 	// should contain localhost and private local addr along with previous listen address
 	require.Len(t, h.AllAddrs(), 3)
-	ipmap := make(map[string]struct{})
+	// Should still contain the original addr.
 	for _, a := range h.AllAddrs() {
-		ipmap[ma.Split(a)[0].String()] = struct{}{}
+		if a.Equal(firstAddr) {
+			return
+		}
 	}
-	require.Contains(t, ipmap, localIPv4Addr.String())
+	t.Fatal("expected addrs to contain original addr")
-	require.Contains(t, ipmap, manet.IP4Loopback.String())
 }
 
 func getHostPair(ctx context.Context, t *testing.T) (host.Host, host.Host) {