Reuse known keys in XmlKeyManager #54490
Merged
Conversation
This file contains hidden or 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
We weren't actually using polymorphism and this makes cloning easier.
Keys read from the IXmlRepository (as opposed to created) use deferred decryption but may still be decrypted multiple times. Since they're immutable (other than revocation), keep track of the keys we've seen and reuse them next time they're returned from the IXmlRepository. In the absence of deletion support, we don't expect the list of keys to ever shrink, so there's no reason to prune the cache. While this is likely a perf win, it's a small perf win and only once a day, so it would probably not be worthwhile on that basis alone. However, this should also reduce the number of calls to `IXmlDecryptor.Decrypt` (which may, e.g., make an Azure KeyVault request). Each time the key ring is refreshed, it requests all keys from the key manager, clobbering all keys that may have already been decrypted (at least the default key will have been). In the single app-instance case, for example, there should be no need to call Decrypt at all for any key created during the current execution of the app (old keys will have to be decrypted the first time they're used). This shouldn't affect security because (a) the key ring already stores all the `IKeys` all the time and (b) `IKey` wraps the actual data in an `ISecret`. It would have been nice to pass the key ring's list of known keys to the key manager, rather than having it store its own cache, but the key ring's storage format would require abstraction violations that would work poorly with our public extension points.
ghost
added
the
amcasey
commented
Mar 11, 2024
| @@ -3,30 +3,156 @@ | |||
|
|
|||
| using System; | |||
| using System.Collections.Generic; | |||
| using System.Xml.Linq; | |||
There was a problem hiding this comment.
It would have been more sensible to treat KeyBase as the diff baseline, but I don't think I can alter that. Basically, Key and DeferredKey were just constructors, so I moved those two constructors into the base and made the existing constructor private.
amcasey
commented
Mar 11, 2024
| IsRevoked = true; | ||
| } | ||
|
|
||
| internal Key Clone() |
amcasey
commented
Mar 11, 2024
| /// <summary> | ||
| /// The basic implementation of <see cref="IKey"/>, where the <see cref="IAuthenticatedEncryptorDescriptor"/> | ||
| /// has already been created. | ||
| /// </summary> | ||
| public Key( |
There was a problem hiding this comment.
I could be persuaded that the constructors should be private and there should be Eager and Deferred factory methods.
dotnet-policy-service
bot
added
the
pending-ci-rerun
captainsafia
approved these changes
Apr 1, 2024
|
This did a very good job of mitigating simulated IXmlDecryptor (e.g. AKV) errors. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Keys read from the IXmlRepository (as opposed to created) use deferred decryption but may still be decrypted multiple times. Since they're immutable (other than revocation), keep track of the keys we've seen and reuse them next time they're returned from the IXmlRepository.
In the absence of deletion support, we don't expect the list of keys to ever shrink, so there's no reason to prune the cache.
While this is likely a perf win, it's a small perf win and only once a day, so it would probably not be worthwhile on that basis alone. However, this should also reduce the number of calls to
IXmlDecryptor.Decrypt(which may, e.g., make an Azure KeyVault request). Each time the key ring is refreshed, it requests all keys from the key manager, clobbering all keys that may have already been decrypted (at least the default key will have been). In the single app-instance case, for example, there should be no need to call Decrypt at all for any key created during the current execution of the app (old keys will have to be decrypted the first time they're used).This shouldn't affect security because (a) the key ring already stores all the
IKeysall the time and (b)IKeywraps the actual data in anISecret.It would have been nice to pass the key ring's list of known keys to the key manager, rather than having it store its own cache, but the key ring's storage format would require abstraction violations that would work poorly with our public extension points.