Skip to content

Conversation

@amcasey
Copy link
Member

@amcasey amcasey commented Feb 22, 2023

Tidy up some irregularities in certificate loading

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

Address some issues that arise when certificate configuration changes happen while the server is running.

Description

While validating #46296, I noticed some irregularities in the way certificates are reloaded when (e.g.) appsettings.json is changed while the server is running.

  1. If you start with the dev cert from the store and then light up, in sequence, the Development certificate, the Default certificate and an Endpoint certificate, each replaces its predecessor. If you then remove them in the reverse order, the Development certificate doesn't go away when it is removed from the configuration. This was because the KestrelServerOptions.DefaultCertificate wasn't being cleared. To improve clarity and address this issue, I've split out the possible sources of the default certificate - this makes the precedence explicit and allows the state of each to be managed separately.
  2. If you change the password of an endpoint certificate to an invalid value, reloading the configuration fails with an exception. Unfortunately, the endpoint has already been removed from the list, so when the password is correct, there's no record of it and the creation of its "replacement" fails because the port is already in use. I've deferred the updating of the endpoint list until after the dangerous parts of Reload have completed. (It's still not atomic, but failures in the actual state change code are too unlikely to justify additional complexity.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I split this out because I think it's nice to have invariants on the value of DefaultCertificate - in particular, that it's only set if IsDevCertLoaded is true. I could live with letting tests set the real one if people feel strongly.

Copy link
Member

Choose a reason for hiding this comment

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

So DefaultCertificate should be renamed to DevCertificate?

Copy link
Member Author

Choose a reason for hiding this comment

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

The naming isn't really consistent. At this point, it's the developer certificate retrieved from CertificateManager (or null).

Copy link
Member

Choose a reason for hiding this comment

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

Since you added these other properties to distinguish where the cert is coming from, it does make sense to rename DefaultCertificate to DeveloperCertificate.

@amcasey
Copy link
Member Author

amcasey commented Feb 23, 2023

CI failure looks legit - locally I have a dev cert in certmgr, but the CI machines don't. I'll figure out how to add that backstop to the tests.

Edit: fixed

1. Values from the user
2. Values set explicitly for test purposes
3. Values from IConfiguration
4. Values from CertificateManager

Note that these are all stored in separate places, so it's possible to "undo" changes if one goes away.

Also, make clearing of the IConfiguration cert on reload explicit.
Otherwise, a configuration error - e.g. a bad endpoint certificate password - could cause it to be left in a bad state, causing issues during subsequent reloads.
@amcasey
Copy link
Member Author

amcasey commented Feb 27, 2023

Force push is a rebase resolving (test-only) merge conflicts with #46868.

Copy link
Member

Choose a reason for hiding this comment

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

Since you added these other properties to distinguish where the cert is coming from, it does make sense to rename DefaultCertificate to DeveloperCertificate.

@amcasey amcasey merged commit 95ed45c into dotnet:main Mar 1, 2023
@amcasey amcasey deleted the CertLoading branch March 1, 2023 00:26
@ghost ghost added this to the 8.0-preview3 milestone Mar 1, 2023
@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.

4 participants