Load ClientCertificateMode from config #24076
Conversation
ghost
added
the
| httpsOptions.ServerCertificate = LoadCertificate(endpoint.Certificate, endpoint.Name) | ||
| ?? httpsOptions.ServerCertificate; | ||
|
|
||
| httpsOptions.ClientCertificateMode = ConfigurationReader.ClientCertificateMode ?? httpsOptions.ClientCertificateMode; |
There was a problem hiding this comment.
Organization: This was inserted between to related code sections dealing with server certs. Move it up above line 293.
| return endpoints; | ||
| } | ||
|
|
||
| private ClientCertificateMode? ReadClientCertificateMode() |
There was a problem hiding this comment.
This only supports a global setting, not a setting per endpoint?
There was a problem hiding this comment.
I couldn't find the code which options for each endpoint is being set from config. Could you point me to it please?
There was a problem hiding this comment.
There was a problem hiding this comment.
Uhhmm. I'm confused! EndpointConfig doesn't have a ClientCertificateMode property. Am I missing something? Is it even possible to set ClientCertificateMode per endpoint?
There was a problem hiding this comment.
EndpointConfig is an internal config construct, you should add ClientCertificateMode there.
| httpsOptions.SslProtocols = endpoint.SslProtocols.Value; | ||
| } | ||
|
|
||
| httpsOptions.ClientCertificateMode = endpoint.ClientCertificateMode ?? ConfigurationReader.ClientCertificateMode ?? httpsOptions.ClientCertificateMode; |
There was a problem hiding this comment.
I think we should apply the global ConfigurationReader.ClientCertificateMode before calling ApplyHttpsDefaults then apply endpoint.ClientCertificateMode afterwards. This way global config doesn't override what was done in code via ConfigureHttpsDefaults. Our handling of SslProtocols is very similar.
We should also add some KestrelConfigurationLoader tests to verify whatever behavior we decide on.
|
Thanks |
|
You're the best @Kahbazi thank you, a small quality of life improvement but much appreciated 🥇 |
amcasey
added
area-networking
5 participants
Fixes #18660