Revert "Prefer keys old enough to have propagated (#54309)" (#57186)

* Revert "Prefer keys old enough to have propagated (#54309)" This reverts commit e72eca7. It was a speculative improvement and now we have concrete evidence of (minor) problems. Fixes #57137 * Add a targeted regression test

Author: amcasey

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,18 +186,19 @@ 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
  // keep trying until we find one that works.
  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)

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);
  }