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 43 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.
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.
This is not a step that "the server performs", right?
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.
That's correct @mxinden. This was a committed suggestion, I'll go back a couple of commits to locate the disconnect.
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 see where this happened. I'll adjust and get @DannyS03 to review my edits.
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?