-
Notifications
You must be signed in to change notification settings - Fork 930
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
Optionally use only global IPs for AutoNAT #2526
Conversation
Oops, just now I see I left some redundant checks in the code. (as_server.rs line 128) |
While using this in my mixed environment, AutoNAT seems to switch from the correct Public() state to Private, perhaps when the random choice of peers selects the wrong one. Will need to investigate further, so this isn't production-ready yet. |
I'll do a proper review later, but I think what is missing right now is that also from the client side only servers are used that are not in the same private network. Because otherwise the client sends a dial-back request to the server in the same network, and the server filters all the addresses from the client because they are not public and then reports back a dial failure, which results in the Private NAT Status. |
rust-libp2p does not depend on any night features today. If I am not mistaken using nightly in rust-libp2p would require all applications build on top of rust-libp2p to also be build with nightly. I don't think we should force that upon our users. Instead I suggest simply copying the Does that make sense @hamamo? |
Yes, absolutely, I'm just unsure where to put it then, probably side by side to the function that uses it. |
I don't have an opinion here. Suggestion sounds good. |
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.
Thank you very much for the PR @hamamo , it is very much appreciated!
One comment to help the general understanding:
For DOS attack prevention (libp2p/specs#369) the server does not dial the exact addresses that the client sends in the dial-request.
Instead, the AutoNAT
behaviour remembers for each connected peer the address at which it observes the peer (= address from the endpoint on Behaviour::inject_connection_established
).
On an inbound dial-back request, it then iterates through all the addresses of the request, and for each address it replaces the address's IP with the client's observed on.
rust-libp2p/protocols/autonat/src/behaviour/as_server.rs
Lines 323 to 338 in 2ff9ace
let observed_ip = match observed_remote_at | |
.into_iter() | |
.find(|p| matches!(p, Protocol::Ip4(_) | Protocol::Ip6(_))) | |
{ | |
Some(ip) => ip, | |
None => return Vec::new(), | |
}; | |
let mut distinct = HashSet::new(); | |
demanded | |
.into_iter() | |
.filter_map(|addr| { | |
// Replace the demanded ip with the observed one. | |
let i = addr | |
.iter() | |
.position(|p| matches!(p, Protocol::Ip4(_) | Protocol::Ip6(_)))?; | |
let mut addr = addr.replace(i, |_| Some(observed_ip.clone()))?; |
Therefore, the "global-ips-only" restriction does not apply individually for each address in the dial-request and they don't need to be filtered. Instead, the observed address has to be checked and based on that the whole dial-request is accepted, or not.
Does that explanation make sense @hamamo?
if self.config.use_only_global_ips { | ||
addresses = addresses | ||
.into_iter() | ||
.filter(|a| a.contains_global_address()) | ||
.collect::<Vec<_>>(); | ||
}; |
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.
When a client sends a dial-back request to the server, the server replaces the IP in the sent addresses with the IP at which it observes the client. So effectively, the server never tests an IP the client sends, but instead only the ports of each address. Therefore it is not necessary to filter the addresses here. On the contrary: if we filter them here we remove all of the params.listened_addresses()
, since those are always private.
if self.config.use_only_global_ips { | ||
addrs = addrs | ||
.into_iter() | ||
.filter(|a| { | ||
!self.config.use_only_global_ips || a.contains_global_address() | ||
}) | ||
.collect(); | ||
} | ||
|
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.
I think this logic should be moved into the AsServer::resolve_inbound_request
method. If the client is in the same private network (= the observed IP is private), we want to directly return an Error
and not do a dial-attempt.
Friendly ping @hamamo. Are you still planning to do this fix? I am happy to help in case that you need more input. |
Fixed via #2618, thus I am closing here. |
Fixes #2514.
Beware, this is my first attempt at a pull request. And my first attempt to contribute Rust code to a public project...
This change introduces a config option for AutoNAT that forces it to only consider globally routable IP addresses for NAT probing. The default behavior is unchanged (all addresses are considered), and the restriction to globally routable addresses can be switched on using
use_only_global_ips
option.This branch is based on v0.42 as that is the libp2p version currently in crates.io. It requires a crate level attribute
#![feature(ip)]
to use theis_global()
methods of Ipv4Addr and Ipv6Addr.