-
Notifications
You must be signed in to change notification settings - Fork 12
replace the port number for double NAT mapping #101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for working on this :) this should help a good set of NAT'd peers
svc.go
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than iterating through all protocols in m
, can you instead directly see if there is a value for the 2 protocols of instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
I think I'm happy with this, given the reality of the ergonomics of multiaddrs. @Stebalien do you have stronger feelings? |
I spend some time to dig deeper into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor readability changes, otherwise 👍
svc.go
Outdated
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider a return signature of (ma.Multiaddr, error)
with an error of nil
indicating that the patch was successful?
Co-authored-by: Will <willscott@gmail.com>
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If obsaddr
had been add into seen[]
at L113, and it will be skipped at L140.
|
||
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this happen? This seems unnecessary.
return true | ||
}) | ||
|
||
if isValid == true && len(rawport) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isValid && len(rawport) > 0 {
Or, even better:
if !isValid || len(rawport) == 0 {
return ...
}
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for bool == true
, just check bool
.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matching the protocol twice seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L268 match both UDP/TCP protocols, but we can replace only the same type protocol.
Also note: this should already sort of work, so I'm wondering what's going wrong. If I have a double NAT with:
Libp2p will see that the internal gateway reported a private IP address ( 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. |
@@ -123,7 +124,12 @@ func (as *autoNATService) handleDial(p peer.ID, obsaddr ma.Multiaddr, mpi *pb.Me | |||
} | |||
|
|||
if as.config.dialPolicy.skipDial(addr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may skip an address because it's a relay address (e.g.). This logic will continue to dial said addresses.
Yes, we have this issue on our testing environment.
Libp2p replaced the |
Hm. It should also have tried |
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 So I will try to see what happens inside |
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. |
Sorry for my late response. I add some debug logs in the code, and this is my discovery.
Then, autonat |
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.