Skip to content
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

Fix: Inconsistent certificate chain handling between endpoint and default configuration #60710

Merged

Conversation

jnjudge1
Copy link
Contributor

@jnjudge1 jnjudge1 commented Mar 3, 2025

Fix: Inconsistent certificate chain handling between endpoint and default configuration

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Changes Kestrel configuration to process default certificate loading configurations the same as per endpoint configurations. With this change, certificates specified in the default configuration section will have their chains presented on the server even if their intermediates are not present in the system certificate store.

Description

IHttpsConfigurationService.cs:

  • Changed to add the CertificateChain property onto the internal CertificateAndConfig struct, necessary for passing cert chain from TlsConfigurationLoader to KestrelConfigurationLoader.

TlsConfigurationLoader.cs:

  • Changed to use the certificate chain from the loaded default certificate to return a CertificateAndConfig object with the chain specified if the chain is not null.

KestrelConfigurationLoader.cs:

  • Changed to add an internal DefaultCertificateChain property for specifying the default certificate chain to load on endpoints in KestrelServerOptions.ApplyDefaultCertificate.

KestrelServerOptions.cs:

  • Changed to get the default certificate chain specified in KestrelConfigurationLoader and set the ServerCertificateChain property on the httpsOptions for an endpoint.

Fixes #60709

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Mar 3, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 3, 2025
@jnjudge1
Copy link
Contributor Author

jnjudge1 commented Mar 3, 2025

@jnjudge1 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@dotnet-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"

Contributor License Agreement

@dotnet-policy-service agree

@martincostello
Copy link
Member

FYI changing something like certificate handling this is almost certainly going to need tests to get accepted, particularly as it's a bug fix.

@jnjudge1
Copy link
Contributor Author

jnjudge1 commented Mar 3, 2025

FYI changing something like certificate handling this is almost certainly going to need tests to get accepted, particularly as it's a bug fix.

Thanks for your feedback @martincostello ! I've added a test that fails on main but succeeds with this change.

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Nice find and appreciate the fix as well! Comments are just code cleanup nits.

It looks like it was probably an oversight when the cert chain work was done, don't see any reference as to why we ignored the chain which makes me think it was meant to be temporary and then forgotten about.

@jnjudge1
Copy link
Contributor Author

jnjudge1 commented Mar 4, 2025

Nice find and appreciate the fix as well! Comments are just code cleanup nits.

It looks like it was probably an oversight when the cert chain work was done, don't see any reference as to why we ignored the chain which makes me think it was meant to be temporary and then forgotten about.

Thanks @BrennanConroy ! That makes sense, happy to help here. I've gone back and resolved those nits, if all looks good please merge when possible. Thank you!

@BrennanConroy BrennanConroy merged commit f31188b into dotnet:main Mar 4, 2025
27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 4, 2025
@adityamandaleeka
Copy link
Member

Great work @jnjudge1!

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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kestrel inconsistent certificate chain handling between endpoint and default configuration
5 participants