Skip to content

Commit e3367f1

Browse files
sukunrtsukunrt
committed
cleanup nat addr handling; add tests
1 parent e8dba37 commit e3367f1

File tree

2 files changed

+188
-83
lines changed

2 files changed

+188
-83
lines changed

‎p2p/host/basic/address_service.go‎

Lines changed: 90 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,7 @@ type addressService struct {
4646
ctx context.Context
4747
ctxCancel context.CancelFunc
4848

49-
wg sync.WaitGroup
50-
updateLocalIPv4Backoff backoff.ExpBackoff
51-
updateLocalIPv6Backoff backoff.ExpBackoff
49+
wg sync.WaitGroup
5250

5351
ifaceAddrs *interfaceAddrsCache
5452
}
@@ -226,91 +224,15 @@ func (a *addressService) AllAddrs() []ma.Multiaddr {
226224
return nil
227225
}
228226

229-
filteredIfaceAddrs := a.ifaceAddrs.Filtered()
230-
231-
// Iterate over all _unresolved_ listen addresses, resolving our primary
232-
// interface only to avoid advertising too many addresses.
233227
finalAddrs := make([]ma.Multiaddr, 0, 8)
234-
if resolved, err := manet.ResolveUnspecifiedAddresses(listenAddrs, filteredIfaceAddrs); err != nil {
235-
// This can happen if we're listening on no addrs, or listening
236-
// on IPv6 addrs, but only have IPv4 interface addrs.
237-
log.Debugw("failed to resolve listen addrs", "error", err)
238-
} else {
239-
finalAddrs = append(finalAddrs, resolved...)
240-
}
241-
242-
finalAddrs = ma.Unique(finalAddrs)
228+
finalAddrs = a.appendInterfaceAddrs(finalAddrs, listenAddrs)
243229

244230
// use nat mappings if we have them
245-
if a.natmgr != nil && a.natmgr.HasDiscoveredNAT() {
246-
// We have successfully mapped ports on our NAT. Use those
247-
// instead of observed addresses (mostly).
248-
// Next, apply this mapping to our addresses.
249-
for _, listen := range listenAddrs {
250-
extMaddr := a.natmgr.GetMapping(listen)
251-
if extMaddr == nil {
252-
// not mapped
253-
continue
254-
}
255-
256-
// if the router reported a sane address
257-
if !manet.IsIPUnspecified(extMaddr) {
258-
// Add in the mapped addr.
259-
finalAddrs = append(finalAddrs, extMaddr)
260-
} else {
261-
log.Warn("NAT device reported an unspecified IP as it's external address")
262-
}
263-
264-
// Did the router give us a routable public addr?
265-
if manet.IsPublicAddr(extMaddr) {
266-
// well done
267-
continue
268-
}
269-
270-
// No.
271-
// in case the router gives us a wrong address or we're behind a double-NAT.
272-
// also add observed addresses
273-
resolved, err := manet.ResolveUnspecifiedAddress(listen, a.ifaceAddrs.All())
274-
if err != nil {
275-
// This can happen if we try to resolve /ip6/::/...
276-
// without any IPv6 interface addresses.
277-
continue
278-
}
279-
280-
for _, addr := range resolved {
281-
// Now, check if we have any observed addresses that
282-
// differ from the one reported by the router. Routers
283-
// don't always give the most accurate information.
284-
observed := a.ids.ObservedAddrsFor(addr)
285-
286-
if len(observed) == 0 {
287-
continue
288-
}
289-
290-
// Drop the IP from the external maddr
291-
_, extMaddrNoIP := ma.SplitFirst(extMaddr)
292-
293-
for _, obsMaddr := range observed {
294-
// Extract a public observed addr.
295-
ip, _ := ma.SplitFirst(obsMaddr)
296-
if ip == nil || !manet.IsPublicAddr(ip) {
297-
continue
298-
}
299-
300-
finalAddrs = append(finalAddrs, ma.Join(ip, extMaddrNoIP))
301-
}
302-
}
303-
}
304-
} else {
305-
var observedAddrs []ma.Multiaddr
306-
if a.ids != nil {
307-
observedAddrs = a.ids.OwnObservedAddrs()
308-
}
309-
finalAddrs = append(finalAddrs, observedAddrs...)
310-
}
231+
finalAddrs = a.appendNATAddrs(finalAddrs, listenAddrs)
311232
finalAddrs = ma.Unique(finalAddrs)
233+
312234
// Remove /p2p-circuit addresses from the list.
313-
// The p2p-circuit tranport listener reports its address as just /p2p-circuit
235+
// The p2p-circuit transport listener reports its address as just /p2p-circuit
314236
// This is useless for dialing. Users need to manage their circuit addresses themselves,
315237
// or use AutoRelay.
316238
finalAddrs = slices.DeleteFunc(finalAddrs, func(a ma.Multiaddr) bool {
@@ -322,6 +244,35 @@ func (a *addressService) AllAddrs() []ma.Multiaddr {
322244
return finalAddrs
323245
}
324246

247+
func (a *addressService) appendInterfaceAddrs(result []ma.Multiaddr, listenAddrs []ma.Multiaddr) []ma.Multiaddr {
248+
// resolving any unspecified listen addressees to use only the primary
249+
// interface to avoid advertising too many addresses.
250+
if resolved, err := manet.ResolveUnspecifiedAddresses(listenAddrs, a.ifaceAddrs.Filtered()); err != nil {
251+
log.Warnw("failed to resolve listen addrs", "error", err)
252+
} else {
253+
result = append(result, resolved...)
254+
}
255+
result = ma.Unique(result)
256+
return result
257+
}
258+
259+
func (a *addressService) appendNATAddrs(result []ma.Multiaddr, listenAddrs []ma.Multiaddr) []ma.Multiaddr {
260+
ifaceAddrs := a.ifaceAddrs.All()
261+
// use nat mappings if we have them
262+
if a.natmgr != nil && a.natmgr.HasDiscoveredNAT() {
263+
// we have a NAT device
264+
for _, listen := range listenAddrs {
265+
extMaddr := a.natmgr.GetMapping(listen)
266+
result = appendValidNATAddrs(result, listen, extMaddr, a.ids.ObservedAddrsFor, ifaceAddrs)
267+
}
268+
} else {
269+
if a.ids != nil {
270+
result = append(result, a.ids.OwnObservedAddrs()...)
271+
}
272+
}
273+
return result
274+
}
275+
325276
func (a *addressService) addCertHashes(addrs []ma.Multiaddr) []ma.Multiaddr {
326277
// This is a temporary workaround/hack that fixes #2233. Once we have a
327278
// proper address pipeline, rework this. See the issue for more context.
@@ -536,3 +487,59 @@ func (i *interfaceAddrsCache) updateUnlocked() {
536487
}
537488
}
538489
}
490+
491+
func getAllPossibleLocalAddrs(listenAddr ma.Multiaddr, ifaceAddrs []ma.Multiaddr) []ma.Multiaddr {
492+
// If the nat mapping fails, use the observed addrs
493+
resolved, err := manet.ResolveUnspecifiedAddress(listenAddr, ifaceAddrs)
494+
if err != nil {
495+
log.Warnf("failed to resolve listen addr %s, %s: %s", listenAddr, ifaceAddrs, err)
496+
return nil
497+
}
498+
return append(resolved, listenAddr)
499+
}
500+
501+
// appendValidNATAddrs adds the NAT-ed addresses to the result. If the NAT device doesn't provide
502+
// us with a public IP address, we use the observed addresses.
503+
func appendValidNATAddrs(result []ma.Multiaddr, listenAddr ma.Multiaddr, natMapping ma.Multiaddr,
504+
obsAddrsFunc func(ma.Multiaddr) []ma.Multiaddr,
505+
ifaceAddrs []ma.Multiaddr) []ma.Multiaddr {
506+
if natMapping == nil {
507+
allAddrs := getAllPossibleLocalAddrs(listenAddr, ifaceAddrs)
508+
for _, a := range allAddrs {
509+
result = append(result, obsAddrsFunc(a)...)
510+
}
511+
return result
512+
}
513+
514+
// if the router reported a sane address, use it.
515+
if !manet.IsIPUnspecified(natMapping) {
516+
result = append(result, natMapping)
517+
} else {
518+
log.Warn("NAT device reported an unspecified IP as it's external address")
519+
}
520+
521+
// If the router gave us a public address, use it and ignore observed addresses
522+
if manet.IsPublicAddr(natMapping) {
523+
return result
524+
}
525+
526+
// Router gave us a private IP; maybe we're behind a CGNAT.
527+
// See if we have a public IP from observed addresses.
528+
_, extMaddrNoIP := ma.SplitFirst(natMapping)
529+
if extMaddrNoIP == nil {
530+
return result
531+
}
532+
533+
allAddrs := getAllPossibleLocalAddrs(listenAddr, ifaceAddrs)
534+
for _, addr := range allAddrs {
535+
for _, obsMaddr := range obsAddrsFunc(addr) {
536+
// Extract a public observed addr.
537+
ip, _ := ma.SplitFirst(obsMaddr)
538+
if ip == nil || !manet.IsPublicAddr(ip) {
539+
continue
540+
}
541+
result = append(result, ma.Join(ip, extMaddrNoIP))
542+
}
543+
}
544+
return result
545+
}

‎p2p/host/basic/address_service_test.go‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package basichost
2+
3+
import (
4+
"testing"
5+
6+
ma "github.com/multiformats/go-multiaddr"
7+
manet "github.com/multiformats/go-multiaddr/net"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func TestAppendNATAddrs(t *testing.T) {
12+
if1, if2 := ma.StringCast("/ip4/192.168.0.100"), ma.StringCast("/ip4/1.1.1.1")
13+
ifaceAddrs := []ma.Multiaddr{if1, if2}
14+
tcpListenAddr, udpListenAddr := ma.StringCast("/ip4/0.0.0.0/tcp/1"), ma.StringCast("/ip4/0.0.0.0/udp/2/quic-v1")
15+
cases := []struct {
16+
Name string
17+
Listen ma.Multiaddr
18+
Nat ma.Multiaddr
19+
ObsAddrFunc func(ma.Multiaddr) []ma.Multiaddr
20+
Expected []ma.Multiaddr
21+
}{
22+
{
23+
Name: "nat map success",
24+
// nat mapping success, obsaddress ignored
25+
Listen: ma.StringCast("/ip4/0.0.0.0/udp/1/quic-v1"),
26+
Nat: ma.StringCast("/ip4/1.1.1.1/udp/10/quic-v1"),
27+
ObsAddrFunc: func(m ma.Multiaddr) []ma.Multiaddr {
28+
return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/udp/100/quic-v1")}
29+
},
30+
Expected: []ma.Multiaddr{ma.StringCast("/ip4/1.1.1.1/udp/10/quic-v1")},
31+
},
32+
{
33+
Name: "nat map failure",
34+
//nat mapping fails, obs addresses added
35+
Listen: ma.StringCast("/ip4/0.0.0.0/tcp/1"),
36+
Nat: nil,
37+
ObsAddrFunc: func(a ma.Multiaddr) []ma.Multiaddr {
38+
ip, _ := ma.SplitFirst(a)
39+
if ip == nil {
40+
return nil
41+
}
42+
if ip.Equal(if1) {
43+
return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/tcp/100")}
44+
} else {
45+
return []ma.Multiaddr{ma.StringCast("/ip4/3.3.3.3/tcp/100")}
46+
}
47+
},
48+
Expected: []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/tcp/100"), ma.StringCast("/ip4/3.3.3.3/tcp/100")},
49+
},
50+
{
51+
Name: "nat map success but CGNAT",
52+
//nat addr added, obs address added with nat provided port
53+
Listen: tcpListenAddr,
54+
Nat: ma.StringCast("/ip4/100.100.1.1/tcp/100"),
55+
ObsAddrFunc: func(a ma.Multiaddr) []ma.Multiaddr {
56+
ip, _ := ma.SplitFirst(a)
57+
if ip == nil {
58+
return nil
59+
}
60+
if ip.Equal(if1) {
61+
return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/tcp/20")}
62+
} else {
63+
return []ma.Multiaddr{ma.StringCast("/ip4/3.3.3.3/tcp/30")}
64+
}
65+
},
66+
Expected: []ma.Multiaddr{
67+
ma.StringCast("/ip4/100.100.1.1/tcp/100"),
68+
ma.StringCast("/ip4/2.2.2.2/tcp/100"),
69+
ma.StringCast("/ip4/3.3.3.3/tcp/100"),
70+
},
71+
},
72+
{
73+
Name: "uses unspecified address for obs address",
74+
// observed address manager should be queries with both specified and unspecified addresses
75+
// udp observed addresses are mapped to unspecified addresses
76+
Listen: udpListenAddr,
77+
Nat: nil,
78+
ObsAddrFunc: func(a ma.Multiaddr) []ma.Multiaddr {
79+
if manet.IsIPUnspecified(a) {
80+
return []ma.Multiaddr{ma.StringCast("/ip4/3.3.3.3/udp/20/quic-v1")}
81+
}
82+
return []ma.Multiaddr{ma.StringCast("/ip4/2.2.2.2/udp/20/quic-v1")}
83+
},
84+
Expected: []ma.Multiaddr{
85+
ma.StringCast("/ip4/2.2.2.2/udp/20/quic-v1"),
86+
ma.StringCast("/ip4/3.3.3.3/udp/20/quic-v1"),
87+
},
88+
},
89+
}
90+
for _, tc := range cases {
91+
t.Run(tc.Name, func(t *testing.T) {
92+
res := appendValidNATAddrs(nil,
93+
tc.Listen, tc.Nat, tc.ObsAddrFunc, ifaceAddrs)
94+
res = ma.Unique(res)
95+
require.ElementsMatch(t, tc.Expected, res, "%s\n%s", tc.Expected, res)
96+
})
97+
}
98+
}

0 commit comments

Comments
 (0)