[release/8.0-staging] Ensure proper cleanup of key files when not persisting them #109845
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code inspection suggested that keys imported into the CNG MachineKey store from PFXImportCertStore were not getting properly cleaned up. This change adds tests that prove that supposition, and then fixes the bug so they pass.
* Bump keysize to 2048 * This caused the tests to be too slow, so reuse 6 random keys for all of them * Remove the random ordering in machine-or-user for defaultkeyset (try both ways) * Remove incorrect copy/paste comment * Remove bad nullable annotation
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
bartonjs
added
Servicing-approved
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #109731 to release/8.0-staging
/cc @bartonjs
Customer Impact
When loading a PKCS#12/PFX without specifying either the PersistKeySet or EphemeralKeySet option, the behavior of the framework is supposed to be to let the key be written to disk, and then remove the key when the cert is disposed/finalized.
When loading a PKCS#12/PFX to a single certificate (
new X509Certificate2(pfx, ...)orX509CertificateLoader.LoadPkcs12(...)), key files belonging to any certificates that aren't being returned should also be removed from disk (before the method returns).In both of these cases, key files written to the CNG machine key store are not being reliably cleaned up. Over time this causes system degradation as the cost of manipulating the directory increases.
Regression
The CNG cleanup code was added to .NET Core to support UWP, which didn't have machine keys. It just never got revisited.
CNG MachineKey keys do get properly cleaned up on .NET Framework, so there is a regression from NetFX.
Testing
This change adds tests that monitor the state of all known CAPI and CNG key directories and ensure that any files written during the import phase of the test are removed by the end of it.
Risk
Medium-Low.
The new tests ensure the behavior we expect. It is possible that there are a few users who have accidentally depended on the bad cleanup behavior and need to specify the PersistKeySet flag during import, but they are undoubtedly fewer in number than the users who don't want the keys persisted (and there's nothing the "don't want the keys persisted" users can easily do to correct the current bad behavior).