Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
libp2p-webrtc-browser-to-server blog post #18
libp2p-webrtc-browser-to-server blog post #18
Changes from 45 commits
2286498
514db39
a627e96
31b1550
eac587c
fccf8b9
3da034a
ef35faf
e9b7253
23d6b5b
90d2674
9dac046
047bae8
a51f0c9
56093d2
09f8d98
b82bbdd
e990ab8
e824967
636e455
00c18e3
81876a4
1c39d6d
1ba1293
8c729d5
66fc8f3
2390141
6c4f12b
4c50e05
9b42b76
3c85be9
f6fbfd3
04a1514
99e0678
9196b43
9cfafb4
a6caa0d
75d1542
f6a7e43
07fc6aa
67774a5
260fb2c
acf7c8c
fd0b8ed
249d769
fc28c41
afa75de
b5f855c
fcfc991
67482d2
7459fae
60aa500
35eee32
f639c08
82631a6
527ba38
8e997f9
b3f188e
058bf88
2a1f04b
733fe9d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Don't forget to add a date. Otherwise it seems to show up as the last post on the blog.
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.
@p-shahi should we populate this now, or is it's population part of a release prcoess?
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 think a single update right before merging is the way to go. I am not aware of a formal "release process".
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.
@ddimaria This is still missing.
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 read @mxinden's comment as the date should be added immediately before merging, so this is intentionally unaddressed until we get the green light to merge. If you have a date that you think it will be merged ahead of time, I can add that in? I can optionally add today's date and just update again prior to merging? I don't have a strong opinion on any approach.
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.
Something I'm not seeing answered in this text flow is: if we don't use these WebRTC protocols, how do we determine public IP 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.
Agreed. Also technically it's not correct, a browser uses a STUN binding request to get its public ip (server reflexive candidate) from the server node using ICE Lite.
Wdyt of expanding on the wording used on the spec?
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.
@BigLep as this section is about WebRTC built into the browser, I didn't touch on not-using WebRTC protocols. Were you thinking of an alternative networking stack to use in lieu of WebRTC for browser-based real time communication in cases where STUN/TURN aren't options? Conversely, if we're talking about "private" servers in the context of libp2p, we may want to keep that in the subsequent section (
WebRTC in 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.
@jxs can you elaborate on
server node
in the context of using traditional WebRTC in the browser? That spec entry relates to a lbp2p use case.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.
yeah sure David, sorry if was not clear on the comment above.
In the context of traditional WebRTC in the browser a server node is usually a SFU or a MCU. As they are deployed to public reachable addresses they can offer ICE Lite implementation and therefore act as STUN agent that the browser can then use to get its public IP.
The aforementioned paragraph describes the WebRTC protocol from the browser perspective and mentions STUN and TURN as tools that "peers need to learn their public IP address and any router restrictions along the path that would prohibit peer-to-peer communication." but then it mentions that in libp2p we don't use these two protocols whereas in fact, in the libp2p context the publicly available WebRTC libp2p server node offers the same ICE Lite via
"a=ice-lite"
session-level attribute in its SDP offer see the Rust case for example.The browser then uses a STUN binding request to get its public ip address from the same libp2p server node it is connecting to.
Does this help make it clear?
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.
To answer the initial question directly: A libp2p WebRTC server discovers its public IP address by either (a) being told by the operator directly or (b) through the identify protocol when connecting via WebRTC to another public server.
On the meta level, I think the root of the confusion here is that the blog post is about libp2p WebRTC browser-to-server while this paragraph is about vanilla WebRTC browser-to-browser.
To move forward here, I suggest either detailing the vanilla browser-to-server flow via ICE lite and SFUs / MCUs as suggested by @jxs, and not the vanilla browser-to-browser flow, or drop this and the following paragraphs entirely.
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.
Thanks for clarifying @jxs and @mxinden, I made the assumption of browser to browser in this section to inform the reader about the typical WebRTC use case (and one they'd be most familiar with). SFUs/MCUs are common of course, and necessary for certain functionality (e.g. recording) and optimizations, though add a lot of complexity to the 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.
Since WebRTC uses DTLS 1.2, the handshake takes 2 roundtrips. The diagram should probably reflect that.
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 can add that level of fidelity. IIRC there are 3 RTs for DTLS (6 flights total), assuming no retransmissions. I'm wondering if it would be OK to add some messaging around this w/o 6 lines (noisy) 🤔
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 added a box beneath the handshake to provide info about the 3 RTs.
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 I am not mistaken 3 round trips are only needed as a denial-of-service countermeasure, see https://www.rfc-editor.org/rfc/rfc6347#section-4.2.1.
I am not sure what the default in major implementations is, 3 RT or 2 RT.
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'll do some digging. This is a chart from the author of Pion:
I'll need to see how Chrome does this. I may need to have Wireshark observe the handshake to be certain.
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.
What's the server here? I'm surprised to see a Hello Verify Request here. That's an anti-DoS measure in DTLS (similar to the Retry mechanism in QUIC, for those familiar with the QUIC spec). It adds 1 RTT to the handshake. If that's either rust-libp2p or go-libp2p, it would be worth investigating how / if we can turn this off. In terms of DoS defense, it doesn't buy us a lot here since we already create state when we receive the STUN packet, so doing a DoS defense in DTLS arguably comes to late.
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.
The server is a Go libp2p server.
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.
Relevant Pion code:
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.
To summarize:
HelloVerifyRequest
, thus adding an additional RTT to the DTLS handshake (2 RTT -> 3 RTT).The discussion on whether to tackle the optimization to only send
HelloVerifyRequest
when being DOS attacked is out of scope for this blog post. Further, it is an implementation decision, that can be rolled out by server implementations independently.To move forward I suggest going with the conservative (current) 3 RTT (including
HelloVerifyRequest
), but adding a small note like the below: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 change has been made.
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 am not sure this adds a lot of value. I would assume that it confuses more than it helps. Not a strong opinion. Feel free to leave as is.
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.
@marten-seemann what are your thoughts? I know you wanted some inclusion of framing, but maybe my approach is overkill? Maybe removing the protobuf message structure will lessen any confusion?
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 might be missing some past discussion. Again, please feel free to ignore my comment.
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 would consider removing this seciton. I think it makes more sense in the connectivity website or the docs. Basically keep the post focused on WebRTC and leave education about other transports to other places.
Maybe we have a post at the end of the connectivity series that gives a textual overview of the pros and cons of the transports.
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 can see the benefits of keeping it as well as removing it. It really depends if a blog post reader use case includes reading the post in isolation (not reading other blog posts and/or not relying on external links). The additional information doesn't overwhelm the reader in this case, but offers an immediate compare/contrast experience. I'll let others weigh in on this.
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 would remove as I don't think this is providing more value in saying. This is the "Limitations" section.
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 need to think about this one as I don't have insight into the intent of the post. It makes sense to orient the reader and transition into the limitations and appropriate in a blog post, but would be out of place in a technical paper. Does anyone have direction on the stylistic goals?