Skip to content

Kestrel Endpoints' "SslProtocols" settable via config (#22663) #22910

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 3 commits into from
Jun 19, 2020

Conversation

fstaffa
Copy link
Contributor

@fstaffa fstaffa commented Jun 13, 2020

Kestrel Endpoints' "SslProtocols" settable via config (#22663)

Priority of using SslProtocols is:

  1. Code default
  2. Configuration from EndpointDefaults section
  3. Code configuration for named endpoint
  4. Configuration from named endpoint section

@ghost ghost added the area-servers label Jun 13, 2020
@fstaffa
Copy link
Contributor Author

fstaffa commented Jun 13, 2020

I have tested the changes manually and they work ok. I have tried to write tests for the changes in KestrelConfigurationLoader but I didn't find a way to do so.
I can test the ConfigurationBackedListOptions (https://github.com/fstaffa/aspnetcore/blob/add-kestrel-ssl-protocols-in-config/src/Servers/Kestrel/Core/src/KestrelServerOptions.cs#L27 ) but that does not really test that the SslProtocols were correctly applied.
What I would like to test is results of
(https://github.com/fstaffa/aspnetcore/blob/add-kestrel-ssl-protocols-in-config/src/Servers/Kestrel/Core/src/KestrelConfigurationLoader.cs#L335 ) since this is the place which actually passes the SslProtocols configuration (as a part of httpsOptions) to the server. Do you knowhow to do that?

I also realized that changing EndpointDefaults in config does not cause configuration to reload. Is that expected or should I create an issue for that?

@fstaffa fstaffa changed the title manually tested version Kestrel Endpoints' "SslProtocols" settable via config (#22663) Jun 13, 2020
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from f14119b to 0bc7f53 Compare June 14, 2020 10:18
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from 0bc7f53 to 4561eb9 Compare June 14, 2020 10:38
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

I'd like the nits addressed, but no big deal.

@halter73
Copy link
Member

Looks great. Thanks @fstaffa!

@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from fe5f099 to a093450 Compare June 16, 2020 21:52
@fstaffa fstaffa force-pushed the add-kestrel-ssl-protocols-in-config branch from a093450 to 8545552 Compare June 16, 2020 21:58
@Tratcher Tratcher merged commit a77e68f into dotnet:master Jun 19, 2020
@Tratcher
Copy link
Member

Thanks

@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.

5 participants