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

Webtransport #183

Merged
merged 101 commits into from
Jun 13, 2023
Merged

Webtransport #183

merged 101 commits into from
Jun 13, 2023

Conversation

darioalessandro
Copy link
Contributor

@darioalessandro darioalessandro commented Mar 29, 2023

Hi friends:

@ten3roberts and I are taking on adding this feature, we are not sure if we will keep it here or move it to a separate crate, but this works!!

I already added webtransport support to web apps written in rust via wasm-bindgen: rustwasm/wasm-bindgen#3344

Reddit demanded it: https://www.reddit.com/r/rust/comments/11rfes0/comment/jc8kyax/?utm_source=share&utm_medium=web2x&context=3

Work left

  • Move webtransport into its own crate
  • Server initiated Datagrams
  • client initiated Datagrams
  • Better api for incoming bi streams
  • Session termination
  • Server initiated uni streams
  • Server initiated bi streams
  • Request pass through (possibly as part of incoming bi streams)
  • Rebase onto latest changes

@ten3roberts
Copy link
Contributor

ten3roberts commented Mar 29, 2023

Hi again.

We were just in a discussion about forking and implementing webtransport per #71 here over at https://github.com/AmbientRun

What are your plans moving forwards and for the API?

I would like to help with this as much as I can.

Dropping the webtransport protocol draft for good measure

https://datatracker.ietf.org/doc/html/draft-ietf-webtrans-http3/

Furthermore, there is an implementation in python and go already, so may serve as inspiration.

python sample: https://github.com/GoogleChrome/samples/blob/gh-pages/webtransport/webtransport_server.py
go: https://github.com/adriancable/webtransport-go-example

@darioalessandro
Copy link
Contributor Author

darioalessandro commented Mar 29, 2023

Hey @ten3roberts !!!

Let's do this!

I am familiar with aioquic, I came from that lib but had to abandon it because I encountered many leaks, you are right, as of now it is kind of the "reference" implementation for server side webtransport.

So how about this:

Implement the bare minimal changes in this MR so that we end up with a webtransport server that works as an echo server and we can call from https://security-union.github.io/yew-webtransport/ Yew WebTransport client.

Right now I am looking at the new frame types that we have to add, tonight (Eastern time) I will break this down into tasks, we can tag team this project if you are interested 😄

btw I looked around the Ambient Run multiplayer game engine and it looks awesome!!

@griffobeid just what we were talking about!!

@seanmonstar
Copy link
Member

Thanks for starting this effort! I haven't kept track of the spec, so I wanted to ask: how much if it is a protocol on top of http3? Is it like websockets? Where it just uses some generic mechanism and otherwise is implemented on top (and this could be a separate library)? Maybe just a new setting, like we did with "extended connect" in h2? Or does it need a lot of integration?

@darioalessandro
Copy link
Contributor Author

darioalessandro commented Mar 29, 2023

Hey @seanmonstar it is extremely similar to websockets.

regarding settings, yes!! there are a couple new settings that the standard introduces like ENABLE_WEBTRANSPORT = 0x2B603742

@ten3roberts
Copy link
Contributor

My idea is to have a WebTransportSession type which is constructed using an h3::Connection and established the wt negotiation, similar to the go example listed earlier.

After awaiting the WebTransportSession it is used to await incoming streams, datagrams, and open new streams, very much like a quinn::Connection

Thoughts?

@darioalessandro I could work on the uni and bi streams of the protocol. Are you going to add me as a contributor to your fork or what is the best way to go about it?

@ten3roberts
Copy link
Contributor

On a further note: Should we expose a Config struct similar to what quiche does https://docs.rs/quiche/latest/quiche/h3/struct.Config.html to make the webtransport support optional, as all servers using h3 don't want to automatically broadcast and respond that yes, webtransport is indeed supported by the server.

This goes on with my previous comment about the session struct, rather than baking *WebTransport` events into the other existing http/3 events such as requests.

examples/Cargo.toml Outdated Show resolved Hide resolved
examples/server.rs Outdated Show resolved Hide resolved
@@ -109,6 +109,12 @@ where
settings
.insert(SettingId::MAX_HEADER_LIST_SIZE, max_field_section_size)
.map_err(|e| Code::H3_INTERNAL_ERROR.with_cause(e))?;
settings.insert(SettingId::ENABLE_CONNECT_PROTOCOL, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attempt to send the webtransport headers on the server side

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to know the release time of the webtransport server. I am really looking forward to your project

Copy link
Contributor Author

@darioalessandro darioalessandro Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @redoriental thanks for reaching out!!

I encourage you to try out the h3-webtransport crate by installing it from this branch by adding this to your cargo dependencies:

futures = "0.3"
h3-webtransport = { git= "https://github.com/security-union/h3.git", rev = "f4e83fb9be28ef3ea99e5feed725550f0169e5c2" } 

that way you wont have to wait for this PR to be merged.

We are currently working on separating h3 and h3-webtransport, and based on the ongoing discussions, it appears that this process may take some time.

The nature of open-source development can result in a slower pace, as contributors often have limited time to dedicate, and individuals from diverse backgrounds, opinions and cultures must come to a consensus on various objectives. However, rest assured that we will ultimately deliver a well-organized and efficient webtransport server!

h3/src/server.rs Outdated Show resolved Hide resolved
@darioalessandro
Copy link
Contributor Author

darioalessandro commented Mar 30, 2023

@ten3roberts

  1. I added you as maintainer, I am looking forward to working with you.

  2. Regarding config: how about adding a second parameter to h3::server::Connection::new with the config struct (to your point, similar to quiche), right now, it just takes que quinn connection:

    pub fn new(new_conn: NewConnection) -> Self {

  3. Do you want to get started with this? how should we divy up the work?

  4. I tried to decrypt webtransport traffic with wireshark but did not work, I am able to decrypt all other http3/quic traffic. Do you have a suggestion for tools?

@ten3roberts
Copy link
Contributor

Thanks, you too.

  1. That would be a very good idea, since the builder way to create a connection is less obvious/hidden, and being clear about what exists is usually good.

  2. I could work with anything really. We will need support for both the server and client part of the things. What do you think about my WebTransportSession type proposal, making a webtransport opt in? First order of business would be establishing a session on either side.

PS: My timezone is Z+0200 and will be working days between roughly 9am-5am
I'll fix the merge conflicts on my PR and then you can merge the PR and we can work on the same branch

@darioalessandro
Copy link
Contributor Author

@ten3roberts

  1. move forward with the WebTransportSession approach sounds good to me.
  2. With regards to the client, you can test with my ui https://security-union.github.io/yew-webtransport/ it is not perfect so do keep the console open (sometimes it does not show all errors)

@Ruben2424
Copy link
Contributor

Hi,

I don't want to mix everything up here, but I would like to share my thoughts.

I think it would be best to only implement http/3 in the h3 crate itself without extensions and only implement necessary parts to support extensions implemented in a separate crate (maybe extended connect I think @eagr started working on this in #159 but I don't know what the status is).
And to provide a API in order to be able to implement extensions to the protocol like WebTransport ontop.
Maybe some functionality to send Settings, Streams, etc. that the h3 don’t know and a way to receive these unknown types.

For WebTransport there are 2 Protocols needed. Http-Datagrams and WebTransport itself. I think both could be in a separate crate each.

I didn’t thought about this too much, so I maybe didn’t see some obvious problems with this proposal or there might be a better solution.

@inflation
Copy link
Contributor

Well, I think if there's another protocol upcoming, then that's the time we'd separate things and make a general extension mechanism. For now it's ok to include WebTransport but maybe behind a feature flag.

@darioalessandro
Copy link
Contributor Author

Hey @Ruben2424 @inflation !!

Thank you so much for your feedback.

I discussed with @ten3roberts and we are happy to move this to a different crate after we get it to work and just including the bare minimal in this repo.

also, thank you for mentioning #159 !

@Ruben2424
Copy link
Contributor

@Ruben2424 are there any features or traits, such as futures::AsyncRead that should be placed behind feature flags, or is it good as is?

I can't thing of a reason to put traits behind a feature flag. But I do not have much experience with those traits so I might missed something. What do you think @seanmonstar

@ten3roberts
Copy link
Contributor

@Ruben2424 I have now fixed all of your suggestions, moved config to a separate file, and fixed the msrv.

Can we re-run the CI?

Is there anything more or could this be merged? I am going to be available going onwards and will be able to fix and submit PR if any issue arises.

@ten3roberts
Copy link
Contributor

Getting an error about section-3.1 not exiting in the duvet report, but the link works.

Not sure if I've missed anything, I tried adding the link to the rfc in the ci/compliance/extract.sh but it didn't help

@ten3roberts
Copy link
Contributor

It seems to be as the link is not an RFC editor link. I removed the only offending section link for now.

The CI should now be all green

@ten3roberts
Copy link
Contributor

@Ruben2424 or @seanmonstar

Could we get a final CI and review? Is there anything more to address that I have missed?

Copy link
Contributor

@Ruben2424 Ruben2424 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the great work!
In my opinion this is ready to merge now.
I would like to wait with merging until @seanmonstar approved this too.

@darioalessandro
Copy link
Contributor Author

darioalessandro commented Jun 4, 2023

Btw, if you have a specific recommendation to keep imports as private as possible let me know.

I am also happy to keep webtransport as a module within h3 until we figure out how to split it.

@Ruben2424 @seanmonstar

I would like to ship it as soon as possible without creating too much tech debt

@Ruben2424
Copy link
Contributor

Btw, if you have a specific recommendation to keep imports as private as possible let me know.

I have some early ideas about creating one or multiple Extention traits. Then for Example the h3 Connection struct could hold a struct whicht impls the trait. H3 could call methods on this struct on specific events for example when the peer opens a stream.

Then only the traits and parameter types would need to be public and all of the internal structure could be private.

But that are just some early ideas from my side that might not work well.

In my opinion this should be explored after this PR was merged.

@darioalessandro
Copy link
Contributor Author

Sgtm let's go 🙌🙌🙌

@kixelated
Copy link

kixelated commented Jun 8, 2023

I migrated my project to this PR and it works great. I was using a quiche fork, but things started to get hairy without async support. Thank you for the hard work!

One problem I'm running into is access to the underlying QUIC library. I'd like to use SendStream.set_priority and eventually Connection.congestion_state. Any suggestions?

@Ruben2424
Copy link
Contributor

One problem I'm running into is access to the underlying QUIC library. I'd like to use SendStream.set_priority and eventually Connection.congestion_state. Any suggestions?

Hi @kixelated , You can try to clone() the quinn::Connection before constructing the h3::Connection and then call Connection.congestion_state on the clone.
I have no idea for the SendStream.set_priority right now. But fell free to open a issue to further discuss this.

@kixelated
Copy link

Hey @Ruben2424, thanks for the tip. I'll make an issue for set_priority after this gets merged.

@darioalessandro
Copy link
Contributor Author

darioalessandro commented Jun 12, 2023

@seanmonstar can we have another review please?

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the work you've done since I last reviewed is fantastic. It feels much more resilient to newer changes, and that "extending" is opt-in is excellent. Really great work! And thank you Ruben, your reviews helped accomplish that too!

I left some final thoughts inline, just a couple this time. They follow the same theme: protect the library from needing breaking changes, so we can optimize and change internals later.

h3/src/config.rs Show resolved Hide resolved
h3/src/error.rs Outdated Show resolved Hide resolved
h3/src/ext.rs Outdated Show resolved Hide resolved
@seanmonstar seanmonstar dismissed their stale review June 12, 2023 21:31

re-reviewed

@ten3roberts
Copy link
Contributor

@seanmonstar I have now addressed your thoughts, could we run the CI once more?

Let me know what you think

@ruffsl
Copy link

ruffsl commented Jun 13, 2023

Perhaps out of scope for the current PR, but would it be possible to later include a web transport client example to connect to the web transport server example? Something similar to the basic JavaScript client at https://googlechrome.github.io/samples/webtransport/client.html, as I'd like to add some additional system integration tests that don't always need a browser.

Thanks for this awesome work!

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it!

(Is there anything else before we click merge?)

ruffsl added a commit to rplayground/rwtrs that referenced this pull request Jun 13, 2023
@darioalessandro
Copy link
Contributor Author

@seanmonstar I think that there will be follow up PRs to polish this, imo this is good enough 🙌

@seanmonstar seanmonstar merged commit 22da938 into hyperium:master Jun 13, 2023
ten3roberts added a commit to security-union/sec-http3 that referenced this pull request Jun 28, 2023
* Initial Webtransport support (hyperium#183)


Co-authored-by: Tei Roberts <[email protected]>
Co-authored-by: Dario Lencina <[email protected]>

* Fix `cargo doc` warning (hyperium#193)

* chore: ensure no changes were missed from renamed files

* fix: certificate generation and instructions

* fix: feature snuck in during merge

---------

Co-authored-by: Dario A Lencina-Talarico <[email protected]>
Co-authored-by: Dario Lencina <[email protected]>
Co-authored-by: Chrislearn Young <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants