Optionally use only global IPs for AutoNAT

[hamamo]

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 the is_global() methods of Ipv4Addr and Ipv6Addr.

[hamamo]

Oops, just now I see I left some redundant checks in the code. (as_server.rs line 128)

[hamamo]

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.

[elenaf9]

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.
The information about the observed address from connected peers is already there, so you just would have to use that information in the as_client module when selecting the server.

[mxinden]

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 the is_global() methods of Ipv4Addr and Ipv6Addr.

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 is_global method from the standard library into the rust-libp2p code-base, giving credit to the rust std authors. Once it stabilized, we can use the standard library method directly.

Does that make sense @hamamo?

[hamamo]

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 is_global method from the standard library into the rust-libp2p code-base, giving credit to the rust std authors. Once it stabilized, we can use the standard library method directly.

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.

[mxinden]

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.

[elenaf9]

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?

[elenaf9]

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.

[elenaf9]

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.

[elenaf9]

Friendly ping @hamamo. Are you still planning to do this fix? I am happy to help in case that you need more input.

[elenaf9]

Fixed via #2618, thus I am closing here.