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

Clarify use of E_DIAL_REFUSED in autonat #430

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+6 −0
Open

Clarify use of E_DIAL_REFUSED in autonat #430

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
6 changes: 6 additions & 0 deletions autonat/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ If all dials fail, the receiver sends a `DialResponse` message with the
successfully, it sends a `DialResponse` with the `ResponseStatus` `OK`. It
SHOULD include the address it successfully dialed in its response.

If the receiver elects not to dial any addresses from the message - for example
because they are invalid, because they resolve to the same host as the receiver,
because they only include private addresses, because it does not support the
Comment on lines +82 to +83
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
because they are invalid, because they resolve to the same host as the receiver,
because they only include private addresses, because it does not support the
because they are invalid, because it does not support the

because they only include private addresses

In rust-libp2p we don't reject requests in this case. Directly after starting to listen, a peer won't know any of their external addresses and instead just their listening addresses (which are all private). The server still attempts to dial them, based on the address it observes this peer on, i.e. it replaces the IP of each address in the message with the observed IP. We do this anyway for DDoS protection (see vacp2p/nim-libp2p#739 (comment); @Tanguy suggested that we also add a section on for this, which imo would make sense).
Only if the observed address is private we may reject the peer (depending on whether this was set in our config or not).

because they resolve to the same host as the receiver

Again, because we replace the IP with the observed one at least in rust-libp2p we don't really handle this case.

Not sure how I think about having an exemplary list of rejection reasons. They seem rather go-lib2p2p specific right now. I'd propose to either properly specify in which cases a DialRequest shall be rejected, or keep it on a higher level to leave it up to the implementation (see suggestion).

relevant transports, or because it has no capacity, is sends a `DialResponse`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
relevant transports, or because it has no capacity, is sends a `DialResponse`
relevant transports, or because it has no capacity, it sends a `DialResponse`

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
relevant transports, or because it has no capacity, is sends a `DialResponse`
relevant transports, or because it has no capacity, it SHOULD send a `DialResponse`

We should probably add that you can’t trust on your peer doing this correctly - the Go implementation has been sending the wrong error code for a long time (until Elena fixed it recently).

@marten-seemann What do you think about using the keyword SHOULD, in the sense that it is recommended but one can not trust that the other peer actually does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, this is a MUST. You should be able to trust your peer's reporting here.

In practice you can't. But that's just because the Go implementation was just not compliant with the spec, which brought us to the mess we're in right now (one more reason for Autonat v2!). I suggest adding a note here that this was the case.

message with the `ResponseStatus` `E_DIAL_REFUSED`.

The initiator uses the responses obtained from multiple peers to determine its
NAT status. If more than 3 peers report a successfully dialed address, the node
SHOULD assume that it is not located behind a NAT and publicly accessible. On
Expand Down