Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle /quic and /ws in NAT mappings #428

Closed
Stebalien opened this issue Sep 18, 2018 · 5 comments
Closed

Handle /quic and /ws in NAT mappings #428

Stebalien opened this issue Sep 18, 2018 · 5 comments
Assignees
Labels
kind/bug A bug in existing code (including security flaws)

Comments

@Stebalien
Copy link
Member

The mappings report /ip{4,6}/.../{udp,tcp}/... without the /quic or /ws suffix. This means that, while we do map these protocols, we don't actually advertise the correct addresses.

We need to:

  1. Fix BasicHost.AllAddrs to pick the right mappings based on the internal port, not the full address.
  2. Fix go-libp2p-nat to only accept tcp/udp addresses (quic, ws, etc. don't make any sense at the NAT level anyways). Really, we may want to switch the interfaces to protocol + port instead of multiaddr but we can do that later.
@Stebalien Stebalien added the kind/bug A bug in existing code (including security flaws) label Sep 18, 2018
@overbool
Copy link

@Stebalien Did you mean that /ip4/.../tcp/4001 and /ip4/.../tcp/4001/ws will generate two mappings, but latter was redundant?

@vyzo
Copy link
Contributor

vyzo commented Sep 22, 2018

Fix go-libp2p-nat to only accept tcp/udp addresses (quic, ws, etc. don't make any sense at the NAT level anyways).

But we do want to open port mappings in this case too.

@overbool
Copy link

/ip4/.../tcp/4001/ws is redundant, because tcp+4001 had been mapped (/ip4/.../tcp/4001). However, /ip4/.../tcp/4002/ws will be mapped, because tcp+4002 is a new mapping.

just like:

Really, we may want to switch the interfaces to protocol + port instead of multiaddr but we can do that later.

@Stebalien
Copy link
Member Author

No. We actually can't listen on both /ip4/.../tcp/4001 and /ip4/.../tcp/4001/ws. Those two listeners would conflict.

The issue is much simpler:

  1. When we see /ip4/.../tcp/4002/ws, we ask the NAT manager to map it. This works as intended.
  2. When we advertise our addresses, we ask the NAT manager which addresses we have mapped. Unfortunately, these addresses don't include the /ws part because the NAT manager really only cares about the IP/port/protocol tripple.

This means that even though we might be listening on /ip4/.../tcp/4002/ws, we'll end up advertising /ip4/.../tcp/4002 (no /ws).

@vyzo
Copy link
Contributor

vyzo commented Sep 24, 2018

that's definitely a bug!

Stebalien added a commit that referenced this issue Mar 6, 2019
1. Update to work with libp2p/go-libp2p-nat#14.
2. Avoid observed addrs when our NAT tells us about external addrs.
3. Ignore bad addrs reported by our NAT. Substitute with observed addrs.
4. Fix #428.
@ghost ghost assigned Stebalien Mar 6, 2019
@ghost ghost added the status/in-progress In progress label Mar 6, 2019
Stebalien added a commit that referenced this issue Mar 6, 2019
1. Update to work with libp2p/go-libp2p-nat#14.
2. Avoid observed addrs when our NAT tells us about external addrs.
3. Ignore bad addrs reported by our NAT. Substitute with observed addrs.
4. Fix #428.
Stebalien added a commit that referenced this issue Mar 6, 2019
1. Update to work with libp2p/go-libp2p-nat#14.
2. Avoid observed addrs when our NAT tells us about external addrs.
3. Ignore bad addrs reported by our NAT. Substitute with observed addrs.
4. Fix #428.
Stebalien added a commit that referenced this issue Mar 6, 2019
1. Update to work with libp2p/go-libp2p-nat#14.
2. Avoid observed addrs when our NAT tells us about external addrs.
3. Ignore bad addrs reported by our NAT. Substitute with observed addrs.
4. Fix #428.
Stebalien added a commit that referenced this issue Mar 6, 2019
1. Update to work with libp2p/go-libp2p-nat#14.
2. Avoid observed addrs when our NAT tells us about external addrs.
3. Ignore bad addrs reported by our NAT. Substitute with observed addrs.
4. Fix #428.
@ghost ghost removed the status/in-progress In progress label Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

No branches or pull requests

3 participants