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

http3: add a RoundTripOpt to check the server's SETTINGS frame #4355

Merged
merged 1 commit into from Mar 12, 2024

Conversation

marten-seemann
Copy link
Member

@marten-seemann marten-seemann commented Mar 9, 2024

Fixes #4160. Needed for #3522 and quic-go/webtransport-go#109.

For some requests, the client is required to check the server's HTTP/3 SETTINGS. For example, a client is only allowed to send HTTP/3 datagrams if the server explicitly enabled support.

SETTINGS are sent asynchronously on a control stream (usually the first unidirectional stream). This means that the SETTINGS might not be available at the beginning of the connection. This is not expected to be the common case, since the server can send the SETTINGS in 0.5-RTT data, but we have to be able to deal with arbitrary delays.

For WebTransport, there are even more SETTINGS values that the client needs to check. By making CheckSettings a callback on the RoundTripOpt, this entire validation logic can live at the WebTransport layer.


There's one thing that this API doesn't cover: While it allows the client to verify HTTP/3 settings, there's no way to perform any checks on any properties of the QUIC connection state. This will be necessary for WebTransport, as WebTransport will require support for the Reliable Stream Reset extension. One option is adding another RoundTripOpt callback: CheckConnectionState func(quic.ConnectionState) error. This callback would be called right after completion of the handshake, or, for 0-RTT requests, right away (the ConnectionState then reflects the QUIC transport parameters restored from the session ticket, not the ones negotiated on the new connection, but that's the price you pay for using 0-RTT).

For some requests, the client is required to check the server's HTTP/3
SETTINGS. For example, a client is only allowed to send HTTP/3 datagrams
if the server explicitly enabled support.

SETTINGS are sent asynchronously on a control stream (usually the first
unidirectional stream). This means that the SETTINGS might not be
available at the beginning of the connection. This is not expected to be
the common case, since the server can send the SETTINGS in 0.5-RTT data,
but we have to be able to deal with arbitrary delays.

For WebTransport, there are even more SETTINGS values that the client
needs to check. By making CheckSettings a callback on the RoundTripOpt,
this entire validation logic can live at the WebTransport layer.
@marten-seemann marten-seemann added this to the v0.42 milestone Mar 9, 2024
Copy link

codecov bot commented Mar 9, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.86%. Comparing base (ac12689) to head (d0c04f4).

Files Patch % Lines
http3/client.go 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4355      +/-   ##
==========================================
+ Coverage   84.81%   84.86%   +0.04%     
==========================================
  Files         150      150              
  Lines       14287    14303      +16     
==========================================
+ Hits        12117    12137      +20     
+ Misses       1671     1667       -4     
  Partials      499      499              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

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

This API is pretty good. I don't see any shortcomings really except the 1 thing you mentioned that some future changes might require the quic connection state too.

As a quality of life improvement we can include the settings check for Extended Connect in the RoundTripOpt implementation.
Individual protocols can check their individual settings like webtranport checks webtransport_max_sessions.

Comment on lines +22 to +26
// Support for HTTP/3 datagrams (RFC 9297)
EnableDatagram bool
// Extended CONNECT, RFC 9220
EnableExtendedConnect bool
// Other settings, defined by the application
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are EnableDatagram and EnableExtendedConnect treated separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? It's two different RFCs, so there's two different options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean why isn't settings typed as

Settings map[uint64]uint64

EnableDatagram / EnableExtendedConnect could also be part of the Other map

Copy link
Member Author

Choose a reason for hiding this comment

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

The Other is pretty much a hack, so that WebTransport can add its setting there. I'd much rather just have bools, and let the http3 package deal with serialization.

http3/client.go Show resolved Hide resolved
@marten-seemann
Copy link
Member Author

As a quality of life improvement we can include the settings check for Extended Connect in the RoundTripOpt implementation.

I like this idea! This would be proper client-side support for Extended CONNECT!

@marten-seemann marten-seemann merged commit 497d3f5 into master Mar 12, 2024
34 checks passed
Copy link
Collaborator

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

API lgtm, thanks!

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.

make the HTTP settings available to the client
3 participants