identify detects if address translation is required #4
Conversation
|
Thanks for the work 🚀 I will review it more closely tomorrow. It would be great if @thomaseizinger could sign off on this as well, since these changes to identify are probably beyond the scope of my grant and my expertise. @jxs |
swarm/src/lib.rs
Outdated
| @@ -1132,39 +1134,69 @@ where | |||
|
|
|||
| self.pending_handler_event = Some((peer_id, handler, event)); | |||
| } | |||
| ToSwarm::NewExternalAddrCandidate(addr) => { | |||
| ToSwarm::NewExternalAddrCandidate { | |||
| endpoint, | |||
There was a problem hiding this comment.
I am not a fan of this API change. It feels like we are passing a lot of information back and forth here. If we don't actually need any state from the TCP transport to do the translation, perhaps this is the time to just remove Transport::address_translation entirely?
AFAIK, all transports just delegate to the generic address_translation function anyway! Plus, all behaviours get told about all listen addresses. So, can we instead:
- Remember all listen addresses in the identify behaviour
- Call the generic
address_translationfunction from there (we might even be able to move it to the identify behaviour which would have the added benefit of it being removed from the public API oflibp2p-core!) - In the identify behaviour, we know which addresses need translation, so we can call it only for those
- All other addresses we can just emit as is
There was a problem hiding this comment.
Thanks for your advise Thomas !
I implemented the address translation without using Transport::address_translation
(I did not removed yet Transport::address_translation but it is not used anymore)
- listen addresses are already stored in identify
- translation is done when both conditions are meet:
- a multiaddr of a transport requiring translation (use
is_addr_tcpandis_quic_addrtaken from the tcp/quic transport implementations) - connection was dialed without port reuse
- a multiaddr of a transport requiring translation (use
- call the
address_translationfunction - If translation gave an address emit the
ToSwarm::NewExternalAddrCandidatewith the translated address, otherwise emit theToSwarm::NewExternalAddrCandidatewith the observed address,
If this approach is ok, i will :
- remove
Transport::address_translationfunction - remove the
is_addr_tcpandis_quic_addrfor the transports - keep the generic
address_translationinlib_p2p_corebecause used inmdns(but not for nat purpose)
There was a problem hiding this comment.
Excellent work @stormshield-ebzh ! Thank you for this!
Much simpler than I expected too :)
It will need some testing but I think it makes sense to do this together with the already existing Transport API changes. Furthermore, if address_translation is now only used in mDNS and identify, we should move it to libp2p-swarm (one less thing to worry about in libp2p-core, yey! cc @jxs).
It would also be great to have some tests around this. Now that this feature is in libp2p-identify, can we write a test for that there? Some manual testing wouldn't hurt too.
| /// Whether an [`Multiaddr`] is a valid for the QUIC transport. | ||
| fn is_quic_addr(addr: &Multiaddr, v1: bool) -> bool { | ||
| use Protocol::*; | ||
| let mut iter = addr.iter(); | ||
| let Some(first) = iter.next() else { | ||
| return false; | ||
| }; | ||
| let Some(second) = iter.next() else { | ||
| return false; | ||
| }; | ||
| let Some(third) = iter.next() else { | ||
| return false; | ||
| }; | ||
| let fourth = iter.next(); | ||
| let fifth = iter.next(); | ||
|
|
||
| matches!(first, Ip4(_) | Ip6(_) | Dns(_) | Dns4(_) | Dns6(_)) | ||
| && matches!(second, Udp(_)) | ||
| && if v1 { | ||
| matches!(third, QuicV1) | ||
| } else { | ||
| matches!(third, Quic) | ||
| } | ||
| && matches!(fourth, Some(P2p(_)) | None) | ||
| && fifth.is_none() | ||
| } |
There was a problem hiding this comment.
I think this came up another time already. Why do we need to perform address translation for QUIC addresses? QUIC never uses ephemeral ports (to my knowledge).
There was a problem hiding this comment.
Autonat server should use ephemeral port when dialing back a peer.
otherwise the server dial-back will always succeed (and will report a false positive to the peer) because the peer has already opened the states in the firewalls when dialing the server.
=> translation for QUIC addresses is required
@umgefahren : When dialing back, the server should use a dedicated quic endpoint (not localy binded) and should not reuse a listner endpoint (this is now not the case)
There was a problem hiding this comment.
Autonat server should use ephemeral port when dialing back a peer. otherwise the server dial-back will always succeed (and will report a false positive to the peer) because the peer has already opened the states in the firewalls when dialing the server. => translation for QUIC addresses is required
@umgefahren : When dialing back, the server should use a dedicated quic endpoint (not localy binded) and should not reuse a listner endpoint (this is now not the case)
You are right. Thank you for laying it out for me. We need to do address translation because we (libp2p) decide to explicitly use an ephemeral port for a new QUIC connection to avoid accidental hole-punching despite QUIC itself not needing it.
I think this is worthwhile documenting here because anybody coming across this will think "hey, QUIC always uses stable ports so why would be bother with address translation for QUIC". (Also cc @mxinden, I know you too have been confused by this in the past)
There was a problem hiding this comment.
Autonat server should use ephemeral port when dialing back a peer. otherwise the server dial-back will always succeed (and will report a false positive to the peer) because the peer has already opened the states in the firewalls when dialing the server. => translation for QUIC addresses is required
@umgefahren : When dialing back, the server should use a dedicated quic endpoint (not localy binded) and should not reuse a listner endpoint (this is now not the case)
I will change it so that when Endpoint is Dialer and PortUse::Reuse we dial a dedicated quic endpoint.
…slation to libp2p_swarm
|
I will merge this now |
Description
The goal of this MR is to show how identify could detect if a connection was dialed with port_use or not and forward the information to the swarm.
The swarm will trigger a
transport.address_translation()only when required (when dialed without port reuse)Now
address_translation()is only called when a translation is needed , so in the tcp transport we do not have to manage whether a multiaddr was dialed with or without port reuseRelated libp2p#4568