Skip to content

Watch file-based certificates for changes #49979

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Aug 12, 2023
Merged

Watch file-based certificates for changes #49979

merged 17 commits into from
Aug 12, 2023

Conversation

@amcasey
Copy link
Member

@amcasey amcasey commented Aug 10, 2023

When IConfiguration specifies certificates by path and reloadOnChange is true, monitor those paths for changes and trigger reloads. The reload behavior is the same as if the path in the configuration file were changed, rather than the contents of the referenced file.

  • Does not apply to code-based configuration, which never triggers reloads
  • Does not trigger on deletion - that would just tear down the server
  • Does not look through symlinks to determine whether the target has changed
  • On by default (when reloadOnChange is true) but can be disabled with app context switch Microsoft.AspNetCore.Server.Kestrel.DisableCertificateFileWatching

Fixes #32351

@amcasey
Watch file-based certificates for changes
0f6d97d 
When `IConfiguration` specifies certificates by path and `reloadOnChange` is true, monitor those paths for changes and trigger reloads.  The reload behavior is the same as if the path in the configuration file were changed, rather than the contents of the referenced file.

- Does not apply to code-based configuration, which never triggers reloads
- Does not trigger on deletion - that would just tear down the server
- Does not look through symlinks to determine whether the target has changed
- On by default (when `reloadOnChange` is true) but can be disabled with app context switch `Microsoft.AspNetCore.Server.Kestrel.DisableCertificateFileWatching`

Fixes dotnet#32351
@ghost ghost added the area-runtime label Aug 10, 2023
@amcasey
Copy link
Member Author

amcasey commented Aug 10, 2023

I still have to write tests for the KestrelConfigurationLoader changes, but this is for RC1, so I wanted to give people time to review. I've validated that code manually, so I'm pretty confident.

@amcasey
Copy link
Member Author

amcasey commented Aug 10, 2023

Obviously, the scariest part is the locking (esp when events arrive during reload), so I'd appreciate some eyes on that. The biggest problem I'm aware of is that a flood of file changes (how many certs are you watching anyway?) could prolong reload, since there's no easy way to specify that it should have priority in the lock queue.

captainsafia
captainsafia reviewed Aug 10, 2023
View reviewed changes
BrennanConroy
BrennanConroy reviewed Aug 10, 2023
View reviewed changes
@amcasey
Copy link
Member Author

amcasey commented Aug 10, 2023 •
edited
Loading

I have to admit I'm surprised all the CI failures are on Windows. I suspect they just need to wait longer for the FS events.

Edit: doubling the timeout to 10 seconds didn't help (odd that exactly the same tests should fail) and I can't repro locally. I've speculatively modified the way I'm touching file.

Edit 2: I have a repro (of a related, but different test) on a helix VM, so I should at least be able to investigate.

BrennanConroy
BrennanConroy reviewed Aug 11, 2023
View reviewed changes
halter73
halter73 approved these changes Aug 11, 2023
View reviewed changes
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.

Looks good. I like how you used CertificateConfig.FileHasChanged so the right endpoints get restarted.

@amcasey amcasey enabled auto-merge (squash) August 11, 2023 21:58
@amcasey amcasey disabled auto-merge August 11, 2023 22:01
@amcasey amcasey requested review from a team and wtgodbe as code owners August 11, 2023 23:09
@amcasey amcasey enabled auto-merge (squash) August 11, 2023 23:09
@amcasey amcasey merged commit 76bb69a into dotnet:main Aug 12, 2023
@ghost ghost added this to the 8.0-rc1 milestone Aug 12, 2023
@craigktreasure craigktreasure mentioned this pull request Aug 14, 2023
Closed
@amcasey amcasey deleted the CertRotation branch August 14, 2023 16:16
@jozkee jozkee mentioned this pull request Aug 18, 2023
Closed
@amcasey amcasey mentioned this pull request Aug 21, 2023
Closed
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
@aelij aelij mentioned this pull request Sep 19, 2023
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JamesNK JamesNK JamesNK left review comments

@captainsafia captainsafia captainsafia left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@mgravell mgravell Awaiting requested review from mgravell

@wtgodbe wtgodbe Awaiting requested review from wtgodbe wtgodbe is a code owner

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

8.0-rc1

Development

Successfully merging this pull request may close these issues.

Support certificate auto-rotation in Kestrel

5 participants

@amcasey @halter73 @JamesNK @captainsafia @BrennanConroy