-
Notifications
You must be signed in to change notification settings - Fork 281
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 #638
add autonat v2 spec #638
Conversation
|
||
## AutoNAT V2 Protocol | ||
|
||
![Autonat V2 Interaction](autonat-v2.svg) |
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.
These diagrams need to be updated. They use “DialStatuses” and reference a undefined error.
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.
done.
autonat/autonat-v2.md
Outdated
|
||
`OK`: the server completed the request successfully. A request is considered | ||
completed successfully when the server either completes a dial(successfully or | ||
unsuccessfully) or rejects all addresses in the request as undialable. |
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.
If a server rejects all addresses as undialable, should it set OK or E_DIAL_REFUSED?
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.
good catch! it should be E_DIAL_REFUSED
IP address, the server asks the client to send some non trivial amount of bytes | ||
as a cost to dial a different IP address. To make amplification attacks | ||
unattractive, servers SHOULD ask for 30k to 100k bytes. Since most handshakes | ||
cost less than 10k bytes in bandwidth, 30kB is sufficient to make attacks |
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.
Linking to QUIC’s similar mitigation here would be helpful
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.
added some text.
as the server is free to use a separate peerID for the dial backs. | ||
|
||
Servers SHOULD determine whether they have IPv6 and IPv4 connectivity. IPv4 only servers SHOULD refuse requests for dialing IPv6 addresses and IPv6 only | ||
servers SHOULD refuse requests for dialing IPv4 addresses. |
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.
Do we currently handle this in go-libp2p?
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.
Yes. We use the black hole detector to tell us if we have IPv6 connectivity. https://github.com/libp2p/go-libp2p/blob/master/p2p/protocol/autonatv2/server.go#L183
55ae737
to
d9f46e6
Compare
|
||
== Amplification Attack Prevention == | ||
|
||
Cli -> Srv: [conn1: stream: dial] DialRequest:{nonce: 0xabcd, addrs: (addr1, addr2, addr3)} |
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 small nit, but can we 0-index these addrs too? addr0, addr1, addr2
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 pushed a couple small changes as well fyi
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.
🤷♂️
c0fdf37
to
92c9b64
Compare
--------- Co-authored-by: Marco Munizaga <[email protected]>
92c9b64
to
0a201f2
Compare
This is the same as #538. That PR targeted
autonat-rename
branch which has a commit to move the autonat v1 spec to a separate v1 specific file.Note: When merging, make sure to use "Rebase and Merge" to ensure git blame works correctly for autonat v1 spec.