From b3039fccd18047e2c6bd788fd9e96a9908a5504c Mon Sep 17 00:00:00 2001 From: sukun Date: Mon, 2 Dec 2024 14:34:41 +0530 Subject: [PATCH] cleanup nat addr handling; add tests --- p2p/host/basic/address_service.go | 130 ++++++++++++++----------- p2p/host/basic/address_service_test.go | 98 +++++++++++++++++++ 2 files changed, 171 insertions(+), 57 deletions(-) create mode 100644 p2p/host/basic/address_service_test.go diff --git a/p2p/host/basic/address_service.go b/p2p/host/basic/address_service.go index c7166e7ac0..df9170afef 100644 --- a/p2p/host/basic/address_service.go +++ b/p2p/host/basic/address_service.go @@ -46,9 +46,7 @@ type addressService struct { ctx context.Context ctxCancel context.CancelFunc - wg sync.WaitGroup - updateLocalIPv4Backoff backoff.ExpBackoff - updateLocalIPv6Backoff backoff.ExpBackoff + wg sync.WaitGroup ifaceAddrs *interfaceAddrsCache } @@ -227,6 +225,7 @@ func (a *addressService) AllAddrs() []ma.Multiaddr { } filteredIfaceAddrs := a.ifaceAddrs.Filtered() + allIfaceAddrs := a.ifaceAddrs.All() // Iterate over all _unresolved_ listen addresses, resolving our primary // interface only to avoid advertising too many addresses. @@ -238,7 +237,6 @@ func (a *addressService) AllAddrs() []ma.Multiaddr { } else { finalAddrs = append(finalAddrs, resolved...) } - finalAddrs = ma.Unique(finalAddrs) // use nat mappings if we have them @@ -248,58 +246,10 @@ func (a *addressService) AllAddrs() []ma.Multiaddr { // Next, apply this mapping to our addresses. for _, listen := range listenAddrs { extMaddr := a.natmgr.GetMapping(listen) - if extMaddr == nil { - // not mapped - continue - } - - // if the router reported a sane address - 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(extMaddr) { - // well done - continue - } - - // No. - // in case the router gives us a wrong address or we're behind a double-NAT. - // also add observed addresses - resolved, err := manet.ResolveUnspecifiedAddress(listen, a.ifaceAddrs.All()) - if err != nil { - // This can happen if we try to resolve /ip6/::/... - // without any IPv6 interface addresses. - continue - } - - 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 := a.ids.ObservedAddrsFor(addr) - - if len(observed) == 0 { - continue - } - - // Drop the IP from the external maddr - _, extMaddrNoIP := ma.SplitFirst(extMaddr) - - 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)) - } - } + finalAddrs = append(finalAddrs, appendNATAddrs( + finalAddrs, listen, extMaddr, a.ids.ObservedAddrsFor, + allIfaceAddrs, + )...) } } else { var observedAddrs []ma.Multiaddr @@ -310,7 +260,7 @@ func (a *addressService) AllAddrs() []ma.Multiaddr { } finalAddrs = ma.Unique(finalAddrs) // Remove /p2p-circuit addresses from the list. - // The p2p-circuit tranport listener reports its address as just /p2p-circuit + // The p2p-circuit transport listener reports its address as just /p2p-circuit // This is useless for dialing. Users need to manage their circuit addresses themselves, // or use AutoRelay. finalAddrs = slices.DeleteFunc(finalAddrs, func(a ma.Multiaddr) bool { @@ -536,3 +486,69 @@ func (i *interfaceAddrsCache) updateUnlocked() { } } } + +func appendNATAddrs(result []ma.Multiaddr, listenAddr ma.Multiaddr, natMapping ma.Multiaddr, + obsAddrsFunc func(ma.Multiaddr) []ma.Multiaddr, + ifaceAddrs []ma.Multiaddr) []ma.Multiaddr { + if natMapping == nil { + // If the nat mapping fails, use the observed addrs + resolved, err := manet.ResolveUnspecifiedAddress(listenAddr, ifaceAddrs) + if err != nil { + log.Errorf("failed to resolve unspec addr %s, %s: %s", listenAddr, ifaceAddrs, err) + return result + } + allAddrs := append(resolved, listenAddr) + for _, a := range allAddrs { + result = append(result, obsAddrsFunc(a)...) + } + return result + } + + // if the router reported a sane address, use it. + if !manet.IsIPUnspecified(natMapping) { + // Add in the mapped addr. + result = append(result, natMapping) + } else { + log.Warn("NAT device reported an unspecified IP as it's external address") + } + + // If the router gave us a public address, use it ignoring observed addresses + if manet.IsPublicAddr(natMapping) { + return result + } + + // No. + // in case the router gives us a wrong address or we're behind a double-NAT. + // also add observed addresses + resolved, err := manet.ResolveUnspecifiedAddress(listenAddr, ifaceAddrs) + if err != nil { + log.Errorf("failed to resolve unspec addr %s, %s: %s", listenAddr, ifaceAddrs, err) + return result + } + // Add the listen addr too. UDP observed addresses are mapped to unspecified listen addresses. + allAddrs := append(resolved, listenAddr) + + // Drop the IP from the external maddr + _, extMaddrNoIP := ma.SplitFirst(natMapping) + if extMaddrNoIP == nil { + return result + } + for _, addr := range allAddrs { + // Now, check if we have any observed addresses. + observed := obsAddrsFunc(addr) + if len(observed) == 0 { + continue + } + + for _, obsMaddr := range observed { + // Extract a public observed addr. + ip, _ := ma.SplitFirst(obsMaddr) + if ip == nil || !manet.IsPublicAddr(ip) { + continue + } + + result = append(result, ma.Join(ip, extMaddrNoIP)) + } + } + return result +} diff --git a/p2p/host/basic/address_service_test.go b/p2p/host/basic/address_service_test.go new file mode 100644 index 0000000000..09c6af73f1 --- /dev/null +++ b/p2p/host/basic/address_service_test.go @@ -0,0 +1,98 @@ +package basichost + +import ( + "testing" + + ma "github.com/multiformats/go-multiaddr" + manet "github.com/multiformats/go-multiaddr/net" + "github.com/stretchr/testify/require" +) + +func TestAppendNATAddrs(t *testing.T) { + if1, if2 := ma.StringCast("/ip4/192.168.0.100"), ma.StringCast("/ip4/1.1.1.1") + ifaceAddrs := []ma.Multiaddr{if1, if2} + tcpListenAddr, udpListenAddr := ma.StringCast("/ip4/0.0.0.0/tcp/1"), ma.StringCast("/ip4/0.0.0.0/udp/2/quic-v1") + cases := []struct { + Name string + Listen ma.Multiaddr + Nat ma.Multiaddr + ObsAddrFunc func(ma.Multiaddr) []ma.Multiaddr + Expected []ma.Multiaddr + }{ + { + Name: "nat map success", + // nat mapping success, obsaddress ignored + Listen: ma.StringCast("/ip4/0.0.0.0/udp/1/quic-v1"), + Nat: ma.StringCast("/ip4/1.1.1.1/udp/10/quic-v1"), + ObsAddrFunc: func(m ma.Multiaddr) []ma.Multiaddr { + return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/udp/100/quic-v1")} + }, + Expected: []ma.Multiaddr{ma.StringCast("/ip4/1.1.1.1/udp/10/quic-v1")}, + }, + { + Name: "nat map failure", + //nat mapping fails, obs addresses added + Listen: ma.StringCast("/ip4/0.0.0.0/tcp/1"), + Nat: nil, + ObsAddrFunc: func(a ma.Multiaddr) []ma.Multiaddr { + ip, _ := ma.SplitFirst(a) + if ip == nil { + return nil + } + if ip.Equal(if1) { + return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/tcp/100")} + } else { + return []ma.Multiaddr{ma.StringCast("/ip4/3.3.3.3/tcp/100")} + } + }, + Expected: []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/tcp/100"), ma.StringCast("/ip4/3.3.3.3/tcp/100")}, + }, + { + Name: "nat map success but CGNAT", + //nat addr added, obs address added with nat provided port + Listen: tcpListenAddr, + Nat: ma.StringCast("/ip4/100.100.1.1/tcp/100"), + ObsAddrFunc: func(a ma.Multiaddr) []ma.Multiaddr { + ip, _ := ma.SplitFirst(a) + if ip == nil { + return nil + } + if ip.Equal(if1) { + return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/tcp/20")} + } else { + return []ma.Multiaddr{ma.StringCast("/ip4/3.3.3.3/tcp/30")} + } + }, + Expected: []ma.Multiaddr{ + ma.StringCast("/ip4/100.100.1.1/tcp/100"), + ma.StringCast("/ip4/2.2.2.2/tcp/100"), + ma.StringCast("/ip4/3.3.3.3/tcp/100"), + }, + }, + { + Name: "uses unspecified address for obs address", + // observed address manager should be queries with both specified and unspecified addresses + // udp observed addresses are mapped to unspecified addresses + Listen: udpListenAddr, + Nat: nil, + ObsAddrFunc: func(a ma.Multiaddr) []ma.Multiaddr { + if manet.IsIPUnspecified(a) { + return []ma.Multiaddr{ma.StringCast("/ip4/3.3.3.3/udp/20/quic-v1")} + } + return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/udp/20/quic-v1")} + }, + Expected: []ma.Multiaddr{ + ma.StringCast("/ip4/2.2.2.2/udp/20/quic-v1"), + ma.StringCast("/ip4/3.3.3.3/udp/20/quic-v1"), + }, + }, + } + for _, tc := range cases { + t.Run(tc.Name, func(t *testing.T) { + res := appendNATAddrs(nil, + tc.Listen, tc.Nat, tc.ObsAddrFunc, ifaceAddrs) + res = ma.Unique(res) + require.ElementsMatch(t, tc.Expected, res, "%s\n%s", tc.Expected, res) + }) + } +}