Skip to content
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

Merged
merged 3 commits into from
Jun 13, 2023

Conversation

oblique
Copy link
Contributor

@oblique oblique commented May 26, 2023

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

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@oblique oblique marked this pull request as draft May 26, 2023 17:47
@oblique oblique changed the title feat(noise): Validate WebTransport certhashes feat(noise): Add WebTransport certhashes extension May 26, 2023
@oblique oblique marked this pull request as ready for review May 26, 2023 19:18
Copy link
Member

@mxinden mxinden left a 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?

Copy link
Contributor

@thomaseizinger thomaseizinger left a 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.

transports/noise/src/io/handshake.rs Outdated Show resolved Hide resolved
transports/noise/src/io/handshake.rs Outdated Show resolved Hide resolved
transports/noise/src/io/handshake.rs Outdated Show resolved Hide resolved
transports/noise/src/io/handshake.rs Outdated Show resolved Hide resolved
transports/noise/src/lib.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

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.

Very much in favor of the current design:

  • Smaller public API
  • Less footguns
  • Works out of the box
  • We can solve future usecases when the arrive

@oblique oblique force-pushed the validate-certhashes branch 2 times, most recently from 41c8c4c to 224bede Compare June 1, 2023 11:23
@oblique
Copy link
Contributor Author

oblique commented Jun 1, 2023

I resolved all the issues, modified changelog, and added tests

@oblique
Copy link
Contributor Author

oblique commented Jun 1, 2023

@mxinden

Out of curiosity, mind sharing your use-case?

I need to use libp2p with WebTransport from a browser and wanted to implement the authentication. I can not disclose anything else for now.

Copy link
Contributor

@thomaseizinger thomaseizinger left a 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! :)

transports/noise/tests/webtransport_certhashes.rs Outdated Show resolved Hide resolved
transports/noise/tests/webtransport_certhashes.rs Outdated Show resolved Hide resolved
transports/noise/tests/webtransport_certhashes.rs Outdated Show resolved Hide resolved
transports/noise/tests/webtransport_certhashes.rs Outdated Show resolved Hide resolved
transports/noise/tests/webtransport_certhashes.rs Outdated Show resolved Hide resolved
transports/noise/tests/smoke.rs Show resolved Hide resolved
@oblique
Copy link
Contributor Author

oblique commented Jun 2, 2023

@thomaseizinger ready

Copy link
Contributor

@thomaseizinger thomaseizinger left a 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!

@thomaseizinger thomaseizinger changed the title feat(noise): Add WebTransport certhashes extension feat(noise): add WebTransport certhashes extension Jun 3, 2023
@thomaseizinger
Copy link
Contributor

I am not sure why mergify isn't merging this. I'll have to dig into it later.

@thomaseizinger thomaseizinger removed the request for review from mxinden June 3, 2023 06:15
@thomaseizinger
Copy link
Contributor

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2023

refresh

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • any of:
    • sender-permission>=write
    • sender={{author}}

@thomaseizinger
Copy link
Contributor

@mxinden Something seems to be borked with mergify's permissions evaluation. Can you approve this PR?

Copy link
Member

@mxinden mxinden left a 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.

transports/noise/src/io/handshake.rs Outdated Show resolved Hide resolved
transports/noise/src/lib.rs Outdated Show resolved Hide resolved
transports/noise/Cargo.toml Outdated Show resolved Hide resolved
transports/noise/src/io/handshake.rs Outdated Show resolved Hide resolved
transports/noise/src/lib.rs Outdated Show resolved Hide resolved
transports/noise/tests/smoke.rs Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Jun 5, 2023

This pull request has merge conflicts. Could you please resolve them @oblique? 🙏

@oblique oblique force-pushed the validate-certhashes branch 2 times, most recently from 39f2981 to cf3880b Compare June 5, 2023 10:53
@mergify
Copy link
Contributor

mergify bot commented Jun 8, 2023

This pull request has merge conflicts. Could you please resolve them @oblique? 🙏

@oblique
Copy link
Contributor Author

oblique commented Jun 8, 2023

rebased and solved merge conflicts

@oblique
Copy link
Contributor Author

oblique commented Jun 9, 2023

Resolved all the comments

thomaseizinger
thomaseizinger previously approved these changes Jun 9, 2023
Copy link
Contributor

@thomaseizinger thomaseizinger left a 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.

@thomaseizinger

This comment was marked as resolved.

@mergify
Copy link
Contributor

mergify bot commented Jun 12, 2023

⚠️ The sha of the head commit of this PR conflicts with #4064. Mergify cannot evaluate rules on this PR. ⚠️

@thomaseizinger
Copy link
Contributor

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.

@mergify mergify bot dismissed thomaseizinger’s stale review June 13, 2023 07:52

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 755e341 into libp2p:master Jun 13, 2023
@mxinden
Copy link
Member

mxinden commented Jun 19, 2023

Great to see this merged. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transport/noise: Implement NoiseExtensions
3 participants