Transient failure to refresh a key ring produces a 500 response

[mdomashchenko]

Here's how it happens:

1. A request comes in, with a cookie. Cookie handler needs to decrypt the cookie, this ultimately results in a call to KeyRingBasedDataProtector.UnprotectCore()
2. UnprotectCore calls KeyRingProvider.GetCurrentKeyRingCore to get keys. The latter determines that it needs to refresh the key ring from the repository.
3. The repository experiences some transient failure and throws an exception. At this time the KeyRingProvider does have the key ring available, so it extends its lifetime for 2 minutes and reports that from here

   aspnetcore/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs

   Line 211 in 3341e46

   _logger.ErrorOccurredWhileRefreshingKeyRing(ex);

4. Now, instead of just returning the old key ring with freshly extended lifetime, KeyRingProvider re-throws the exception. Rationale for that is described here

   aspnetcore/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs

   Lines 229 to 231 in 3341e46

   	// The immediate caller should fail so that they can report the error up the chain. This makes it more likely
   	// that an administrator can see the error and react to it as appropriate. The caller can retry the operation
   	// and will probably have success as long as they fall within the temporary extension mentioned above.

5. The problem is, KeyRingBasedDataProtector.UnprotectCore does not retry. If it did, it would get the key ring, because it's lifetime was just extended. But it just doesn't make that second call to GetCurrentKeyRing

   aspnetcore/src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedDataProtector.cs

   Line 239 in 3341e46

   var currentKeyRing = _keyRingProvider.GetCurrentKeyRing();

6. So, the cookie is not decrypted, authentication handler fails and the error leaks to the caller as a 500 response, which is completely preventable.

Here's a stack trace for your reference

at Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.GetAllKeys()
at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.CreateCacheableKeyRingCore(DateTimeOffset now, IKey keyJustAdded)
at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.ICacheableKeyRingProvider.GetCacheableKeyRing(DateTimeOffset now)
at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingProvider.GetCurrentKeyRingCore(DateTime utcNow, Boolean forceRefresh)
at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.UnprotectCore(Byte[] protectedData, Boolean allowOperationsOnRevokedKeys, UnprotectStatus& status)

[ghost]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[HaoK]

Tracked as part of #33116

[ghost]

Thanks for contacting us.
We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[amcasey]

This is definitely worth fixing and we're planning to prioritize it in 9.0, but I don't think this particular issue adds new details to the general understanding of the key creation/synchronization race. Tracked in #36157 and elsewhere.