replace the port number for double NAT mapping #101

huo-ju · 2021-02-18T05:41:52Z

This fixed the double-NAT problem.

Scenario:

libp2p listening on port 7000 -> Router A with upnp -> Router B with upnp -> Internet

Router A in the DMZ of the Router B. Router A ip range 10.0.0.0-255 Router B ip range 192.168.0.0-255

Router A will add a dynamic port forwarding by upnp, for example, 34372 -> 7000, and Router B will forward port 34372 to Router A.

So we have two addresses:

/ip4/10.0.0.100/tcp/7000
/ip4/192.168.0.100/tcp/34372

go-libp2p-autonat will try to dialback to /ipv4/PUBLIC_IP/tcp/7000 in this scenario, and it will fail.

The port 7000 should be replaced with the upnp forwarding port 34372.

willscott

👍 Thanks for working on this :) this should help a good set of NAT'd peers

willscott · 2021-02-18T06:04:13Z

svc.go

+func (as *autoNATService) extractPort(m ma.Multiaddr) (name string, port string, err error) {
+	for _, p := range m.Protocols() {
+		if p.Code == ma.P_TCP || p.Code == ma.P_UDP {
+			v, err := m.ValueForProtocol(p.Code)


rather than iterating through all protocols in m, can you instead directly see if there is a value for the 2 protocols of instance?

The multiformats encoding format is (<addr-protocol-code><addr-value>)+ so /tcp/34372/ip4/192.168.0.100 also a correctly formatted address, but search over bytes might be a better way.

svc.go

willscott · 2021-02-18T06:18:38Z

svc.go

+				if err == nil && addrcodename == obscodename && addrport != obsport { //make a new address
+					obsportstr := fmt.Sprintf("/%s/%s", obscodename, obsport)
+					addrportstr := fmt.Sprintf("/%s/%s", addrcodename, addrport)
+					addr, err = ma.NewMultiaddr(strings.Replace(obsaddr.String(), obsportstr, addrportstr, -1)) //replace the private addr


this could have issues with circuit addresses or other weird edges of multiaddr. rather than string replace prefer finding the component with the expected code, and changing it in that way using the multiaddr library for parsing.

The nat obsaddr shouldn't be a circuit address or over the proxy, and it supposed to be a simple /ipv4 or /ipv6 address in this situation.

The go-multiaddr package didn't provide a function to replace a component and I have to rewrite all bytes and parse to multiaddr, so replace the /tcp/port could be a clean way to resolve the issue, IMO.

willscott · 2021-02-18T17:19:53Z

I think I'm happy with this, given the reality of the ergonomics of multiaddrs. @Stebalien do you have stronger feelings?

…ping

huo-ju · 2021-02-19T06:24:10Z

I spend some time to dig deeper into go-multiaddr package and I had rewritten my code.
This implementation will resolve your review comments.
Thanks @willscott

willscott

Minor readability changes, otherwise 👍

willscott · 2021-02-22T14:46:42Z

svc.go

-		if p.Code == ma.P_TCP || p.Code == ma.P_UDP {
-			v, err := m.ValueForProtocol(p.Code)
+//replace obsaddr's port number with the port number of a
+func patchObsaddr(a ma.Multiaddr, obsaddr ma.Multiaddr) (bool, ma.Multiaddr) {


consider a return signature of (ma.Multiaddr, error) with an error of nil indicating that the patch was successful?

svc.go

Co-authored-by: Will <willscott@gmail.com>

Stebalien · 2021-02-24T23:31:06Z

svc.go

@@ -123,7 +124,12 @@ func (as *autoNATService) handleDial(p peer.ID, obsaddr ma.Multiaddr, mpi *pb.Me
 		}

 		if as.config.dialPolicy.skipDial(addr) {
-			continue
+			err, newobsaddr := patchObsaddr(addr, obsaddr)


err should always come after the value.

We likely don't even need an error in this case. Just return a replacement multiaddr if relevant.

What if obsaddr is in our set of addresses to skip (see lines 113-117). We should skip this check in that case.

If obsaddr had been add into seen[] at L113, and it will be skipped at L140.

Stebalien · 2021-02-24T23:31:35Z

svc.go

+
+// patchObsaddr replaces obsaddr's port number with the port number of `localaddr`
+func patchObsaddr(localaddr, obsaddr ma.Multiaddr) (error, ma.Multiaddr) {
+	if localaddr == nil || obsaddr == nil {


Can this happen? This seems unnecessary.

Stebalien · 2021-02-24T23:50:11Z

svc.go

+		return true
+	})
+
+	if isValid == true && len(rawport) > 0 {


if isValid && len(rawport) > 0 {

Or, even better:

if !isValid || len(rawport) == 0 { return ... }

Stebalien · 2021-02-24T23:51:59Z

svc.go

+		ma.ForEach(obsaddr, func(c ma.Component) bool {
+			switch c.Protocol().Code {
+			case ma.P_UDP, ma.P_TCP:
+				if code == c.Protocol().Code && isObsValid == true { //obsaddr has the same type protocol, and we can replace it.


No need for bool == true, just check bool.

Stebalien · 2021-02-25T00:21:25Z

svc.go

+			case ma.P_UDP, ma.P_TCP:
+				if code == c.Protocol().Code && isObsValid == true { //obsaddr has the same type protocol, and we can replace it.


matching the protocol twice seems redundant.

L268 match both UDP/TCP protocols, but we can replace only the same type protocol.

Stebalien · 2021-02-25T00:32:33Z

Also note: this should already sort of work, so I'm wondering what's going wrong. If I have a double NAT with:

10.0.1.2:7000 <- 192.168.1.20:1234 <- 1.2.3.4:1234

Libp2p will see that the internal gateway reported a private IP address (192.168.1.20) and it will replace this with the observed IP address (1.2.3.4) reported by our peers.

Trying to replace the local IPs with remote IPs is still a good idea, but I'm wondering if you've seen this be an issue in practice.

Stebalien · 2021-02-25T01:00:53Z

svc.go

@@ -123,7 +124,12 @@ func (as *autoNATService) handleDial(p peer.ID, obsaddr ma.Multiaddr, mpi *pb.Me
 		}

 		if as.config.dialPolicy.skipDial(addr) {


We may skip an address because it's a relay address (e.g.). This logic will continue to dial said addresses.

huo-ju · 2021-02-25T07:41:37Z

Yes, we have this issue on our testing environment.
We have a double nat network, and the libp2p node listening on 7000.

192.168.86.55:7000 <- 192.168.0.10:32922 <- 72.x.x.x:32922

Libp2p replaced the 192.168.86.55:7000 with observed IP address 72.x.x.x.x, and autonat tried to dailback 72.x.x.x.x:7000 in our environment.

Stebalien · 2021-02-25T19:03:39Z

Hm. It should also have tried 72.x.x.x:32922. Is your node advertising 192.168.0.10:32922 (check ipfs id).

huo-ju · 2021-02-25T20:19:33Z

We build our node from libp2p and it's not an ipfs node. But I believed we had advertised the address. I am also curious to why obsaddr not replace it with the correct port.

So I will try to see what happens inside p2p/protocol/identify/obsaddr.go and report back later.

Stebalien · 2021-02-25T20:39:47Z

Awesome. Sorry, I just usually assume IPFS. The responsible code is https://github.com/libp2p/go-libp2p/blob/69916ed4656f45c0fbcfeca4ef7554bbf89a4975/p2p/host/basic/basic_host.go#L950-L982.

huo-ju · 2021-03-01T22:54:25Z

Sorry for my late response. I add some debug logs in the code, and this is my discovery.

finalAddrs of basic_host.go#L950-L982 is correctly, it may take a few minutes becuase the OwnObservedAddrTTL is 10 minutes. In my environment, the finalAddrs will be [/ip4/192.168.86.48/tcp/19005 /ip4/127.0.0.1/tcp/19005 /ip4/192.168.0.10/tcp/25332].

Then, autonat obsaddr will be /ip4/72.x.x.x/tcp/19005 because it had connected to the remote peer before NAT Mapping. So we still need to replace the port with addresses of pb.Message_PeerInfo or replace the ip address in all address of pb.Message_PeerInfo may better.

replace the port number for double NAT mapping

f5a90b9

willscott reviewed Feb 18, 2021

View reviewed changes

willscott requested a review from aarshkshah1992 February 18, 2021 17:17

a better implementation to replace the port number for double NAT map…

3960002

…ping

bug fixed. don't jump out of the loop if obsaddr be replaced

741ca1a

willscott reviewed Feb 22, 2021

View reviewed changes

huo-ju and others added 2 commits February 22, 2021 11:38

Update svc.go

9d5510a

Co-authored-by: Will <willscott@gmail.com>

improve readability from the review comments

4c5f23f

willscott approved these changes Feb 22, 2021

View reviewed changes

willscott merged commit 7de1c95 into libp2p:master Feb 22, 2021

willscott mentioned this pull request Feb 22, 2021

Guess more #58

Closed

Stebalien reviewed Feb 25, 2021

View reviewed changes

Stebalien mentioned this pull request May 11, 2021

Release v0.14.0 libp2p/go-libp2p#1090

Closed

27 tasks

aschmahmann mentioned this pull request May 14, 2021

Release v0.9.0 ipfs/kubo#8058

Closed

71 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace the port number for double NAT mapping #101

replace the port number for double NAT mapping #101

huo-ju commented Feb 18, 2021

willscott left a comment

willscott Feb 18, 2021

huo-ju Feb 18, 2021 •

edited

Loading

willscott Feb 18, 2021

huo-ju Feb 18, 2021 •

edited

Loading

willscott commented Feb 18, 2021

huo-ju commented Feb 19, 2021 •

edited

Loading

willscott left a comment

willscott Feb 22, 2021

Stebalien Feb 24, 2021

huo-ju Feb 25, 2021 •

edited

Loading

Stebalien Feb 24, 2021

Stebalien Feb 24, 2021

Stebalien Feb 24, 2021

Stebalien Feb 25, 2021

huo-ju Feb 25, 2021

Stebalien commented Feb 25, 2021

Stebalien Feb 25, 2021

huo-ju commented Feb 25, 2021 •

edited

Loading

Stebalien commented Feb 25, 2021

huo-ju commented Feb 25, 2021

Stebalien commented Feb 25, 2021

huo-ju commented Mar 1, 2021 •

edited

Loading

		case ma.P_UDP, ma.P_TCP:
		if code == c.Protocol().Code && isObsValid == true { //obsaddr has the same type protocol, and we can replace it.

		@@ -123,7 +124,12 @@ func (as autoNATService) handleDial(p peer.ID, obsaddr ma.Multiaddr, mpi pb.Me
		}

		if as.config.dialPolicy.skipDial(addr) {

replace the port number for double NAT mapping #101

replace the port number for double NAT mapping #101

Conversation

huo-ju commented Feb 18, 2021

willscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huo-ju Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huo-ju Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

willscott commented Feb 18, 2021

huo-ju commented Feb 19, 2021 • edited Loading

willscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huo-ju Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Feb 25, 2021

Choose a reason for hiding this comment

huo-ju commented Feb 25, 2021 • edited Loading

Stebalien commented Feb 25, 2021

huo-ju commented Feb 25, 2021

Stebalien commented Feb 25, 2021

huo-ju commented Mar 1, 2021 • edited Loading

huo-ju Feb 18, 2021 •

edited

Loading

huo-ju Feb 18, 2021 •

edited

Loading

huo-ju commented Feb 19, 2021 •

edited

Loading

huo-ju Feb 25, 2021 •

edited

Loading

huo-ju commented Feb 25, 2021 •

edited

Loading

huo-ju commented Mar 1, 2021 •

edited

Loading