-
Notifications
You must be signed in to change notification settings - Fork 272
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
add autonat v2 spec #538
base: autonat-rename
Are you sure you want to change the base?
add autonat v2 spec #538
Conversation
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.
Very nice, this is a solid starting point for the spec!
What's the plan for resolving #536? Would you open a new PR that targets this PR here?
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.
Exciting! Thanks for your work. Left some comments/questions :)
Sorry if they have already been answered somewhere.
thanks for your review @thomaseizinger. Proposal: use a list of addresses in priority order for autonat v2 dial requests #539 |
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.
Great work @sukunrt. Thank you!
I don't have anything to add to those at the moment :) |
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.
On a brief skim, this looks good! I'm curious if we'll want to relax the "implementations MUST NOT dial any multiaddress unless it is based on the IP address the requesting node is observed as". Would it be useful to do this, and we can mitigate the amplification attack some other way?
It seems like there's a healthy discussion already going on, so I'll step back here and let other folks stay involved. If there's anything I can help with, please don't hesitate to ping.
Thanks for your review @MarcoPolo
The suggested strategy is discussed here: #536 Here's the PR for those changes: #542 |
How is this putting a strain on servers? It’s the server that decides when to require this verification, and for every incoming request, the server has the option to reject it. |
Yes that's true. |
I need another clarification: In line 67 it is mentioned that every Or is only the |
Thanks umgefahren, I've fixed this. It should be |
on the stream |
Great, that's clear now. Another question, just to make sure: There will always be several And an additional suggestion: What is the upper bound for the size of |
Yes your understanding is correct, there'll be multiple DialDataResponse till you get the requested amount of data. You will have to track the amount of data received so far per dial-request stream. A peer can open multiple streams in parallel. Of course an implementation MAY choose to not allow this, but I don't think it is necessary to make this a MUST.
I will think about this. Currently this is only limited by the size of the Message which again is implementation dependent. I think most implementations have a limit lower than 8kB. I think it is fine to add a suggestion to limit the number of addresses inspected. |
Another quick question, I probably missed something: When the server successfully dials the client and provides the nonce. The client closes the stream either way. How does the server know if the provided nonce was correct? |
In the spec it says that all private IP address should be excluded, but it also says it's just for checking reachability on the public internet. That said, we should exclude: For IPv4:
For IPv6:
In the rust-libp2p implementation there was a PR discussing those globally reachable IP address: libp2p/rust-libp2p#3814 Also this list is probably not complete and not formal, it's also a small nitpick. |
@umgefahren it'd be better if you can add the comments as a review, commenting on the specific section.
It means non public. Happy to change the wording to
A correct server will always provide the correct nonce, no? This issue should be easy enough to debug for implementors without signalling from the client. Is there any benefit to the server knowing that it provided an incorrect nonce? |
I will do that the next time. I'm sorry.
Thanks for clarification.
I think there is a benefit. There is a pathological example where the network configuration or a NAT forward traffic to the wrong libp2p node. Not the one that requested the dial back, but a different one. In that case the server would report reachability on that address, but it's actually not reaching the peer in question. I'm not an expert enough here to think of any case where that might occur apart from a bad config or a malicious actor. |
In this case, the client sees that the server is reporting OK-Reachable but it has not received the nonce, so it should reject the response. |
If we dial the node with |
So since that is not possible, the implementation doesn't needs to handle this case, right? But thanks for the clarification and sorry for the dumb questions. |
Yep I think you are right! We can assume that this will never happen. Feel free to use
No worries at all! I think your questions are pretty spot on actually :) |
|
||
Cli -> Srv: [dial] DialRequest:{nonce: 0xabcd, addrs: (addr1, addr2, addr3)} | ||
Srv -> Cli: [attempt]addr2 DialAttempt:{nonce: 0xabcd} | ||
Srv -> Cli: [dial] DialResponse:{status: OK, dialStatuses:(E_TRANSPORT_NOT_SUPPORTED, OK)} |
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.
This mentions E_TRANSPORT_NOT_SUPPORTED
but that is missing from the protobufs?
While doing the rust-libp2p implementation, we discovered a race condition, which we are now circumventing by a 100ms delay. You can read the finally comment by @thomaseizinger here: umgefahren/rust-libp2p#1 (comment) It happens when the server successfully performs a dial back, thus sends the confirmation of the address back to the client. However the client hasn't progressed enough to be notified of that successful dial back when receiving the confirmation. In that case the client wrongly assumed an address was confirmed where no dial back occurred. |
Minor correction here: The behaviour is usually that the client discards the "successful" confirmation because it has not yet processed the dial-back so it thinks the server is sending it a confirmation without having actually done the dial. I think the correct way to solve this would be to add an ACK message from the client back to the server for the dial-back where the client can say: "Yes I've processed your dial-back". The server can then proceed to respond on the other stream and thus guarantee that we don't have a race condition between the two streams. |
You can read the closing of the stream as the ACK. See: https://github.com/libp2p/go-libp2p/blob/sukun/autonat-v2-2/p2p/protocol/autonatv2/server.go#L251-L257 The spec also dictates closing the stream: https://github.com/libp2p/specs/blame/autonat-v2/autonat/autonat-v2.md#L87 Do you think an explicit ACK is better? |
Yeah I think so. I associate closing a stream with "I have no more data to write". The client never writes data so why wouldn't it immediately close the stream? Also, reading a stream and waiting for that to fail because it has been closed it also somewhat odd 🤷♂️ |
That's a fair point. I'll add an ACK. |
Updated the specs with a |
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.
Nice, thank you!
First draft for autonat v2. #503
This protocol allows for testing reachability on exactly one address. This helps determine reachability at an address level. This also simplifies the protocol a lot.
I'll change the spec to reflect the discussion on dialing a different ip address from the nodes observed ip address: #536
Discussion for nonce in message is here: libp2p/go-libp2p#1480
and this comment in particular libp2p/go-libp2p#1480 (comment)