Move computation of ShouldGenerateNewKey to KeyRingProvider (#54264)

* Move computation of ShouldGenerateNewKey to KeyRingProvider

It used to be the case that part of IDefaultKeyResolver.ResolveDefaultKeyPolicy's job was to determine whether the current default key was close enough to expiration that a new one ought to be generated.  This didn't make sense as the definition of "too close" depended upon the refresh period and propagation time of the ICacheableKeyRingProvider.  That is, the IDefaultKeyResolver had to make assumptions about how often it would be polled for changed.

The old logic was also very subtle and, as far as I was able to determine, slightly incorrect.  Formerly, the presence of any key activated prior to the current default key's expiration date and not expiring during the next propagation cycle was considered an acceptable replacement.  Several things seem strange about this:

1. The logic for finding a successor key is not the same as the logic for finding a preferred key (e.g. CanCreateAuthenticatedEncryptor is not checked)
2. The propagation window is counted forward from the current time, rather than backward from the expiration time
3. It's not immediately clear what happens if the successor key is unexpired at the end of the propagation window but expired before the default key's expiration time (maybe that's impossible or maybe that would be caught next refresh?)
4. As mentioned above, it doesn't seem like the resolver should know about the refresh period or make assumptions about how often it's called

Now, the ICacheableKeyRingProvider is responsible for determining whether the returned default key is close enough to expiration that a new key should be generated.  It checks whether the current time is within one propagation cycle of the expiration time, padding by an extra refresh period to account for the fact that we don't know where in the refresh cycle expiration will fall (i.e. so that we never generate a new key _less_ than a full propagation cycle ahead of when it's needed).

Part of #53654

* Don't repeat the second resolution after key generation

* Update comment

* Add explanatory comment

* Make comment more explicit

Author: amcasey

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]