diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
index 903e41b73e31..c16c8842ad12 100644
--- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
+++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs
@@ -147,26 +147,20 @@ private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining)
private IKey? FindDefaultKey(DateTimeOffset now, IEnumerable<IKey> allKeys, out IKey? fallbackKey)
{
- // Keys created before this time should have propagated to all instances.
- var propagationCutoff = now - _keyPropagationWindow;
-
- // Prefer the most recently activated key that's old enough to have propagated to all instances.
- // If no such key exists, fall back to the *least* recently activated key that's too new to have
- // propagated to all instances.
-
- // An unpropagated key can still be preferred insofar as we wouldn't want to generate a replacement
- // for it (as the replacement would also be unpropagated).
-
- // Note that the two sort orders are opposite: we want the *newest* key that's old enough
- // (to have been propagated) or the *oldest* key that's too new.
- var activatedKeys = allKeys.Where(key => key.ActivationDate <= now + _maxServerToServerClockSkew);
- var preferredDefaultKey = (from key in activatedKeys
- where key.CreationDate <= propagationCutoff
+ // find the preferred default key (allowing for server-to-server clock skew)
+
+ // Note: another approach would be to prefer the *least-recently* activated key if none
+ // are old enough to have been propagated. We tried that and reverted it because it
+ // made us less resilient against bad keys. This way, we'll reject a bad key and generate
+ // a replacement and then the next instance to run will pick up the replacement, rather
+ // than the bad key. It did have the conceptual advantage of being more similar to the
+ // fallback code below and the hypothetical advantage of making it easier for instances
+ // to choose the same key in the event of a race (though we never managed to show that
+ // empirically. See also https://github.com/dotnet/aspnetcore/issues/57137.
+ var preferredDefaultKey = (from key in allKeys
+ where key.ActivationDate <= now + _maxServerToServerClockSkew
orderby key.ActivationDate descending, key.KeyId ascending
- select key).Concat(from key in activatedKeys
- where key.CreationDate > propagationCutoff
- orderby key.ActivationDate ascending, key.KeyId ascending
- select key).FirstOrDefault();
+ select key).FirstOrDefault();
var decryptRetriesRemaining = _maxDecryptRetries;
@@ -192,7 +186,8 @@ where key.CreationDate > propagationCutoff
// key has propagated to all callers (so its creation date should be before the previous
// propagation period), and we cannot use revoked keys. The fallback key may be expired.
- // As above, the two sort orders are opposite.
+ // Note that the two sort orders are opposite: we want the *newest* key that's old enough
+ // (to have been propagated) or the *oldest* key that's too new.
// Unlike for the preferred key, we don't choose a fallback key and then reject it if
// CanCreateAuthenticatedEncryptor is false. We want to end up with *some* key, so we
@@ -200,10 +195,10 @@ where key.CreationDate > propagationCutoff
var unrevokedKeys = allKeys.Where(key => !key.IsRevoked);
fallbackKey = (from key in (from key in unrevokedKeys
where !ReferenceEquals(key, preferredDefaultKey) // Don't reconsider it as a fallback
- where key.CreationDate <= propagationCutoff
+ where key.CreationDate <= now - _keyPropagationWindow
orderby key.CreationDate descending
select key).Concat(from key in unrevokedKeys
- where key.CreationDate > propagationCutoff
+ where key.CreationDate > now - _keyPropagationWindow
orderby key.CreationDate ascending
select key)
where CanCreateAuthenticatedEncryptor(key, ref decryptRetriesRemaining)
diff --git a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs
index 581ff83b9b24..8fa6fb6c5742 100644
--- a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs
+++ b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs
@@ -239,35 +239,17 @@ public void ResolveDefaultKeyPolicy_FallbackKey_NoNonRevokedKeysBeforePriorPropa
}
[Fact]
- public void ResolveDefaultKeyPolicy_PropagatedKeyPreferred()
+ public void ResolveDefaultKeyPolicy_OlderBadKeyVersusNewerGoodKey()
{
- // Arrange
- var resolver = CreateDefaultKeyResolver();
-
- var now = ParseDateTimeOffset("2010-01-01 00:00:00Z");
+ // In https://github.com/dotnet/aspnetcore/issues/57137, we encountered an issue
+ // where we selected the oldest unpropagated key, even though it could not be decrypted.
+ // Choosing the oldest unpropagated key makes sense in principle, but we were late
+ // enough in the release cycle that it made more sense to revert to the older behavior
+ // (preferring the most recently activated key, regardless of propagation) than to
+ // attempt a forward fix (i.e. harden the fancier logic against decryption failures).
+ // This test is intended to ensure that the bad key is never chosen, regardless of
+ // how we order the activated keys in the future.
- var creation1 = now - KeyManagementOptions.KeyPropagationWindow;
- var creation2 = now;
- var activation1 = now + TimeSpan.FromMinutes(1);
- var activation2 = activation1 + TimeSpan.FromMinutes(1); // More recently activated, but not propagated
- var expiration1 = creation1 + TimeSpan.FromDays(90);
- var expiration2 = creation2 + TimeSpan.FromDays(90);
-
- // Both active (key 2 more recently), key 1 propagated, key 2 not
- var key1 = CreateKey(activation1, expiration1, creationDate: creation1);
- var key2 = CreateKey(activation2, expiration2, creationDate: creation2);
-
- // Act
- var resolution = resolver.ResolveDefaultKeyPolicy(now, [key1, key2]);
-
- // Assert
- Assert.Same(key1, resolution.DefaultKey);
- Assert.False(resolution.ShouldGenerateNewKey);
- }
-
- [Fact]
- public void ResolveDefaultKeyPolicy_OlderUnpropagatedKeyPreferred()
- {
// Arrange
var resolver = CreateDefaultKeyResolver();
@@ -280,15 +262,15 @@ public void ResolveDefaultKeyPolicy_OlderUnpropagatedKeyPreferred()
var expiration1 = creation1 + TimeSpan.FromDays(90);
var expiration2 = creation2 + TimeSpan.FromDays(90);
- // Both active (key 1 more recently), neither propagated
+ // Both active (key 1 more recently), neither propagated, key2 can't be decrypted
var key1 = CreateKey(activation1, expiration1, creationDate: creation1);
- var key2 = CreateKey(activation2, expiration2, creationDate: creation2);
+ var key2 = CreateKey(activation2, expiration2, creationDate: creation2, createEncryptorThrows: true);
// Act
var resolution = resolver.ResolveDefaultKeyPolicy(now, [key1, key2]);
// Assert
- Assert.Same(key2, resolution.DefaultKey);
+ Assert.Same(key1, resolution.DefaultKey);
Assert.False(resolution.ShouldGenerateNewKey);
}