From 25a860a04a24f79e8e3ac44a59af675b883a54d4 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Mon, 5 Aug 2024 21:09:13 -0700 Subject: [PATCH 1/2] Revert "Prefer keys old enough to have propagated (#54309)" This reverts commit e72eca7cfda111047223ea5e5171a724b6a4e544. It was a speculative improvement and now we have concrete evidence of (minor) problems. Fixes #57137 --- .../src/KeyManagement/DefaultKeyResolver.cs | 39 ++++++-------- .../KeyManagement/DefaultKeyResolverTests.cs | 54 ------------------- 2 files changed, 17 insertions(+), 76 deletions(-) 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 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..dd77fc84b807 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 @@ -238,60 +238,6 @@ 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); - } - [Fact] public void CreateEncryptor_NoRetryOnNullReturn() { From 65d0b0b3dcf8582b29b50ecbf82536851622f74e Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 6 Aug 2024 10:35:12 -0700 Subject: [PATCH 2/2] Add a targeted regression test --- .../KeyManagement/DefaultKeyResolverTests.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) 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 dd77fc84b807..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 @@ -238,6 +238,42 @@ public void ResolveDefaultKeyPolicy_FallbackKey_NoNonRevokedKeysBeforePriorPropa Assert.True(resolution.ShouldGenerateNewKey); } + [Fact] + public void ResolveDefaultKeyPolicy_OlderBadKeyVersusNewerGoodKey() + { + // 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. + + // 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, key2 can't be decrypted + var key1 = CreateKey(activation1, expiration1, creationDate: creation1); + var key2 = CreateKey(activation2, expiration2, creationDate: creation2, createEncryptorThrows: true); + + // Act + var resolution = resolver.ResolveDefaultKeyPolicy(now, [key1, key2]); + + // Assert + Assert.Same(key1, resolution.DefaultKey); + Assert.False(resolution.ShouldGenerateNewKey); + } + [Fact] public void CreateEncryptor_NoRetryOnNullReturn() {