-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Kestrel SNI from config #24286
Kestrel SNI from config #24286
Conversation
Glad I didn't bet that you would get this done. |
/// <param name="serverOptionsCallback">Callback to configure HTTPS options.</param> | ||
/// <param name="state">State for the <see cref="ServerOptionsSelectionCallback" />.</param> | ||
/// <returns>The <see cref="ListenOptions"/>.</returns> | ||
internal static ListenOptions UseHttps(this ListenOptions listenOptions, ServerOptionsSelectionCallback serverOptionsCallback, object state = null) |
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.
Consider making this public. We want the config to use a consistent API with whatever the eventual public API for this callback is.
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.
@halter73 I'd like a full description of the behavior in the PR description:
- What is this feature?
- What is supported?
- Wildcards (how matching works)
- Explicit host names
- etc.
- Where/How it can fail?
- What APIs are exposed (if any)?
- How it composes with anything we already have built in?
- What the fallbacks are (I saw a discussion about falling back to the default certificate in some cases).
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.
Only minor feedback at the moment.
The big items that still need to be addressed:
- Should there be a global SNI config? E.g. so you can have 10 endpoints all with the same SNI settings.
- Making the new ServerOptionsSelectionCallback accessible in code.
src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs
Outdated
Show resolved
Hide resolved
So make it available in EndpointDefaults? |
Right |
95352fe
to
0403b56
Compare
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.
This looks really good. What's left on the draft?
29af02d
to
751f78a
Compare
src/Servers/Kestrel/Core/src/Internal/Certificates/ICertificateConfigLoader.cs
Show resolved
Hide resolved
8e59bf2
to
b6fbeed
Compare
- TODO: Add even more SniOptionsSelectorTests - Verify we clone all SslServerAuthenticationOptions properties - Verify every property from fallbackOptions are used correctly - TODO: Integration tests
b6fbeed
to
d326850
Compare
d326850
to
052bd57
Compare
src/Servers/Kestrel/test/InMemory.FunctionalTests/HttpsConnectionMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
- Hopefully this is it!
What is this feature?
What is supported?
Where/How it can fail?
What APIs are exposed (if any)?
internal static ListenOptions UseHttps(this ListenOptions listenOptions, ServerOptionsSelectionCallback serverOptionsCallback, object state = null)
could be made public once we figure out some kinks like how the handshake timeout can be configured with that overload.How it composes with anything we already have built in? (What the fallbacks are)
If an Endpoint doesn't have its own SNI section, but an "EndpointDefaults" SNI section does exist, the configuration logic behaves as if the SNI section was defined directly on the endpoint. This means that a "Certificate" defined directly on the endpoint could be overridden by a certificate defined in an "EndpointDefaults" SNI section.public KestrelConfigurationLoader Endpoint(string name, Action<EndpointConfiguration> configureOptions)
. Since there's no public API currently exposing SNI-specific config yet, only named-endpoint-wide configuration, non SNI-specific configuration" can be overridden this way."EndpointDefaults" and/or"Certificates:Default/Development" configuration will be used instead."EndpointDefaults" and/or"Certificates:Default/Development", configuration will fallback to what was configured viaKSO.ConfigureHttpsDefaults(Action<HttpsConnectionAdapterOptions> configureOptions)
orKSO.ConfigureEndpointDefaults(Action<ListenOptions> configureOptions)
.We decided against adding an SNI section to "EndpointDefaults", because certificates defined in "EndpointDefaults:SNI" it had the weird behavior of overriding "Certificate"s defined directly on endpoints or via
ListenOptions.UseHttps()
since certs defined via SNI config is preferred over normal certs.Addresses #15144