Prefer keys old enough to have propagated (dotnet#54309)

The code for choosing a fallback key when no default was found sensibly distinguished between keys that were old enough to have propagated to all instances and those that were not.  The code for choosing the preferred key should do the same.

This should help in the case where two instances both discover that a new key is needed and create an immediately-activated key.  One of them will get both keys back from the repository and it should choose the _older_ of the two to match the choice of of the other instance (which only knows about its own key).

Author: amcasey

src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs

@@ -73,11 +73,26 @@ private bool CanCreateAuthenticatedEncryptor(IKey key)
 
     private IKey? FindDefaultKey(DateTimeOffset now, IEnumerable<IKey> allKeys, out IKey? fallbackKey)
     {
-        // find the preferred default key (allowing for server-to-server clock skew)
-        var preferredDefaultKey = (from key in allKeys
-                                   where key.ActivationDate <= now + _maxServerToServerClockSkew
+        // 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
                                    orderby key.ActivationDate descending, key.KeyId ascending
-                                   select key).FirstOrDefault();
+                                   select key).Concat(from key in activatedKeys
+                                                      where key.CreationDate > propagationCutoff
+                                                      orderby key.ActivationDate ascending, key.KeyId ascending
+                                                      select key).FirstOrDefault();
 
         if (preferredDefaultKey != null)
         {
@@ -101,18 +116,17 @@ private bool CanCreateAuthenticatedEncryptor(IKey key)
         // 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.
 
-        // 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.
+        // As above, the two sort orders are opposite.
 
         // 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 key.CreationDate <= now - _keyPropagationWindow
+                                    where key.CreationDate <= propagationCutoff
                                     orderby key.CreationDate descending
                                     select key).Concat(from key in unrevokedKeys
-                                                       where key.CreationDate > now - _keyPropagationWindow
+                                                       where key.CreationDate > propagationCutoff
                                                        orderby key.CreationDate ascending
                                                        select key)
                        where CanCreateAuthenticatedEncryptor(key)

src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs

@@ -237,18 +237,77 @@ public void ResolveDefaultKeyPolicy_FallbackKey_NoNonRevokedKeysBeforePriorPropa
         Assert.True(resolution.ShouldGenerateNewKey);
     }
 
+    [Fact]
+    public void ResolveDefaultKeyPolicy_PropagatedKeyPreferred()
+    {
+        // Arrange
+        var resolver = CreateDefaultKeyResolver();
+
+        var now = ParseDateTimeOffset("2010-01-01 00:00:00Z");
+
+        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();
+
+        var now = ParseDateTimeOffset("2010-01-01 00:00:00Z");
+
+        var creation1 = now - TimeSpan.FromHours(1);
+        var creation2 = creation1 - TimeSpan.FromHours(1);
+        var activation1 = creation1;
+        var activation2 = creation2;
+        var expiration1 = creation1 + TimeSpan.FromDays(90);
+        var expiration2 = creation2 + TimeSpan.FromDays(90);
+
+        // Both active (key 1 more recently), neither propagated
+        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(key2, resolution.DefaultKey);
+        Assert.False(resolution.ShouldGenerateNewKey);
+    }
+
     private static IDefaultKeyResolver CreateDefaultKeyResolver()
     {
         return new DefaultKeyResolver(NullLoggerFactory.Instance);
     }
 
     private static IKey CreateKey(string activationDate, string expirationDate, string creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false)
+    {
+        return CreateKey(ParseDateTimeOffset(activationDate), ParseDateTimeOffset(expirationDate), creationDate == null ? (DateTimeOffset?)null : ParseDateTimeOffset(creationDate), isRevoked, createEncryptorThrows);
+    }
+
+    private static IKey CreateKey(DateTimeOffset activationDate, DateTimeOffset expirationDate, DateTimeOffset? creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false)
     {
         var mockKey = new Mock<IKey>();
         mockKey.Setup(o => o.KeyId).Returns(Guid.NewGuid());
-        mockKey.Setup(o => o.CreationDate).Returns((creationDate != null) ? DateTimeOffset.ParseExact(creationDate, "u", CultureInfo.InvariantCulture) : DateTimeOffset.MinValue);
-        mockKey.Setup(o => o.ActivationDate).Returns(DateTimeOffset.ParseExact(activationDate, "u", CultureInfo.InvariantCulture));
-        mockKey.Setup(o => o.ExpirationDate).Returns(DateTimeOffset.ParseExact(expirationDate, "u", CultureInfo.InvariantCulture));
+        mockKey.Setup(o => o.CreationDate).Returns(creationDate ?? DateTimeOffset.MinValue);
+        mockKey.Setup(o => o.ActivationDate).Returns(activationDate);
+        mockKey.Setup(o => o.ExpirationDate).Returns(expirationDate);
         mockKey.Setup(o => o.IsRevoked).Returns(isRevoked);
         if (createEncryptorThrows)
         {
@@ -261,6 +320,11 @@ private static IKey CreateKey(string activationDate, string expirationDate, stri
 
         return mockKey.Object;
     }
+
+    private static DateTimeOffset ParseDateTimeOffset(string dto)
+    {
+        return DateTimeOffset.ParseExact(dto, "u", CultureInfo.InvariantCulture);
+    }
 }
 
 internal static class DefaultKeyResolverExtensions