-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add async ServerOptionsSelectionCallback UseHttps overload #25390
Conversation
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
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.
No tests or samples?
26139ea
to
a9436ea
Compare
Approved for RC1 pending CI completion if merged before 10am Pacific on 2020-09-01. |
Thanks for adding this. We're using I had to spend some time to figure this out and to get it working with HTTP/2. Posting some sample code here for the benefit of others trying to use We needed Also would be great to have access to listenOptions.UseHttps(ServerOptionsSelectionCallback, state: listenOptions.Protocols);
ValueTask<SslServerAuthenticationOptions> ServerOptionsSelectionCallback(
SslStream stream,
SslClientHelloInfo clientHelloInfo,
object state,
CancellationToken cancellationToken)
{
var serverOptions = new SslServerAuthenticationOptions
{
EnabledSslProtocols = SslProtocols.Tls12 | SslProtocols.Tls13,
CertificateRevocationCheckMode = X509RevocationMode.NoCheck,
ServerCertificateContext = SslStreamCertificateContext.Create(certificate, intermediateCertificates, offline: true)
};
//ConfigureAlpn / Enable Http2
var httpProtocols = (HttpProtocols)state;
ConfigureAlpn(serverOptions, httpProtocols);
return new ValueTask<SslServerAuthenticationOptions>(serverOptions);
}
//Copy of method from:
//https://github.com/dotnet/aspnetcore/blob/main/src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
void ConfigureAlpn(SslServerAuthenticationOptions serverOptions, HttpProtocols httpProtocols)
{
serverOptions.ApplicationProtocols = new List<SslApplicationProtocol>();
// This is order sensitive
if ((httpProtocols & HttpProtocols.Http2) != 0)
{
serverOptions.ApplicationProtocols.Add(SslApplicationProtocol.Http2);
// https://tools.ietf.org/html/rfc7540#section-9.2.1
serverOptions.AllowRenegotiation = false;
}
if ((httpProtocols & HttpProtocols.Http1) != 0)
{
serverOptions.ApplicationProtocols.Add(SslApplicationProtocol.Http11);
}
} |
Hi @nitinag. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
The changes to HttpsConnectionMiddleware to support this UseHttps overload were made in #24286. I didn't want to slow down that PR with public API considerations, but now is the time to review the API.
This not only allows async SNI configuration, but allows configuring nearly any part of SslServerAuthenticationOptions asynchronously after getting the ClientHello.
Initially, I was a little concerned that this doesn't respect configuration made via ConfigureHttpsDefaults or EndpointDefaults in config, but then I noticed that this not unique to this UseHttps overload.
UseHttps(HttpsConnectionAdapterOptions)
has this same limitation, because unlikeUseHttps(Action<HttpsConnectionAdapterOptions>)
and like this new overload, there are no incoming options to pre-populate with defaults.Addresses #20981
@davidfowl @Tratcher