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

protocols/: Add basic AutoNAT implementation #2262

Merged
merged 87 commits into from
Jan 14, 2022

Conversation

elenaf9
Copy link
Contributor

@elenaf9 elenaf9 commented Oct 3, 2021

This PR drafts a behaviour protocol that implements basic AutoNAT. It is based on #1672 (minus #1667) from @dvc94ch .
The Autonat Protocol implements a Codec for the Request-Response protocol, and wraps it in a new Network Behaviour with some additional functionality. The basic idea is:

See #2262 (comment) description of the most recent implementation.

  • A peer A maintains a list of its own addresses:
    • For each address the current score is set: Higher score means higher reachability.
    • Addresses are either listening addresses reported from Listeners (initial Score: 0), or external addresses observed by other peers (initial Score: 1).
  • When a new connection is established to a Peer B, A sends a DialRequest to B, with all addresses from the above list (ordered by Address Score, descending)
  • B tries to dial the addresses:
    • It adds all addresses of A to the address-book (part of the request-response protocol)
    • Swarm::dial(Peer A) sequentially tries to dial all addresses until either one attempt succeeds, or all have failed
    • If success: B sends back an Ok with the address that it successfully dial, if error: B sends back Error
  • If A receives an Ok(address) it increases the address score of this address by 1
  • Contrary to Autonat #1672, this implementation of the AutoNAT Protocol has no OutEvent. Instead, when increasing a peer's score a NetworkBehaviourAction::ReportObservedAddr with the current score is reported to to the Swarm.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Wow, that was quick! Thanks.

As an aside, great to see libp2p-request-response being useful here.

Can you reference the new crate in src/lib.rs? Similar to:

rust-libp2p/src/lib.rs

Lines 58 to 61 in c7abb6f

#[cfg(feature = "floodsub")]
#[cfg_attr(docsrs, doc(cfg(feature = "floodsub")))]
#[doc(inline)]
pub use libp2p_floodsub as floodsub;

Listing a couple of things needed beyond a basic version. Unfortunately the current draft of the specification is still very basic, thus many of these likely need design work as well.

  • Expose whether the local node is publicly reachable or not. (e.g. needed for libp2p-kad to decide to run in client or server mode Support client mode in kademlia #2184).
  • Implement various rate limiting to prevent attacks.
  • Continuously trigger probes for local external addresses.
  • ...

protocols/autonat/Cargo.toml Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@elenaf9
Copy link
Contributor Author

elenaf9 commented Oct 5, 2021

Expose whether the local node is publicly reachable or not. (e.g. needed for libp2p-kad to decide to run in client or server mode #2184).

Ok so I would propose some additional logic:
The behaviour has a list of public nodes, that have to be provided by the outside (similar how kademlia requires such a list of initial nodes; nodes can be added / removed from the list at any time).
Add a method determine_nat_status() that determines whether the current node is publicly available or not:
The method takes as input

  1. The above list of publicly available nodes
  2. The number of requests that have to be successful in order for the address to count as "publicly available" (can be set in a config)

It will then send a DialRequest to each node, and collect the results to determine if one of the external_addresses can be reached by the required number of peers. It will then create a AutoNAT behaviour OutEvent, with either NatStatus::Public(public_address), or NatStatus::Private, depending on whether one of the address was reachable by the required number of peers.
Depending on this result, the AutoNAT behaviour will report the address score, and also set a local status.
In a configurable frequency it will retriggger the determine_nat_status method to verify that its status did not change (or change its status in cases that it did change). This will:

  1. keep the status up to date
  2. keep mapping on NAT devices active

If we are in NatStatus::Private, we could also retrigger determine_nat_status directly if a new external address is observed. (Which would actually be an argument towards what @mxinden suggested above about changing ReportObservedAddr to a push-like behaviour).
I am not sure whether we would then even need to send a DialRequest to each peer that we establish a connection.


I think this is also a bit more aligned with what is described in the specs PR. Wdyt @mxinden?

@mxinden
Copy link
Member

mxinden commented Oct 6, 2021

Thanks for the follow-up @elenaf9!

The behaviour has a list of public nodes, that have to be provided by the outside (similar how kademlia requires such a list of initial nodes; nodes can be added / removed from the list at any time).

Is that a strict requirement? It would be great for libp2p-autonat to not require any configuration nor depend on any preconfigured nodes.

The number of requests that have to be successful in order for the address to count as "publicly available" (can be set in a config)

Sounds good to me. We can likely learn a bunch from the Golang implementation, which is set to assume it is public on 3 successes.

It will then create a AutoNAT behaviour OutEvent, with either NatStatus::Public(public_address), or NatStatus::Private, depending on whether one of the address was reachable by the required number of peers.

Reporting through OutEvent sounds good to me.

In a configurable frequency it will retriggger the determine_nat_status method to verify that its status did not change (or change its status in cases that it did change).

Sounds good to me. We can likely use the Golang defaults at the beginning, right?

If we are in NatStatus::Private, we could also retrigger determine_nat_status directly if a new external address is observed.

👍 good idea.

I am not sure whether we would then even need to send a DialRequest to each peer that we establish a connection.

How about keeping track of all the peers we are connected to. On an interval, we chose one of them at random and ask them to dial us. Results of those dials then influence whether we assume we are publicly reachable or not.

@mxinden
Copy link
Member

mxinden commented Oct 6, 2021

Crossreferencing libp2p/specs#369 here which we will very much need in an initial version. Don't want libp2p to become the next amplification attack network 😨

Copy link
Contributor Author

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here.
I have now refactored a majority of the behaviour logic according to the above comments. It is now also more aligned with the go-autonat implementation, which btw helped a lot thanks! There is still quite a lot missing, especially regarding config parameters and the calculation of the retry-nat-status frequency, and tests of course.
I hope I'll find more time towards this weekend / next week to invest into this.

protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the clean and comprehensible code!

From a features perspective, I think this is sufficient for a first version.

I haven't had a chance to test this out, which I would like to do before merging and releasing it.

Have you had a chance to test this (a) in the wild and (b) with the Golang implementation?

protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
protocols/autonat/src/behaviour.rs Outdated Show resolved Hide resolved
@canewsin
Copy link
Contributor

@elenaf9 could you provide some basic example to use it ?

@elenaf9
Copy link
Contributor Author

elenaf9 commented Nov 17, 2021

@canewsin I will definitely add some test & code-docs that will exemplify how this can be used. But I currently don't have the capacity to add a full example, though I agree that it would be great to have one that combines AutoNAT with e.g. Kademlia and the Relay protocol. But imo that would be an issue on its own.

Btw if you are generally interested in AutoNAT, it would also be great to have your review on it. Since @mxinden stated that the features are sufficient for a first version, the only thing left for this for this PR is to resolve the review and add docs & tests. If you feel like there's anything else missing, feel free to comment on it.

Add `AsClient` and `AsServer` views for the Behaviour to separate the
logic of the two roles. Instead of adding permanent (sub-)structs to
the Behaviour, the two new structures only hold temporary references
to the parent fields. This allows shared access to fields that are used
by both structures.
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

One small suggestion. Otherwise code looks good to me. Thank you for the continuous work here.

My public server identifies itself as private which is odd (StatusChanged { old: Unknown, new: Private }), though that might be on my end. Still need to investigate further.

protocols/autonat/src/protocol.rs Outdated Show resolved Hide resolved
@mxinden
Copy link
Member

mxinden commented Jan 11, 2022

My public server identifies itself as private which is odd (StatusChanged { old: Unknown, new: Private }), though that might be on my end. Still need to investigate further.

Never mind, this has been a mistake on my end. Public server successfully reports being public (StatusChanged { old: Unknown, new: Public("/ip4/XXX/tcp/4001") }). Private node reports being private (AutoNat(StatusChanged { old: Unknown, new: Private })).

🚀

Track all non-relayed addresses of the remote. On inbound dial-back
request select one of these addresses as observed one, reject dial-back
if there are none (i.g. all connections are relayed).
@elenaf9
Copy link
Contributor Author

elenaf9 commented Jan 12, 2022

One more note regarding the changed specs for autonat in libp2p/specs#386:

This restriction as well implies that implementations MUST NOT accept dial requests via relayed connections as one can
not validate the IP address of the requesting node

The request-response protocol does not bubble up the info on via what specific connection a request was received (if we have multiple active connections), so we can not check if that specific connection is relayed or not.
But with 2a73082 I changed it to track all addresses from active non-relayed connections. When there is at least one active non-relayed connection, we select the remote address as observed address, otherwise (if there are only relayed connections) we reject it.
From how I understood it, the main problem with relayed connections is that we can not know the actual address of the remote. If we have at least one non-relayed connection, that is not a problem, so I would think this fulfills the requirements from the quoted spec?

@mxinden
Copy link
Member

mxinden commented Jan 13, 2022

From how I understood it, the main problem with relayed connections is that we can not know the actual address of the remote. If we have at least one non-relayed connection, that is not a problem, so I would think this fulfills the requirements from the quoted spec?

Agreed that this fulfills the requirement. Thanks for double-checking.

@mxinden
Copy link
Member

mxinden commented Jan 13, 2022

protocols/autonat: override dial concurrency factor

That was fast. Thanks! I was about to comment.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

🎉 Ready to merge from my side.

Unless there are any further comments I will merge tomorrow.

@mxinden mxinden changed the title Basic AutoNAT protocols/: Add basic AutoNAT implementation Jan 13, 2022
@mxinden mxinden merged commit c61ea6a into libp2p:master Jan 14, 2022
@mxinden
Copy link
Member

mxinden commented Jan 14, 2022

🚀 Again, thanks @elenaf9 for the continuous work here. A very important piece of libp2p's hole punching story.

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jan 15, 2022
This commit adds a behaviour protocol that implements the AutoNAT specification.
It enables users to detect whether they are behind a NAT. The Autonat Protocol
implements a Codec for the Request-Response protocol, and wraps it in a new
Network Behaviour with some additional functionality.

Co-authored-by: David Craven <david@craven.ch>
Co-authored-by: Max Inden <mail@max-inden.de>
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
This commit adds a behaviour protocol that implements the AutoNAT specification.
It enables users to detect whether they are behind a NAT. The Autonat Protocol
implements a Codec for the Request-Response protocol, and wraps it in a new
Network Behaviour with some additional functionality.

Co-authored-by: David Craven <david@craven.ch>
Co-authored-by: Max Inden <mail@max-inden.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants