From 743d1cf48a6a5ba831b2e32a642b8a4ac7e88535 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 1 Jul 2020 17:54:38 -0700 Subject: [PATCH 1/9] fix: use all interface addresses when we can't find the default route When we can't find the default route, advertise all addresses so we have something. --- p2p/host/basic/basic_host.go | 51 +++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index cde370d43d..ee5779cb7d 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -103,9 +103,8 @@ type BasicHost struct { addrChangeChan chan struct{} - lipMu sync.RWMutex - localIPv4Addr ma.Multiaddr - localIPv6Addr ma.Multiaddr + lipMu sync.RWMutex + interfaceAddrs []ma.Multiaddr disableSignedPeerRecord bool signKey crypto.PrivKey @@ -257,24 +256,49 @@ func (h *BasicHost) updateLocalIpAddr() { h.lipMu.Lock() defer h.lipMu.Unlock() + h.interfaceAddrs = nil + 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 { maddr, err := manet.FromIP(localIPv4) if err == nil { - h.localIPv4Addr = maddr + h.interfaceAddrs = append(h.interfaceAddrs, maddr) } } 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 { maddr, err := manet.FromIP(localIpv6) if err == nil { - h.localIPv6Addr = maddr + h.interfaceAddrs = append(h.interfaceAddrs, maddr) + } + } + } + + ifaceAddrs, err := manet.InterfaceMultiaddrs() + if err != nil { + log.Errorw("failed to resolve local interface addresses", "error", err) + // Add both loopback addresses anyways. + h.interfaceAddrs = append(h.interfaceAddrs, manet.IP4Loopback, manet.IP6Loopback) + return + } + + // If netroute failed to get us any interface addresses, use all of + // them. + if len(h.interfaceAddrs) == 0 { + // Add all addresses. + h.interfaceAddrs = append(h.interfaceAddrs, ifaceAddrs...) + } else { + // Only add loopback addresses. Filter these because we might + // not _have_ an IPv6 loopback address. + for _, addr := range ifaceAddrs { + if manet.IsIPLoopback(addr) { + h.interfaceAddrs = append(h.interfaceAddrs, addr) } } } @@ -748,18 +772,9 @@ func dedupAddrs(addrs []ma.Multiaddr) (uniqueAddrs []ma.Multiaddr) { // 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() - localIPv4Addr := h.localIPv4Addr - localIPv6Addr := h.localIPv6Addr + ifaceAddrs := h.interfaceAddrs h.lipMu.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() From a0ce610a67fdf4400ee9845de35ef9591d1bdf11 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 1 Jul 2020 18:59:41 -0700 Subject: [PATCH 2/9] fix: don't add advertise unspecified addresses --- p2p/host/basic/basic_host.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index ee5779cb7d..45b60943f1 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -888,8 +888,11 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { extMaddr = ma.Join(extMaddr, rest) } - // Add in the mapped addr. - finalAddrs = append(finalAddrs, extMaddr) + // if the router reported a sane address + if !manet.IsIPUnspecified(extMaddr) { + // Add in the mapped addr. + finalAddrs = append(finalAddrs, extMaddr) + } // Did the router give us a routable public addr? if manet.IsPublicAddr(mappedMaddr) { From e03a031c5760c4bc3f83471265f0bb13c936dd4b Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 1 Jul 2020 21:24:23 -0700 Subject: [PATCH 3/9] fix: resolve addresses before looking up observed addresses --- p2p/host/basic/basic_host.go | 97 +++++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 45b60943f1..9db2fa2cbd 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -103,8 +103,9 @@ type BasicHost struct { addrChangeChan chan struct{} - lipMu sync.RWMutex - interfaceAddrs []ma.Multiaddr + addrMu sync.RWMutex + filteredInterfaceAddrs []ma.Multiaddr + allInterfaceAddrs []ma.Multiaddr disableSignedPeerRecord bool signKey crypto.PrivKey @@ -253,10 +254,13 @@ func NewHost(ctx context.Context, n network.Network, opts *HostOpts) (*BasicHost } func (h *BasicHost) updateLocalIpAddr() { - h.lipMu.Lock() - defer h.lipMu.Unlock() + h.addrMu.Lock() + defer h.addrMu.Unlock() - h.interfaceAddrs = nil + 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", "error", err) @@ -266,7 +270,7 @@ func (h *BasicHost) updateLocalIpAddr() { } else { maddr, err := manet.FromIP(localIPv4) if err == nil { - h.interfaceAddrs = append(h.interfaceAddrs, maddr) + h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, maddr) } } @@ -275,30 +279,38 @@ func (h *BasicHost) updateLocalIpAddr() { } else { maddr, err := manet.FromIP(localIpv6) if err == nil { - h.interfaceAddrs = append(h.interfaceAddrs, maddr) + h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, maddr) } } } + // Resolve the interface addresses ifaceAddrs, err := manet.InterfaceMultiaddrs() if err != nil { log.Errorw("failed to resolve local interface addresses", "error", err) - // Add both loopback addresses anyways. - h.interfaceAddrs = append(h.interfaceAddrs, manet.IP4Loopback, manet.IP6Loopback) + } else { + h.allInterfaceAddrs = ifaceAddrs + } + + // If we failed to lookup interface addrs. + if len(h.allInterfaceAddrs) == 0 { + // Add the loopback addresses to the filtered addrs and use them as the non-filtered addrs. + h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, manet.IP4Loopback, manet.IP6Loopback) + h.allInterfaceAddrs = h.filteredInterfaceAddrs return } // If netroute failed to get us any interface addresses, use all of // them. - if len(h.interfaceAddrs) == 0 { + if len(h.filteredInterfaceAddrs) == 0 { // Add all addresses. - h.interfaceAddrs = append(h.interfaceAddrs, ifaceAddrs...) + h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, ifaceAddrs...) } else { // Only add loopback addresses. Filter these because we might // not _have_ an IPv6 loopback address. for _, addr := range ifaceAddrs { if manet.IsIPLoopback(addr) { - h.interfaceAddrs = append(h.interfaceAddrs, addr) + h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, addr) } } } @@ -771,26 +783,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() - ifaceAddrs := h.interfaceAddrs - h.lipMu.RUnlock() + h.addrMu.RLock() + filteredIfaceAddrs := h.filteredInterfaceAddrs + allIfaceAddrs := h.allInterfaceAddrs + h.addrMu.RUnlock() // 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 !manet.IsIPUnspecified(addr) { - finalAddrs = append(finalAddrs, addr) - continue - } - - resolved, err := addrutil.ResolveUnspecifiedAddress(addr, ifaceAddrs) - if err == nil { - for _, r := range resolved { - finalAddrs = append(finalAddrs, r) - } - } + if resolved, err := addrutil.ResolveUnspecifiedAddresses(listenAddrs, filteredIfaceAddrs); err != nil { + // This can happen if we're listening on no addrs, or listening + // on IPv6 addrs, but only have IPv4 interface addrs. + log.Debugw("failed to resolve listen addrs", "error", err) + } else { + finalAddrs = append(finalAddrs, resolved...) } finalAddrs = dedupAddrs(finalAddrs) @@ -903,27 +910,35 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { // No. // in case router give us a wrong address. // also add observed addresses - - // 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(listen) - - if len(observed) == 0 { + resolved, err := addrutil.ResolveUnspecifiedAddress(listen, allIfaceAddrs) + if err != nil { + // This an happen we try to resolve /ip6/::/... + // without any IPv6 interface addresses. continue } - // Drop the IP from the external maddr - _, extMaddrNoIP := ma.SplitFirst(mappedMaddr) + for _, addr := range resolved { + // 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 { - // Extract a public observed addr. - ip, _ := ma.SplitFirst(obsMaddr) - if ip == nil || !manet.IsPublicAddr(ip) { + if len(observed) == 0 { 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 { From 293929edf1d2e1fcffb8eed86cd5cfcf4da2b90a Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 1 Jul 2020 21:36:23 -0700 Subject: [PATCH 4/9] fix: only advertise global unicast --- p2p/host/basic/basic_host.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 9db2fa2cbd..149c9ba823 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -267,17 +267,17 @@ func (h *BasicHost) updateLocalIpAddr() { } else { if _, _, localIPv4, err := r.Route(net.IPv4zero); err != nil { log.Debugw("failed to fetch local IPv4 address", "error", err) - } else { + } else if localIPv4.IsGlobalUnicast() { maddr, err := manet.FromIP(localIPv4) if err == nil { 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", "error", err) - } else { - maddr, err := manet.FromIP(localIpv6) + } else if localIPv6.IsGlobalUnicast() { + maddr, err := manet.FromIP(localIPv6) if err == nil { h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, maddr) } From 2cc1fa689296d6ea6aa5707a2a8eab5389f47894 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 1 Jul 2020 21:56:56 -0700 Subject: [PATCH 5/9] fix: filter link-local addresses from advertisement --- p2p/host/basic/basic_host.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 149c9ba823..5b3d2a8f7f 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -289,7 +289,12 @@ func (h *BasicHost) updateLocalIpAddr() { if err != nil { log.Errorw("failed to resolve local interface addresses", "error", err) } else { - h.allInterfaceAddrs = ifaceAddrs + for _, addr := range ifaceAddrs { + // Skip link-local addrs, they're mostly useless. + if !manet.IsIP6LinkLocal(addr) { + h.allInterfaceAddrs = append(h.allInterfaceAddrs, addr) + } + } } // If we failed to lookup interface addrs. @@ -304,11 +309,11 @@ func (h *BasicHost) updateLocalIpAddr() { // them. if len(h.filteredInterfaceAddrs) == 0 { // Add all addresses. - h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, ifaceAddrs...) + h.filteredInterfaceAddrs = h.allInterfaceAddrs } else { // Only add loopback addresses. Filter these because we might // not _have_ an IPv6 loopback address. - for _, addr := range ifaceAddrs { + for _, addr := range h.allInterfaceAddrs { if manet.IsIPLoopback(addr) { h.filteredInterfaceAddrs = append(h.filteredInterfaceAddrs, addr) } From 3d57d61ae181a4215770ac8e23688705335af825 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 1 Jul 2020 22:24:48 -0700 Subject: [PATCH 6/9] test: fix basic host addr tests --- p2p/host/basic/basic_host_test.go | 36 +++++++++++++++---------------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/p2p/host/basic/basic_host_test.go b/p2p/host/basic/basic_host_test.go index 31d732d132..f7726da0bc 100644 --- a/p2p/host/basic/basic_host_test.go +++ b/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.localIPv4Addr = nil - h.lipMu.Unlock() + h.addrMu.Lock() + h.filteredInterfaceAddrs = nil + 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.lipMu.RUnlock() - require.NotNil(t, h.localIPv4Addr) + h.addrMu.RLock() + defer h.addrMu.RUnlock() + 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() - localIPv4Addr := h.localIPv4Addr - h.lipMu.RUnlock() - - // listen on private IP address and see it's available on the address - laddr := localIPv4Addr.Encapsulate(ma.StringCast("/tcp/0")) + // listen on loopback + laddr := ma.StringCast("/ip4/127.0.0.1/tcp/0") require.NoError(t, h.Network().Listen(laddr)) require.Len(t, h.AllAddrs(), 1) - addr := ma.Split(h.AllAddrs()[0]) - require.Equal(t, localIPv4Addr.String(), addr[0].String()) + firstAddr := h.AllAddrs()[0] + 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()) - require.Contains(t, ipmap, manet.IP4Loopback.String()) + t.Fatal("expected addrs to contain original addr") } func getHostPair(ctx context.Context, t *testing.T) (host.Host, host.Host) { From 5d7d30d5c26336f981768dfc031382d1bad38cf8 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 7 Jul 2020 09:20:14 -0700 Subject: [PATCH 7/9] Fix comment typo Co-authored-by: Adin Schmahmann --- p2p/host/basic/basic_host.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 5b3d2a8f7f..7569b51102 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -917,7 +917,7 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { // also add observed addresses resolved, err := addrutil.ResolveUnspecifiedAddress(listen, allIfaceAddrs) if err != nil { - // This an happen we try to resolve /ip6/::/... + // This can happen if we try to resolve /ip6/::/... // without any IPv6 interface addresses. continue } From 0f8eb7bab8cd323d99640ef6bf571c82727be608 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 7 Jul 2020 09:35:29 -0700 Subject: [PATCH 8/9] address code review and cleanup some edge cases * If we fail to lookup interface addresses, do our best and bail. * Only fallback on guessing addresses if we fail to lookup any interface addresses. If the interface address lookup function returns nothing, accept that we have no IP addresses and move on. * Log when the NAT returns a bad address. --- p2p/host/basic/basic_host.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index 7569b51102..c5314af146 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -287,24 +287,24 @@ func (h *BasicHost) updateLocalIpAddr() { // 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) - } else { - for _, addr := range ifaceAddrs { - // Skip link-local addrs, they're mostly useless. - if !manet.IsIP6LinkLocal(addr) { - h.allInterfaceAddrs = append(h.allInterfaceAddrs, addr) - } - } - } - // If we failed to lookup interface addrs. - if len(h.allInterfaceAddrs) == 0 { // 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 { @@ -904,6 +904,8 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { 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? @@ -913,7 +915,7 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { } // No. - // in case router give us a wrong address. + // in case router give us a wrong address or we're behind a double-NAT. // also add observed addresses resolved, err := addrutil.ResolveUnspecifiedAddress(listen, allIfaceAddrs) if err != nil { From 8cb0350168ce36e059134945c283e12a6f3f5bdb Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Tue, 7 Jul 2020 16:21:43 -0700 Subject: [PATCH 9/9] Fix english. Co-authored-by: Adin Schmahmann --- p2p/host/basic/basic_host.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/p2p/host/basic/basic_host.go b/p2p/host/basic/basic_host.go index c5314af146..fd73c7b964 100644 --- a/p2p/host/basic/basic_host.go +++ b/p2p/host/basic/basic_host.go @@ -915,7 +915,7 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr { } // No. - // in case router give us a wrong address or we're behind a double-NAT. + // 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) if err != nil {