-
Notifications
You must be signed in to change notification settings - Fork 995
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
feat(noise): add WebTransport certhashes extension #3991
Conversation
7965a1e
to
213050d
Compare
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.
🚀 thank you for your work here! Overall this change looks good to me, though will defer another review until after reviewing #4015.
(Nit pick: Missing a changelog entry in transports/noise/CHANGELOG.md
.)
Out of curiosity, mind sharing your 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.
This looks really good, thank you!
It is also great to see the foundations for inbound muxer selection. That will shave off a roundtrip for all TCP connections.
I'd like to see a positive and a negative test for it to make sure it actually works and we don't break it in the future.
Very much in favor of the current design:
|
41c8c4c
to
224bede
Compare
I resolved all the issues, modified changelog, and added tests |
I need to use libp2p with WebTransport from a browser and wanted to implement the authentication. I can not disclose anything else for now. |
224bede
to
69ec3b5
Compare
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! A few comments about the test but otherwise good! :)
69ec3b5
to
c090e79
Compare
@thomaseizinger ready |
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 really good, thank you for the contribution!
I am not sure why mergify isn't merging this. I'll have to dig into it later. |
@Mergifyio refresh |
❌ Command disallowed due to command restrictions in the Mergify configuration.
|
@mxinden Something seems to be borked with mergify's permissions evaluation. Can you approve this PR? |
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.
Again, well done. I still have a couple of comments. Though nothing major.
0efc0a5
to
ea0ab8b
Compare
This pull request has merge conflicts. Could you please resolve them @oblique? 🙏 |
39f2981
to
cf3880b
Compare
This pull request has merge conflicts. Could you please resolve them @oblique? 🙏 |
cf3880b
to
1f519ad
Compare
rebased and solved merge conflicts |
1f519ad
to
649bd1e
Compare
Resolved all the comments |
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 clean, thank you for this work and your patience with the reviews :)
Good to go from my end. I'll defer to @mxinden for merging.
This comment was marked as resolved.
This comment was marked as resolved.
|
This is low risk, has passing tests and no unresolved comments. I'll merge this because it makes orchestrating the two PRs sitting on top easier. We can ship potential improvements spotted by @mxinden's final review as separate PRs. |
Approvals have been dismissed because the PR was updated after the send-it
label was applied.
Great to see this merged. Thank you! |
Description
The extension for WebTransport certhashes was added and can be configured via
Config::with_webtransport_certhashes
.In case of initiator, these certhashes will be used to validate the ones reported by responder. In case of responder, these certhashes will be reported to initiator.
Resolves #3988.
Notes & open questions
I had also implemented another way to resolve this issue, which was exposing certhashes via
Output<T>::webtransport_certhashes
. In that implementation the user (i.e. the WebTransport implementer) was responsible to check if the certhashes were valid, but I wasn't sure if I like it.With the code of this PR, the user needs to only define certhashes in the
Config
, so I find it less error prone. However it might be less future proof.Change checklist