Allow retries in DefaultKeyResolver.CanCreateAuthenticatedEncryptor #54711
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
ghost
added
the
|
This will probably have undesirable perf consequences if we don't also do #54675. |
amcasey
commented
Mar 23, 2024
src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs
Outdated
Show resolved
Hide resolved
dotnet-policy-service
bot
added
the
pending-ci-rerun
amcasey
commented
Apr 12, 2024
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Outdated
Show resolved
Hide resolved
|
Sadly, this does not actually work, in its present state. The key stores the decryption result in a Lazy, effectively caching the failure. The tests didn't catch this because I was mocking IKey and my mock didn't cache. Edit: with that fixed, this seems effective against simulated failures. PR updates pending. |
This code is trying to ensure that the selected key can be decrypted (i.e. is usable). It may fail if, for example, Azure KeyVault is unreachable due to connectivity issues. If it fails, there's a log message and then an immediately-activated key will be generated. An immediately-activated key can cause problems for sessions making requests to multiple app instances and those problems won't obviously be connected to the (almost silent) failure in CanCreateAuthenticatedEncryptor. Rather than effectively swallowing such errors, we should allow some retries. Part of dotnet#52678, which is part of dotnet#36157
|
Force push is a trivial rebase - diff should be the same. |
Retries against the actual `Key` type weren't working because the exception was getting cached in the key's lazy descriptor. Implement our own simple lazy and expose a method for clearing the cached value and exception.
src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs
Outdated
Show resolved
Hide resolved
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Show resolved
Hide resolved
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Show resolved
Hide resolved
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Show resolved
Hide resolved
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Outdated
Show resolved
Hide resolved
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Show resolved
Hide resolved
...tion/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs
Show resolved
Hide resolved
src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
Show resolved
Hide resolved
|
CI failure is in IIS shadow copy and is unrelated. It'll re-run next time I push, so I won't bother doing it now. |
captainsafia
approved these changes
Apr 19, 2024
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.
This code is trying to ensure that the selected key can be decrypted (i.e. is usable). It may fail if, for example, Azure KeyVault is unreachable due to connectivity issues. If it fails, there's a log message and then an immediately-activated key will be generated. An immediately-activated key can cause problems for sessions making requests to multiple app instances and those problems won't obviously be connected to the (almost silent) failure in CanCreateAuthenticatedEncryptor. Rather than effectively swallowing such errors, we should allow some retries.
Part of #36157