Move computation of ShouldGenerateNewKey to KeyRingProvider

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

@@ -71,7 +71,7 @@ private bool CanCreateAuthenticatedEncryptor(IKey key)
         }
     }
 
-    private IKey? FindDefaultKey(DateTimeOffset now, IEnumerable<IKey> allKeys, out IKey? fallbackKey, out bool callerShouldGenerateNewKey)
+    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
@@ -87,59 +87,48 @@ private bool CanCreateAuthenticatedEncryptor(IKey key)
             if (preferredDefaultKey.IsRevoked || preferredDefaultKey.IsExpired(now) || !CanCreateAuthenticatedEncryptor(preferredDefaultKey))
             {
                 _logger.KeyIsNoLongerUnderConsiderationAsDefault(preferredDefaultKey.KeyId);
-                preferredDefaultKey = null;
             }
-        }
-
-        // Only the key that has been most recently activated is eligible to be the preferred default,
-        // and only if it hasn't expired or been revoked. This is intentional: generating a new key is
-        // an implicit signal that we should stop using older keys (even if they're not revoked), so
-        // activating a new key should permanently mark all older keys as non-preferred.
-
-        if (preferredDefaultKey != null)
-        {
-            // Does *any* key in the key ring fulfill the requirement that its activation date is prior
-            // to the preferred default key's expiration date (allowing for skew) and that it will
-            // remain valid one propagation cycle from now? If so, the caller doesn't need to add a
-            // new key.
-            callerShouldGenerateNewKey = !allKeys.Any(key =>
-               key.ActivationDate <= (preferredDefaultKey.ExpirationDate + _maxServerToServerClockSkew)
-               && !key.IsExpired(now + _keyPropagationWindow)
-               && !key.IsRevoked);
-
-            if (callerShouldGenerateNewKey)
+            else
             {
-                _logger.DefaultKeyExpirationImminentAndRepository();
+                fallbackKey = null;
+                return preferredDefaultKey;
             }
-
-            fallbackKey = null;
-            return preferredDefaultKey;
         }
 
         // If we got this far, the caller must generate a key now.
         // We should locate a fallback key, which is a key that can be used to protect payloads if
         // the caller is configured not to generate a new key. We should try to make sure the fallback
         // 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.
-        fallbackKey = (from key in (from key in allKeys
+
+        // 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 key.CreationDate <= now - _keyPropagationWindow
                                     orderby key.CreationDate descending
-                                    select key).Concat(from key in allKeys
+                                    select key).Concat(from key in unrevokedKeys
+                                                       where key.CreationDate > now - _keyPropagationWindow
                                                        orderby key.CreationDate ascending
                                                        select key)
-                       where !key.IsRevoked && CanCreateAuthenticatedEncryptor(key)
+                       where CanCreateAuthenticatedEncryptor(key)
                        select key).FirstOrDefault();
 
         _logger.RepositoryContainsNoViableDefaultKey();
 
-        callerShouldGenerateNewKey = true;
         return null;
     }
 
     public DefaultKeyResolution ResolveDefaultKeyPolicy(DateTimeOffset now, IEnumerable<IKey> allKeys)
     {
         var retVal = default(DefaultKeyResolution);
-        retVal.DefaultKey = FindDefaultKey(now, allKeys, out retVal.FallbackKey, out retVal.ShouldGenerateNewKey);
+        var defaultKey = FindDefaultKey(now, allKeys, out retVal.FallbackKey);
+        retVal.DefaultKey = defaultKey;
+        retVal.ShouldGenerateNewKey = defaultKey is null;
         return retVal;
     }
 }

src/DataProtection/DataProtection/src/KeyManagement/Internal/DefaultKeyResolution.cs

@@ -30,8 +30,12 @@ public struct DefaultKeyResolution
     public IKey? FallbackKey;
 
     /// <summary>
-    /// 'true' if a new key should be persisted to the keyring, 'false' otherwise.
+    /// True if the caller should generate and persist a new key to the keyring.
+    /// False if the caller should determine for itself whether to generate a new key.
     /// This value may be 'true' even if a valid default key was found.
     /// </summary>
+    /// <remarks>
+    /// Does not reflect the time to expiration of the default key, if there is one.
+    /// </remarks>
     public bool ShouldGenerateNewKey;
 }

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

@@ -66,31 +66,56 @@ private CacheableKeyRing CreateCacheableKeyRingCore(DateTimeOffset now, IKey? ke
 
         // Fetch the current default key from the list of all keys
         var defaultKeyPolicy = _defaultKeyResolver.ResolveDefaultKeyPolicy(now, allKeys);
-        if (!defaultKeyPolicy.ShouldGenerateNewKey)
+        var defaultKey = defaultKeyPolicy.DefaultKey;
+
+        // We shouldn't call CreateKey more than once, else we risk stack diving. Thus, we don't even
+        // check defaultKeyPolicy.ShouldGenerateNewKey.  However, this code path shouldn't get hit
+        // with ShouldGenerateNewKey true unless there was an ineligible key with an activation date
+        // slightly later than the one we just added. If this does happen, then we'll just use whatever
+        // key we can instead of creating new keys endlessly, eventually falling back to the one we just
+        // added if all else fails.
+        if (keyJustAdded != null)
         {
-            CryptoUtil.Assert(defaultKeyPolicy.DefaultKey != null, "Expected to see a default key.");
-            return CreateCacheableKeyRingCoreStep2(now, cacheExpirationToken, defaultKeyPolicy.DefaultKey, allKeys);
+            var keyToUse = defaultKey ?? defaultKeyPolicy.FallbackKey ?? keyJustAdded;
+            return CreateCacheableKeyRingCoreStep2(now, cacheExpirationToken, keyToUse, allKeys);
         }
 
-        _logger.PolicyResolutionStatesThatANewKeyShouldBeAddedToTheKeyRing();
+        // Determine whether we need to generate a new key
+        bool shouldGenerateNewKey;
+        if (defaultKeyPolicy.ShouldGenerateNewKey || defaultKey == null)
+        {
+            shouldGenerateNewKey = true;
+        }
+        else
+        {
+            // If we have a default key, we have to consider its expiration date.  We have to generate a replacement
+            // if it will expire within the propagation window starting now (so that all other consumers pick up the
+            // replacement before the current default key expires).  However, we also have to factor in the refresh
+            // period, since we need to ensure that key generation occurs during the refresh that *precedes* the
+            // propagation window ending at the expiration date.
+            var minExpirationDate = now + KeyManagementOptions.KeyRingRefreshPeriod + KeyManagementOptions.KeyPropagationWindow;
+            var defaultKeyExpirationDate = defaultKey.ExpirationDate;
+            shouldGenerateNewKey =
+                defaultKeyExpirationDate < minExpirationDate &&
+                    (_defaultKeyResolver.ResolveDefaultKeyPolicy(defaultKeyExpirationDate, allKeys).DefaultKey is not { } nextDefaultKey ||
+                    nextDefaultKey.ExpirationDate < minExpirationDate);
+        }
 
-        // We shouldn't call CreateKey more than once, else we risk stack diving. This code path shouldn't
-        // get hit unless there was an ineligible key with an activation date slightly later than the one we
-        // just added. If this does happen, then we'll just use whatever key we can instead of creating
-        // new keys endlessly, eventually falling back to the one we just added if all else fails.
-        if (keyJustAdded != null)
+        if (!shouldGenerateNewKey)
         {
-            var keyToUse = defaultKeyPolicy.DefaultKey ?? defaultKeyPolicy.FallbackKey ?? keyJustAdded;
-            return CreateCacheableKeyRingCoreStep2(now, cacheExpirationToken, keyToUse, allKeys);
+            CryptoUtil.Assert(defaultKey != null, "Expected to see a default key.");
+            return CreateCacheableKeyRingCoreStep2(now, cacheExpirationToken, defaultKey, allKeys);
         }
 
+        _logger.PolicyResolutionStatesThatANewKeyShouldBeAddedToTheKeyRing();
+
         // At this point, we know we need to generate a new key.
 
         // We have been asked to generate a new key, but auto-generation of keys has been disabled.
         // We need to use the fallback key or fail.
         if (!_keyManagementOptions.AutoGenerateKeys)
         {
-            var keyToUse = defaultKeyPolicy.DefaultKey ?? defaultKeyPolicy.FallbackKey;
+            var keyToUse = defaultKey ?? defaultKeyPolicy.FallbackKey;
             if (keyToUse == null)
             {
                 _logger.KeyRingDoesNotContainValidDefaultKey();
@@ -103,7 +128,10 @@ private CacheableKeyRing CreateCacheableKeyRingCore(DateTimeOffset now, IKey? ke
             }
         }
 
-        if (defaultKeyPolicy.DefaultKey == null)
+        // We're going to generate a new key.  You'd think we could just take for granted what effect
+        // this would have on the final result, but the key resolver is an extension point, so we have
+        // to give it a chance to weigh in - hence the recursive call, triggering re-resolution.
+        if (defaultKey == null)
         {
             // The case where there's no default key is the easiest scenario, since it
             // means that we need to create a new key with immediate activation.
@@ -115,7 +143,7 @@ private CacheableKeyRing CreateCacheableKeyRingCore(DateTimeOffset now, IKey? ke
             // If there is a default key, then the new key we generate should become active upon
             // expiration of the default key. The new key lifetime is measured from the creation
             // date (now), not the activation date.
-            var newKey = _keyManager.CreateNewKey(activationDate: defaultKeyPolicy.DefaultKey.ExpirationDate, expirationDate: now + _keyManagementOptions.NewKeyLifetime);
+            var newKey = _keyManager.CreateNewKey(activationDate: defaultKey.ExpirationDate, expirationDate: now + _keyManagementOptions.NewKeyLifetime);
             return CreateCacheableKeyRingCore(now, keyJustAdded: newKey); // recursively call
         }
     }

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

@@ -74,7 +74,7 @@ public void ResolveDefaultKeyPolicy_ValidExistingKey_AllowsForClockSkew_AllKeysI
     }
 
     [Fact]
-    public void ResolveDefaultKeyPolicy_ValidExistingKey_NoSuccessor_ReturnsExistingKey_SignalsGenerateNewKey()
+    public void ResolveDefaultKeyPolicy_ValidExistingKey_NoSuccessor_ReturnsExistingKey_DoesNotSignalGenerateNewKey()
     {
         // Arrange
         var resolver = CreateDefaultKeyResolver();
@@ -85,11 +85,11 @@ public void ResolveDefaultKeyPolicy_ValidExistingKey_NoSuccessor_ReturnsExisting
 
         // Assert
         Assert.Same(key1, resolution.DefaultKey);
-        Assert.True(resolution.ShouldGenerateNewKey);
+        Assert.False(resolution.ShouldGenerateNewKey); // Does not reflect pending expiration
     }
 
     [Fact]
-    public void ResolveDefaultKeyPolicy_ValidExistingKey_NoLegitimateSuccessor_ReturnsExistingKey_SignalsGenerateNewKey()
+    public void ResolveDefaultKeyPolicy_ValidExistingKey_NoLegitimateSuccessor_ReturnsExistingKey_DoesNotSignalGenerateNewKey()
     {
         // Arrange
         var resolver = CreateDefaultKeyResolver();
@@ -102,7 +102,7 @@ public void ResolveDefaultKeyPolicy_ValidExistingKey_NoLegitimateSuccessor_Retur
 
         // Assert
         Assert.Same(key1, resolution.DefaultKey);
-        Assert.True(resolution.ShouldGenerateNewKey);
+        Assert.False(resolution.ShouldGenerateNewKey); // Does not reflect pending expiration
     }
 
     [Fact]