feat: stun protocol & stun connection #9
Conversation
| laddr*: TransportAddress # Local address | ||
| raddr*: TransportAddress # Remote address | ||
| dataRecv*: AsyncQueue[seq[byte]] # data received which will be read by DTLS | ||
| stunMsgs*: AsyncQueue[seq[byte]] # stun messages received and to be |
There was a problem hiding this comment.
Yes, those two queues are unbounded.
There was a problem hiding this comment.
There's nothing in the RFC saying anything about this. And I'm not confident enough to say There should be a limit and this limit is this number. So I leave things as they are because of my uncertainty.
There was a problem hiding this comment.
I'm not sure this is something always mentioned on specs, but I believe there should always be a limit to avoid memory DoS attacks. See more in https://github.com/libp2p/rust-libp2p/blob/master/docs/coding-guidelines.md#bound-everything.
| try: | ||
| let decoded = StunMessage.decode(await self.stunMsgs.popFirst()) | ||
| if not decoded.isFingerprintValid(): | ||
| # Fingerprint is invalid, the StunMessage received might be a false positive. |
There was a problem hiding this comment.
What does "might be a false positive" mean? Not a Stun message? Why is it moved to the dataRecv queue?
There was a problem hiding this comment.
Basically, there's a first check with the raw data (without decoding) where we check different things (the size of the message, the presence of the magic stun cookie etc...). When the incoming message is sorted, we can decode it. If something is wrong with the Fingerprint, it can means that it is, in fact, not a Stun Message but a message for another protocol. And the RFC specifies this by saying :
The FINGERPRINT attribute can aid in distinguishing STUN packets from
packets of other protocols. See [Section 7](https://datatracker.ietf.org/doc/html/rfc8489#section-7).```
There was a problem hiding this comment.
Can you briefly describe it in the comment and add the link? Why is it moved to the dataRecv queue?
There was a problem hiding this comment.
Why is it moved to the dataRecv queue?
Because StunConn uses two queues for received messages:
- one for Stun Messages
stunMsgswhich are decoded/answered/etc... instunMessageHandler - one for the others protocols
dataRecv(in the case of the WebRTC stack, it's DTLS messages), which are popped from the queue whenreadis called.
And, if the Fingerprint is wrong, it could be a false negative, which mean, it's not a Stun Message, but probably a DTLS message, thus it should be in dataRecv and not in stunMsgs
webrtc/stun/stun_connection.nim
Outdated
| except WebRtcError as exc: | ||
| trace "Failed to write the Stun response", error=exc.msg | ||
|
|
||
| proc init*( |
There was a problem hiding this comment.
It should be new as StunConn is a ref.
| ## | ||
| await self.closeEvent.wait() | ||
|
|
||
| proc close*(self: StunConn) = |
There was a problem hiding this comment.
Should we close the underlying UDP conn?
There was a problem hiding this comment.
Udp conn is a really bad name, it should be Udp Transport, I might change this in another PR.
But no, UdpConn is closed only when we close the Stun transport
webrtc/stun/stun_connection.nim
Outdated
| if self.closed: | ||
| debug "Try to close an already closed StunConn" | ||
| return | ||
| self.closed = true |
There was a problem hiding this comment.
Or maybe having a closing and closed, but not sure.
There was a problem hiding this comment.
I changed it, but as the proc is synchronous, it doesn't change anything
| import sequtils, typetraits, std/sha1 | ||
| import bearssl | ||
|
|
||
| proc generateRandomSeq*(rng: ref HmacDrbgContext, size: int): seq[byte] = |
There was a problem hiding this comment.
it is supposed to return a seq, but I believe it doesn't return anything, potentially nil.
There was a problem hiding this comment.
What do you mean? I initialize result. It definitely returns a seq.
There was a problem hiding this comment.
I find it confusing. There are 3 different ways of returning in Nim and it's used arbitrarily.
| rem = (rem shr 1) xor 0xedb88320'u32 | ||
| else: | ||
| rem = rem shr 1 | ||
| result[i] = rem |
There was a problem hiding this comment.
I'd recommend to avoid result and use explicit returns unless you have a very strong preference for that.
There was a problem hiding this comment.
Hum.... I like result actually, it's a neat tool
There was a problem hiding this comment.
I find it unusual and harder to reason. Another reason is that it is used arbitrarily in the procs. I believe it is better to follow the same pattern.
Presentation
This PR is a part of the stack to create the nim-libp2p webrtc-direct transport (defined here: https://github.com/libp2p/specs/blob/master/webrtc/webrtc-direct.md)
We implement a small version of the STUN protocol following https://datatracker.ietf.org/doc/html/rfc5389
As the RFC tells us: STUN
serves as a tool for other protocols in dealing with NAT traversal, thus we also implement a small version of the ICE Lite protocol following https://datatracker.ietf.org/doc/html/rfc8445.The STUN protocol reads and writes from the UdpConn. All the received STUN messages are treated as such and answered directly, all the other received messages are queued to be read by the underlying DTLS protocol.
Stun protocol
As said above, this STUN implementation isn't complete. At the moment of writing this PR, we are only interested in implementing the server part of the webrtc-direct transport. We will reconsider and improve the implementation when vacp2p/nim-libp2p#409 is addressed or if the webrtc-direct spec changes, for example. But for now on, there are some part missing.
Non-exhaustive list of what is missing:
ICE Lite protocol
For the same reasons as to why the STUN protocol isn't fully implemented (and because it's in the webrtc-direct spec), we use the Lite implementation of ICE. And again, only the server (ICE-CONTROLLED) side. As the server in the webrtc-direct transport must be publicly available, the Lite version should be sufficient.