Skip to content

Enable ServerCertificateSelector for HTTP/3 #35243

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

Merged
merged 2 commits into from
Aug 10, 2021

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Aug 10, 2021

Contributes to #34858

ServerCertificateSelector is the only SNI callback we can support with the current quic APIs. All of the others return a full SslServerAuthenticationOptions per connection which isn't supported.

Note because the SNI callback happens before the connection is accepted, there is no ConnectionContext to pass to the callback (nor a MultiplexedConnectionContext). Invoking it with null seemed preferable to not supporting SNI at all.

@Tratcher Tratcher added this to the 6.0-rc1 milestone Aug 10, 2021
@Tratcher Tratcher self-assigned this Aug 10, 2021
@ghost ghost added the area-runtime label Aug 10, 2021
sslServerAuthenticationOptions.ServerCertificateSelectionCallback = (sender, host) =>
{
// There is no ConnectionContext available durring the QUIC handshake.
var cert = listenOptions.HttpsOptions.ServerCertificateSelector(null, host);
Copy link
Member

@halter73 halter73 Aug 10, 2021

Choose a reason for hiding this comment

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

Is there a way to associate an invocation of this callback with a specific QuicConnection instance? Would it make sense to create a fake ConnectionContext and copy the fake's FeatureCollection to the QuicConnectionContext? The Transport property and the Abort methods on the fake would likely need to throw, but I imagine the most common thing to use the ConnectionContext for in this callback is the FeatureCollection.

It seems better to try to at least partially fake it if possible rather than make existing code referencing the ConnectionContext in the callback nullref.

Copy link
Member Author

@Tratcher Tratcher Aug 10, 2021

Choose a reason for hiding this comment

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

Unfortunately not. The callback is invoked before the QuicConnection is even created and returned from Accept.

var quicConnection = await _listener.AcceptConnectionAsync(cancellationToken);
var connectionContext = new QuicConnectionContext(quicConnection, _context);

The Sender passed to ServerCertificateSelectionCallback is System.Net.Quic.QuicOptions.

If we made a fake context it would be completely empty.

I think we have to design some new callbacks in .NET 7 at both the System.Net.Quic and Kestrel levels that provide some kind of usable state in these callbacks.

Copy link
Member

Choose a reason for hiding this comment

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

If we made a fake context it would be completely empty.

That would be better than nothing if we copy features that were added out the the later-created QuicConnectionContext. But if we cannot even associate it with the later-created QuicConnectionContext, I guess we're stuck. Maybe we should to update the QuicListener API to make this possible somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think even associating the features is possible. I only added the callback in it's current state because I think it's still better than not supporting SNI at all, most use cases only require the host name.

Maybe we should to update the QuicListener API to make this possible somehow.

Exactly, that's the .NET 7 task.

Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

LGTM.

Remember to update aspnet/Announcements#444 with nullable change.

@Tratcher Tratcher merged commit a671f96 into dotnet:main Aug 10, 2021
@Tratcher Tratcher deleted the tratcher/h3/callback branch August 10, 2021 23:56
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants