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.

Sign up for GitHub

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

fix(quic): fix address translation #4896

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(quic): fix address translation #4896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ libp2p-perf = { version = "0.3.0", path = "protocols/perf" }
libp2p-ping = { version = "0.44.0", path = "protocols/ping" }
libp2p-plaintext = { version = "0.41.0", path = "transports/plaintext" }
libp2p-pnet = { version = "0.24.0", path = "transports/pnet" }
libp2p-quic = { version = "0.10.1", path = "transports/quic" }
libp2p-quic = { version = "0.10.2", path = "transports/quic" }
libp2p-relay = { version = "0.17.1", path = "protocols/relay" }
libp2p-rendezvous = { version = "0.14.0", path = "protocols/rendezvous" }
libp2p-request-response = { version = "0.26.0", path = "protocols/request-response" }
Expand Down
5 changes: 5 additions & 0 deletions transports/quic/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## 0.10.2 - unreleased

- Fix address translation.
See [PR 4896](https://github.com/libp2p/rust-libp2p/pull/4896).

## 0.10.1

- Allow disabling path MTU discovery.
Expand Down
2 changes: 1 addition & 1 deletion transports/quic/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "libp2p-quic"
version = "0.10.1"
version = "0.10.2"
authors = ["Parity Technologies <admin@parity.io>"]
edition = "2021"
rust-version = { workspace = true }
Expand Down
3 changes: 2 additions & 1 deletion transports/quic/src/transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use futures::{prelude::*, stream::SelectAll};
use if_watch::IfEvent;

use libp2p_core::{
address_translation,
multiaddr::{Multiaddr, Protocol},
transport::{ListenerId, TransportError, TransportEvent},
Transport,
Expand Down Expand Up @@ -253,7 +254,7 @@ impl<P: Provider> Transport for GenTransport<P> {
{
return None;
}
Some(observed.clone())
address_translation(listen, observed)
Comment on lines -256 to +257
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what issue this is addressing.

The libp2p_core::address_translation function takes the port of listen and the IP of observed. This is relevant in the case of TCP without port-reuse where one does not listen on the port of an outgoing connection, i.e. here were one does not listen of the port of observed.

The relevant section of the address_translation doc comment:

rust-libp2p/core/src/translation.rs

Lines 29 to 32 in 338e467

/// This function can for example be useful when handling tcp connections. Tcp does not listen and
/// dial on the same port by default. Thus when receiving an observed address on a connection that
/// we initiated, it will contain our dialing port, not our listening port. We need to take the ip
/// address or dns address from the observed address and the port from the original address.

I argue that this is not relevant for QUIC, given that QUIC will use the same UDP port for both outgoing and incoming connections. The port of observed is either equal to the port of listen, or, if the local node is behind a NAT, equal to the public NAT port mapping of listen. In the former case address_translation is a no-op. In the latter case address_translation returns the wrong port, namely LAN port, not the public Internet port.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that with the upcoming redesign of #4568 we can get rid of Transport::address_translation. Whether a connection has been established using port-reuse or not will be visible to a NetworkBehaviour. Thus e.g. libp2p-identify receiving an observed address can judge itself, whether it needs to alter any ports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #2289 (comment) for past discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what issue this is addressing.

This addresses the problem of ephemeral ports with QUIC. At least when behind NAT with port forwarding, without address translation you'll end up with invalid external port. The more annoying thing is that it will be valid for the duration of Autonat probing, but when later anyone else tries to reach the node, the port is no longer functional.

The port of observed is either equal to the port of listen, or, if the local node is behind a NAT, equal to the public NAT port mapping of listen.

This is most certainly not true in practice, at least in some cases. Even on my machine with pfSense acting as a router, I have seen random ports all over the place observed by remote nodes.

In the former case address_translation is a no-op.

Yes

In the latter case address_translation returns the wrong port, namely LAN port, not the public Internet port.

No, in this case address translation will result in correct port. I guess it depends on how to do port forwarding, it should be possible for router to map ports both ways, but I can't imagine many users actually doing it, it seems that external ports are mapped onto LAN ports, but probably rarely outgoing LAN ports are mapped onto specific WAN ports. This is my educated guess based on observation, I'm not claiming some consumer routers are not doing that.

Note that with the upcoming redesign of #4568 we can get rid of Transport::address_translation. Whether a connection has been established using port-reuse or not will be visible to a NetworkBehaviour. Thus e.g. libp2p-identify receiving an observed address can judge itself, whether it needs to alter any ports.

Interesting. Well, then this will need to be adjusted when that happens, but in the meantime network needs to function and lack of address translation was a big issue for us since many nodes were not able to discover their public addresses with QUIC (that we use almost exclusively in place of TCP now).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see #2289 (comment) for past discussion.

This means the current implementation is still wrong though as it will happily return an observed TCP address as the translated address. If QUIC is "before" TCP in the transport stack, it would return a wrong address, wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See lines 252-256 above, the protocol is checked there and returns None if it doesn't match. Same is done in TCP and it works properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See lines 252-256 above, the protocol is checked there and returns None if it doesn't match. Same is done in TCP and it works properly.

Ah you are right, I missed that because the diff didn't show it 🙄

I don't really understand why we need to change anything here then. QUIC doesn't have a concept of ephemeral ports so address_translation can't do anything useful. If you still observe changing ports with QUIC, it might be that you are sitting behind a symmetric NAT. We can't do anything against that I am afraid.

The more annoying thing is that it will be valid for the duration of Autonat probing, but when later anyone else tries to reach the node, the port is no longer functional.

We are aware of this issue. Unfortunately, it is a design flaw in AutoNAT and one of the things that prompted the design of AutoNATv2. In v2, the server uses a different outgoing port to perform the dial-back, meaning we don't accidentally "hole-punch" through the NAT and get a more trustworthy result that the address is actually reachable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you still observe changing ports with QUIC, it might be that you are sitting behind a symmetric NAT. We can't do anything against that I am afraid.

But I still might have port forwarding configured such that the port matching my listening port is still reachable from the outside. This is what we recommend our users to do when they are behind NAT and it seems to work. It doesn't cover the case when forwarded port doesn't match listening port, in that case user has to specify external address explicitly, but it is still useful to do address translation, as mentioned above, in worst case it is no-op.

}

fn dial(&mut self, addr: Multiaddr) -> Result<Self::Dial, TransportError<Self::Error>> {
Expand Down
52 changes: 52 additions & 0 deletions transports/quic/tests/smoke.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use quic::Provider;
use rand::RngCore;
use std::future::Future;
use std::io;
use std::net::Ipv4Addr;
use std::num::NonZeroU8;
use std::task::Poll;
use std::time::Duration;
Expand Down Expand Up @@ -236,6 +237,57 @@ fn new_tcp_quic_transport() -> (PeerId, Boxed<(PeerId, StreamMuxerBox)>) {
(peer_id, transport)
}

#[cfg(feature = "async-std")]
#[test]
fn test_address_translation_async_std() {
test_address_translation::<quic::async_std::Provider>()
}

#[cfg(feature = "tokio")]
#[test]
fn test_address_translation_tokio() {
test_address_translation::<quic::tokio::Provider>()
}

fn test_address_translation<T>()
where
T: Provider,
{
let (_peer_id, transport) = create_default_transport::<T>();

let port = 42;
let quic_listen_addr = Multiaddr::empty()
.with(Protocol::Ip4(Ipv4Addr::new(127, 0, 0, 1)))
.with(Protocol::Udp(port))
.with(Protocol::QuicV1);
Comment on lines +259 to +262
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a helper function that takes in an Ipv4Addr and a port would make this test a bit more concise.

let observed_ip = Ipv4Addr::new(123, 45, 67, 8);
let quic_observed_addr = Multiaddr::empty()
.with(Protocol::Ip4(observed_ip))
.with(Protocol::Udp(1))
.with(Protocol::QuicV1)
.with(Protocol::P2p(PeerId::random()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one important?


let translated = transport
.address_translation(&quic_listen_addr, &quic_observed_addr)
.unwrap();
let mut iter = translated.iter();
assert_eq!(iter.next(), Some(Protocol::Ip4(observed_ip)));
assert_eq!(iter.next(), Some(Protocol::Udp(port)));
assert_eq!(iter.next(), Some(Protocol::QuicV1));
assert_eq!(iter.next(), None);
Comment on lines +273 to +277
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be cleaner to use assert_eq against a constructed address?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is translated 1:1 from TCP test, I can update it, but then TCP should probably be updated too (and maybe others?).


let tcp_addr = Multiaddr::empty()
.with(Protocol::Ip4(Ipv4Addr::new(87, 65, 43, 21)))
.with(Protocol::Tcp(1));

assert!(transport
.address_translation(&quic_listen_addr, &tcp_addr)
.is_none());
assert!(transport
.address_translation(&tcp_addr, &quic_observed_addr)
.is_none());
}

#[cfg(feature = "async-std")]
#[async_std::test]
async fn tcp_and_quic() {
Expand Down