From 6a89fba1d684cbcbbc55fb0b2689d3c03f929441 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 22 Mar 2024 14:13:52 -0700 Subject: [PATCH 1/9] Allow retries in DefaultKeyResolver.CanCreateAuthenticatedEncryptor This code is trying to ensure that the selected key can be decrypted (i.e. is usable). It may fail if, for example, Azure KeyVault is unreachable due to connectivity issues. If it fails, there's a log message and then an immediately-activated key will be generated. An immediately-activated key can cause problems for sessions making requests to multiple app instances and those problems won't obviously be connected to the (almost silent) failure in CanCreateAuthenticatedEncryptor. Rather than effectively swallowing such errors, we should allow some retries. Part of #52678, which is part of #36157 --- .../src/KeyManagement/DefaultKeyResolver.cs | 72 ++++++++--- .../src/KeyManagement/KeyManagementOptions.cs | 19 +++ .../DataProtection/src/LoggingExtensions.cs | 3 + .../KeyManagement/DefaultKeyResolverTests.cs | 113 +++++++++++++++++- 4 files changed, 189 insertions(+), 18 deletions(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index 55d8df66bff4..265a99c49f41 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -3,7 +3,9 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; +using System.Threading; using Microsoft.AspNetCore.Cryptography; using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption; using Microsoft.AspNetCore.DataProtection.KeyManagement.Internal; @@ -28,6 +30,9 @@ internal sealed class DefaultKeyResolver : IDefaultKeyResolver /// private readonly TimeSpan _keyPropagationWindow; + private readonly int _maxDecryptRetries; + private readonly TimeSpan _decryptRetryDelay; + private readonly ILogger _logger; /// @@ -41,33 +46,65 @@ internal sealed class DefaultKeyResolver : IDefaultKeyResolver /// private readonly TimeSpan _maxServerToServerClockSkew; - public DefaultKeyResolver() - : this(NullLoggerFactory.Instance) + public DefaultKeyResolver(IOptions keyManagementOptions) + : this(keyManagementOptions, NullLoggerFactory.Instance) { } - public DefaultKeyResolver(ILoggerFactory loggerFactory) + public DefaultKeyResolver(IOptions keyManagementOptions, ILoggerFactory loggerFactory) { _keyPropagationWindow = KeyManagementOptions.KeyPropagationWindow; _maxServerToServerClockSkew = KeyManagementOptions.MaxServerClockSkew; + _maxDecryptRetries = keyManagementOptions.Value.MaximumDefaultKeyResolverRetries; + _decryptRetryDelay = keyManagementOptions.Value.DefaultKeyResolverRetryDelay; _logger = loggerFactory.CreateLogger(); } - private bool CanCreateAuthenticatedEncryptor(IKey key) + private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining) { - try + List? exceptions = null; + while (true) { - var encryptorInstance = key.CreateEncryptor(); - if (encryptorInstance == null) + try + { + var encryptorInstance = key.CreateEncryptor(); + if (encryptorInstance is null) + { + // This generally indicates a key (descriptor) without a corresponding IAuthenticatedEncryptorFactory, + // which is not something that can succeed on retry - return immediately + _logger.KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed( + key.KeyId, + nameof(IKey.CreateEncryptor), + new InvalidOperationException($"{nameof(IKey.CreateEncryptor)} returned null.")); + return false; + } + + return true; + } + catch (Exception ex) { - CryptoUtil.Fail("CreateEncryptorInstance returned null."); + if (exceptions is null) + { + // The first failure doesn't count as a retry + exceptions = new(); + } + else + { + retriesRemaining--; + } + + exceptions.Add(ex); + + if (retriesRemaining == 0) + { + _logger.KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed(key.KeyId, nameof(IKey.CreateEncryptor), new AggregateException(exceptions)); + return false; + } + + _logger.RetryingMethodOfKeyAfterFailure(key.KeyId, nameof(IKey.CreateEncryptor), ex); } - return true; - } - catch (Exception ex) - { - _logger.KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed(key.KeyId, nameof(IKey.CreateEncryptor), ex); - return false; + // Don't retry immediately + Thread.Sleep(_decryptRetryDelay); } } @@ -94,12 +131,14 @@ where key.CreationDate > propagationCutoff orderby key.ActivationDate ascending, key.KeyId ascending select key).FirstOrDefault(); + var decryptRetriesRemaining = _maxDecryptRetries; + if (preferredDefaultKey != null) { _logger.ConsideringKeyWithExpirationDateAsDefaultKey(preferredDefaultKey.KeyId, preferredDefaultKey.ExpirationDate); // if the key has been revoked or is expired, it is no longer a candidate - if (preferredDefaultKey.IsRevoked || preferredDefaultKey.IsExpired(now) || !CanCreateAuthenticatedEncryptor(preferredDefaultKey)) + if (preferredDefaultKey.IsRevoked || preferredDefaultKey.IsExpired(now) || !CanCreateAuthenticatedEncryptor(preferredDefaultKey, ref decryptRetriesRemaining)) { _logger.KeyIsNoLongerUnderConsiderationAsDefault(preferredDefaultKey.KeyId); } @@ -123,13 +162,14 @@ where key.CreationDate > propagationCutoff // 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 orderby key.CreationDate descending select key).Concat(from key in unrevokedKeys where key.CreationDate > propagationCutoff orderby key.CreationDate ascending select key) - where CanCreateAuthenticatedEncryptor(key) + where CanCreateAuthenticatedEncryptor(key, ref decryptRetriesRemaining) select key).FirstOrDefault(); _logger.RepositoryContainsNoViableDefaultKey(); diff --git a/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs b/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs index 0806ee2df9d7..05fc3c57b177 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs @@ -94,6 +94,25 @@ internal static TimeSpan MaxServerClockSkew } } + /// + /// During each default key resolution, if a key decryption attempt fails, + /// it can be retried, as long as the total number of retries across all keys + /// does not exceed this value. + /// + /// + /// Settable for testing. + /// + internal int MaximumDefaultKeyResolverRetries { get; set; } = 10; + + /// + /// Wait this long before each default key resolution decryption retry. + /// + /// + /// + /// Settable for testing. + /// + internal TimeSpan DefaultKeyResolverRetryDelay { get; set; } = TimeSpan.FromMilliseconds(200); + /// /// Controls the lifetime (number of days before expiration) /// for newly-generated keys. diff --git a/src/DataProtection/DataProtection/src/LoggingExtensions.cs b/src/DataProtection/DataProtection/src/LoggingExtensions.cs index cd4e38918b6d..f5f6587b4cf4 100644 --- a/src/DataProtection/DataProtection/src/LoggingExtensions.cs +++ b/src/DataProtection/DataProtection/src/LoggingExtensions.cs @@ -252,4 +252,7 @@ private static bool IsLogLevelEnabledCore([NotNullWhen(true)] ILogger? logger, L [LoggerMessage(72, LogLevel.Error, "The key ring's default data protection key {KeyId:B} has been revoked.", EventName = "KeyRingDefaultKeyIsRevoked")] public static partial void KeyRingDefaultKeyIsRevoked(this ILogger logger, Guid keyId); + + [LoggerMessage(73, LogLevel.Debug, "Key {KeyId:B} method {MethodName} failed. Retrying.", EventName = "RetryingMethodOfKeyAfterFailure")] + public static partial void RetryingMethodOfKeyAfterFailure(this ILogger logger, Guid keyId, string methodName, Exception exception); } 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 9d508785ba1d..84b40b0ba386 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 @@ -291,9 +291,113 @@ public void ResolveDefaultKeyPolicy_OlderUnpropagatedKeyPreferred() Assert.False(resolution.ShouldGenerateNewKey); } + [Fact] + public void CreateEncryptor_NoRetryOnNullReturn() + { + // Arrange + var resolver = CreateDefaultKeyResolver(); + + var now = ParseDateTimeOffset("2010-01-01 00:00:00Z"); + + var mockKey = new Mock(); + mockKey.Setup(o => o.KeyId).Returns(Guid.NewGuid()); + mockKey.Setup(o => o.CreationDate).Returns(now.AddDays(-3)); // Propagated + mockKey.Setup(o => o.ActivationDate).Returns(now.AddDays(-1)); // Activated + mockKey.Setup(o => o.ExpirationDate).Returns(now.AddDays(14)); // Unexpired + mockKey.Setup(o => o.IsRevoked).Returns(false); // Unrevoked + mockKey.Setup(o => o.CreateEncryptor()).Returns((IAuthenticatedEncryptor)null); // Anomalous null return + + // Act + var resolution = resolver.ResolveDefaultKeyPolicy(now, [mockKey.Object]); + + // Assert + Assert.Null(resolution.DefaultKey); + Assert.Null(resolution.FallbackKey); + Assert.True(resolution.ShouldGenerateNewKey); + + mockKey.Verify(o => o.CreateEncryptor(), Times.Once); // Not retried + } + + [Fact] + public void CreateEncryptor_FirstAttemptIsNotARetry() + { + // Arrange + var options = Options.Create(new KeyManagementOptions() + { + MaximumDefaultKeyResolverRetries = 3, + DefaultKeyResolverRetryDelay = TimeSpan.Zero, + }); + + var resolver = new DefaultKeyResolver(options, NullLoggerFactory.Instance); + + var now = ParseDateTimeOffset("2010-01-01 00:00:00Z"); + + var creation1 = now.AddDays(-3); + var activation1 = creation1.AddDays(2); + var expiration1 = creation1.AddDays(90); + + // Newer but still propagated => preferred + var creation2 = creation1.AddHours(1); + var activation2 = activation1.AddHours(1); + var expiration2 = expiration1.AddHours(1); + + var mockKey1 = CreateMockKey(activation1, expiration1, creation1, isRevoked: false, createEncryptorThrows: false); + var mockKey2 = CreateMockKey(activation2, expiration2, creation2, isRevoked: false, createEncryptorThrows: true); // Uses up all the retries + + // Act + var resolution = resolver.ResolveDefaultKeyPolicy(now, [mockKey1.Object, mockKey2.Object]); + + // Assert + Assert.Null(resolution.DefaultKey); + Assert.Same(mockKey1.Object, resolution.FallbackKey); + Assert.True(resolution.ShouldGenerateNewKey); + + mockKey1.Verify(o => o.CreateEncryptor(), Times.Once); // 1 try + mockKey2.Verify(o => o.CreateEncryptor(), Times.Exactly(4)); // 1 try plus max (3) retries + } + + [Fact] + public void CreateEncryptor_SucceedsOnRetry() + { + // Arrange + var options = Options.Create(new KeyManagementOptions() + { + MaximumDefaultKeyResolverRetries = 3, + DefaultKeyResolverRetryDelay = TimeSpan.Zero, + }); + + var resolver = new DefaultKeyResolver(options, NullLoggerFactory.Instance); + + var now = ParseDateTimeOffset("2010-01-01 00:00:00Z"); + + var creation = now.AddDays(-3); + var activation = creation.AddDays(2); + var expiration = creation.AddDays(90); + + var mockKey = CreateMockKey(activation, expiration, creation); + mockKey + .Setup(o => o.CreateEncryptor()) + .Returns(() => + { + // Added to list before the callback is called + if (mockKey.Invocations.Count(i => i.Method.Name == nameof(IKey.CreateEncryptor)) == 1) + { + throw new Exception("This method fails."); + } + return new Mock().Object; + }); + + // Act + var resolution = resolver.ResolveDefaultKeyPolicy(now, [mockKey.Object]); + + // Assert + Assert.Same(mockKey.Object, resolution.DefaultKey); + mockKey.Verify(o => o.CreateEncryptor(), Times.Exactly(2)); // 1 try plus 1 retry + } + private static IDefaultKeyResolver CreateDefaultKeyResolver() { - return new DefaultKeyResolver(NullLoggerFactory.Instance); + return new DefaultKeyResolver(Options.Create(new KeyManagementOptions()), NullLoggerFactory.Instance); } private static IKey CreateKey(string activationDate, string expirationDate, string creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false) @@ -302,6 +406,11 @@ private static IKey CreateKey(string activationDate, string expirationDate, stri } private static IKey CreateKey(DateTimeOffset activationDate, DateTimeOffset expirationDate, DateTimeOffset? creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false) + { + return CreateMockKey(activationDate, expirationDate, creationDate, isRevoked, createEncryptorThrows).Object; + } + + private static Mock CreateMockKey(DateTimeOffset activationDate, DateTimeOffset expirationDate, DateTimeOffset? creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false) { var mockKey = new Mock(); mockKey.Setup(o => o.KeyId).Returns(Guid.NewGuid()); @@ -318,7 +427,7 @@ private static IKey CreateKey(DateTimeOffset activationDate, DateTimeOffset expi mockKey.Setup(o => o.CreateEncryptor()).Returns(Mock.Of()); } - return mockKey.Object; + return mockKey; } private static DateTimeOffset ParseDateTimeOffset(string dto) From 1b48c842a65e50be14217b46b60f25152c7e9388 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 12 Apr 2024 14:43:14 -0700 Subject: [PATCH 2/9] Roll our own Lazy that allows resets Retries against the actual `Key` type weren't working because the exception was getting cached in the key's lazy descriptor. Implement our own simple lazy and expose a method for clearing the cached value and exception. --- .../src/KeyManagement/DefaultKeyResolver.cs | 3 + .../DataProtection/src/KeyManagement/Key.cs | 109 +++++++++++++-- .../KeyManagement/DefaultKeyResolverTests.cs | 124 +++++++++++++----- 3 files changed, 193 insertions(+), 43 deletions(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index 265a99c49f41..441c83ae642e 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -103,6 +103,9 @@ private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining) _logger.RetryingMethodOfKeyAfterFailure(key.KeyId, nameof(IKey.CreateEncryptor), ex); } + // Reset the descriptor to allow for a retry + (key as Key)?.ResetDescriptor(); + // Don't retry immediately Thread.Sleep(_decryptRetryDelay); } diff --git a/src/DataProtection/DataProtection/src/KeyManagement/Key.cs b/src/DataProtection/DataProtection/src/KeyManagement/Key.cs index 31870015c7f5..e8f5a02f6d3c 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/Key.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/Key.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Xml.Linq; using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption; using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ConfigurationModel; @@ -16,7 +17,13 @@ namespace Microsoft.AspNetCore.DataProtection.KeyManagement; /// internal sealed class Key : IKey { - private readonly Lazy _lazyDescriptor; + private IAuthenticatedEncryptorDescriptor? _descriptor; + + // If descriptor is available at construction time, these will remain null forever + private readonly object? _descriptorLock; // Protects _descriptor and _descriptorException + private readonly Func? _descriptorFactory; // May not be used + private Exception? _descriptorException; + private readonly IEnumerable _encryptorFactories; private IAuthenticatedEncryptor? _encryptor; @@ -36,8 +43,10 @@ public Key( creationDate, activationDate, expirationDate, - new Lazy(() => descriptor), - encryptorFactories) + encryptorFactories, + descriptor, + descriptorFactory: null, + descriptorException: null) { } @@ -57,8 +66,29 @@ public Key( creationDate, activationDate, expirationDate, - new Lazy(GetLazyDescriptorDelegate(keyManager, keyElement)), - encryptorFactories) + encryptorFactories, + descriptor: null, + descriptorFactory: GetLazyDescriptorDelegate(keyManager, keyElement), + descriptorException: null) + { + } + + // internal for testing + internal Key( + Guid keyId, + DateTimeOffset creationDate, + DateTimeOffset activationDate, + DateTimeOffset expirationDate, + IEnumerable encryptorFactories, + Func? descriptorFactory) + : this(keyId, + creationDate, + activationDate, + expirationDate, + encryptorFactories, + descriptor: null, + descriptorFactory: descriptorFactory, + descriptorException: null) { } @@ -67,15 +97,20 @@ private Key( DateTimeOffset creationDate, DateTimeOffset activationDate, DateTimeOffset expirationDate, - Lazy lazyDescriptor, - IEnumerable encryptorFactories) + IEnumerable encryptorFactories, + IAuthenticatedEncryptorDescriptor? descriptor, + Func? descriptorFactory, + Exception? descriptorException) { KeyId = keyId; CreationDate = creationDate; ActivationDate = activationDate; ExpirationDate = expirationDate; - _lazyDescriptor = lazyDescriptor; _encryptorFactories = encryptorFactories; + _descriptor = descriptor; + _descriptorFactory = descriptorFactory; + _descriptorException = descriptorException; + _descriptorLock = descriptor is null ? new() : null; } public DateTimeOffset ActivationDate { get; } @@ -92,7 +127,56 @@ public IAuthenticatedEncryptorDescriptor Descriptor { get { - return _lazyDescriptor.Value; + // We could check for _descriptorException here, but there's no reason to optimize that case + // (i.e. by avoiding taking the lock) + + if (_descriptor is not null) // Can only become less null, so losing a race here doesn't matter + { + Debug.Assert(_descriptorException is null); // Mutually exclusive with _descriptor + return _descriptor; + } + + lock (_descriptorLock!) + { + if (_descriptorException is not null) + { + throw _descriptorException; + } + + if (_descriptor is not null) + { + return _descriptor; + } + + Debug.Assert(_descriptorFactory is not null, "Key constructed without either descriptor or descriptor factory"); + + try + { + _descriptor = _descriptorFactory(); + return _descriptor; + } + catch (Exception ex) + { + _descriptorException = ex; + throw; + } + } + } + } + + internal void ResetDescriptor() + { + if (_descriptor is not null) + { + Debug.Fail("ResetDescriptor called with descriptor available"); + Debug.Assert(_descriptorException is null); // Mutually exclusive with _descriptor + return; + } + + lock (_descriptorLock!) + { + _descriptor = null; + _descriptorException = null; } } @@ -121,13 +205,16 @@ internal void SetRevoked() internal Key Clone() { + // Note that we don't reuse _descriptorLock return new Key( keyId: KeyId, creationDate: CreationDate, activationDate: ActivationDate, expirationDate: ExpirationDate, - lazyDescriptor: _lazyDescriptor, - encryptorFactories: _encryptorFactories) + encryptorFactories: _encryptorFactories, + descriptor: _descriptor, + descriptorFactory: _descriptorFactory, + descriptorException: _descriptorException) { IsRevoked = IsRevoked, }; 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 84b40b0ba386..ba5616965d0d 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 @@ -3,6 +3,7 @@ using System.Globalization; using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption; +using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ConfigurationModel; using Microsoft.AspNetCore.DataProtection.KeyManagement.Internal; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; @@ -299,23 +300,30 @@ public void CreateEncryptor_NoRetryOnNullReturn() var now = ParseDateTimeOffset("2010-01-01 00:00:00Z"); - var mockKey = new Mock(); - mockKey.Setup(o => o.KeyId).Returns(Guid.NewGuid()); - mockKey.Setup(o => o.CreationDate).Returns(now.AddDays(-3)); // Propagated - mockKey.Setup(o => o.ActivationDate).Returns(now.AddDays(-1)); // Activated - mockKey.Setup(o => o.ExpirationDate).Returns(now.AddDays(14)); // Unexpired - mockKey.Setup(o => o.IsRevoked).Returns(false); // Unrevoked - mockKey.Setup(o => o.CreateEncryptor()).Returns((IAuthenticatedEncryptor)null); // Anomalous null return + int descriptorFactoryCalls = 0; + + // Retries don't work with mock keys + var key = new Key( + Guid.NewGuid(), + creationDate: now.AddDays(-3), // Propagated + activationDate: now.AddDays(-1), // Activated + expirationDate: now.AddDays(14), // Unexpired + encryptorFactories: [], // Causes CreateEncryptor to return null + descriptorFactory: () => + { + descriptorFactoryCalls++; + throw new InvalidOperationException("Shouldn't be called"); + }); // Act - var resolution = resolver.ResolveDefaultKeyPolicy(now, [mockKey.Object]); + var resolution = resolver.ResolveDefaultKeyPolicy(now, [key]); // Assert Assert.Null(resolution.DefaultKey); Assert.Null(resolution.FallbackKey); Assert.True(resolution.ShouldGenerateNewKey); - mockKey.Verify(o => o.CreateEncryptor(), Times.Once); // Not retried + Assert.Equal(0, descriptorFactoryCalls); // Not retried } [Fact] @@ -332,28 +340,67 @@ public void CreateEncryptor_FirstAttemptIsNotARetry() var now = ParseDateTimeOffset("2010-01-01 00:00:00Z"); + var keyId1 = Guid.NewGuid(); var creation1 = now.AddDays(-3); var activation1 = creation1.AddDays(2); var expiration1 = creation1.AddDays(90); // Newer but still propagated => preferred + var keyId2 = Guid.NewGuid(); var creation2 = creation1.AddHours(1); var activation2 = activation1.AddHours(1); var expiration2 = expiration1.AddHours(1); - var mockKey1 = CreateMockKey(activation1, expiration1, creation1, isRevoked: false, createEncryptorThrows: false); - var mockKey2 = CreateMockKey(activation2, expiration2, creation2, isRevoked: false, createEncryptorThrows: true); // Uses up all the retries + var mockEncryptor = new Mock(); + var mockDescriptor = new Mock(); + + var mockEncryptorFactory = new Mock(); + mockEncryptorFactory + .Setup(o => o.CreateEncryptorInstance(It.IsAny())) + .Returns(key => + { + _ = key.Descriptor; // A normal implementation would call this + return mockEncryptor.Object; + }); + + var descriptorFactoryCalls1 = 0; + var descriptorFactoryCalls2 = 0; + + // Retries don't work with mock keys + var key1 = new Key( + keyId1, + creation1, + activation1, + expiration1, + encryptorFactories: [mockEncryptorFactory.Object], + descriptorFactory: () => + { + descriptorFactoryCalls1++; + return mockDescriptor.Object; + }); + + var key2 = new Key( + keyId2, + creation2, + activation2, + expiration2, + encryptorFactories: [mockEncryptorFactory.Object], + descriptorFactory: () => + { + descriptorFactoryCalls2++; + throw new InvalidOperationException("Simulated decryption failure"); + }); // Act - var resolution = resolver.ResolveDefaultKeyPolicy(now, [mockKey1.Object, mockKey2.Object]); + var resolution = resolver.ResolveDefaultKeyPolicy(now, [key1, key2]); // Assert Assert.Null(resolution.DefaultKey); - Assert.Same(mockKey1.Object, resolution.FallbackKey); + Assert.Same(key1, resolution.FallbackKey); Assert.True(resolution.ShouldGenerateNewKey); - mockKey1.Verify(o => o.CreateEncryptor(), Times.Once); // 1 try - mockKey2.Verify(o => o.CreateEncryptor(), Times.Exactly(4)); // 1 try plus max (3) retries + Assert.Equal(1, descriptorFactoryCalls1); // 1 try + Assert.Equal(4, descriptorFactoryCalls2); // 1 try plus max (3) retries } [Fact] @@ -374,25 +421,43 @@ public void CreateEncryptor_SucceedsOnRetry() var activation = creation.AddDays(2); var expiration = creation.AddDays(90); - var mockKey = CreateMockKey(activation, expiration, creation); - mockKey - .Setup(o => o.CreateEncryptor()) - .Returns(() => + var mockEncryptor = new Mock(); + var mockDescriptor = new Mock(); + + var mockEncryptorFactory = new Mock(); + mockEncryptorFactory + .Setup(o => o.CreateEncryptorInstance(It.IsAny())) + .Returns(key => { - // Added to list before the callback is called - if (mockKey.Invocations.Count(i => i.Method.Name == nameof(IKey.CreateEncryptor)) == 1) + _ = key.Descriptor; // A normal implementation would call this + return mockEncryptor.Object; + }); + + var descriptorFactoryCalls = 0; + + // Retries don't work with mock keys + var key = new Key( + Guid.NewGuid(), + creation, + activation, + expiration, + encryptorFactories: [mockEncryptorFactory.Object], + descriptorFactory: () => + { + descriptorFactoryCalls++; + if (descriptorFactoryCalls == 1) { - throw new Exception("This method fails."); + throw new InvalidOperationException("Simulated decryption failure"); } - return new Mock().Object; + return mockDescriptor.Object; }); // Act - var resolution = resolver.ResolveDefaultKeyPolicy(now, [mockKey.Object]); + var resolution = resolver.ResolveDefaultKeyPolicy(now, [key]); // Assert - Assert.Same(mockKey.Object, resolution.DefaultKey); - mockKey.Verify(o => o.CreateEncryptor(), Times.Exactly(2)); // 1 try plus 1 retry + Assert.Same(key, resolution.DefaultKey); + Assert.Equal(2, descriptorFactoryCalls); // 1 try plus 1 retry } private static IDefaultKeyResolver CreateDefaultKeyResolver() @@ -406,11 +471,6 @@ private static IKey CreateKey(string activationDate, string expirationDate, stri } private static IKey CreateKey(DateTimeOffset activationDate, DateTimeOffset expirationDate, DateTimeOffset? creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false) - { - return CreateMockKey(activationDate, expirationDate, creationDate, isRevoked, createEncryptorThrows).Object; - } - - private static Mock CreateMockKey(DateTimeOffset activationDate, DateTimeOffset expirationDate, DateTimeOffset? creationDate = null, bool isRevoked = false, bool createEncryptorThrows = false) { var mockKey = new Mock(); mockKey.Setup(o => o.KeyId).Returns(Guid.NewGuid()); @@ -427,7 +487,7 @@ private static Mock CreateMockKey(DateTimeOffset activationDate, DateTimeO mockKey.Setup(o => o.CreateEncryptor()).Returns(Mock.Of()); } - return mockKey; + return mockKey.Object; } private static DateTimeOffset ParseDateTimeOffset(string dto) From c71056cce15e145ba8a5a419f61b41dd0de17a7c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 12 Apr 2024 17:02:37 -0700 Subject: [PATCH 3/9] Fix handling of disallowed retries --- .../src/KeyManagement/DefaultKeyResolver.cs | 25 +++++++++++-------- .../KeyManagement/DefaultKeyResolverTests.cs | 11 +++++--- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index 441c83ae642e..17d027681bd8 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -82,21 +82,24 @@ private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining) } catch (Exception ex) { - if (exceptions is null) + if (retriesRemaining > 0) { - // The first failure doesn't count as a retry - exceptions = new(); + if (exceptions is null) + { + // The first failure doesn't count as a retry + exceptions = new(); + } + else + { + retriesRemaining--; + } + + exceptions.Add(ex); } - else - { - retriesRemaining--; - } - - exceptions.Add(ex); - if (retriesRemaining == 0) + if (retriesRemaining <= 0) // Guard against infinite loops from programmer errors { - _logger.KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed(key.KeyId, nameof(IKey.CreateEncryptor), new AggregateException(exceptions)); + _logger.KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed(key.KeyId, nameof(IKey.CreateEncryptor), exceptions is null ? ex : new AggregateException(exceptions)); return false; } 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 ba5616965d0d..870a3201fc3a 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 @@ -326,13 +326,16 @@ public void CreateEncryptor_NoRetryOnNullReturn() Assert.Equal(0, descriptorFactoryCalls); // Not retried } - [Fact] - public void CreateEncryptor_FirstAttemptIsNotARetry() + [Theory] + [InlineData(0)] // Retries disabled (as by appcontext switch) + [InlineData(1)] + [InlineData(10)] + public void CreateEncryptor_FirstAttemptIsNotARetry(int maxRetries) { // Arrange var options = Options.Create(new KeyManagementOptions() { - MaximumDefaultKeyResolverRetries = 3, + MaximumDefaultKeyResolverRetries = maxRetries, DefaultKeyResolverRetryDelay = TimeSpan.Zero, }); @@ -400,7 +403,7 @@ public void CreateEncryptor_FirstAttemptIsNotARetry() Assert.True(resolution.ShouldGenerateNewKey); Assert.Equal(1, descriptorFactoryCalls1); // 1 try - Assert.Equal(4, descriptorFactoryCalls2); // 1 try plus max (3) retries + Assert.Equal(1 + maxRetries, descriptorFactoryCalls2); // 1 try plus max retries } [Fact] From a33ac3b4477f57b71a951c6c527fd1bf8dbb7ba3 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 19 Apr 2024 10:06:57 -0700 Subject: [PATCH 4/9] Make property name more explicit --- .../DataProtection/src/KeyManagement/DefaultKeyResolver.cs | 2 +- .../DataProtection/src/KeyManagement/KeyManagementOptions.cs | 4 ++-- .../KeyManagement/DefaultKeyResolverTests.cs | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index 17d027681bd8..ed7fd57b918e 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -54,7 +54,7 @@ public DefaultKeyResolver(IOptions keyManagementOptions, I { _keyPropagationWindow = KeyManagementOptions.KeyPropagationWindow; _maxServerToServerClockSkew = KeyManagementOptions.MaxServerClockSkew; - _maxDecryptRetries = keyManagementOptions.Value.MaximumDefaultKeyResolverRetries; + _maxDecryptRetries = keyManagementOptions.Value.MaximumTotalDefaultKeyResolverRetries; _decryptRetryDelay = keyManagementOptions.Value.DefaultKeyResolverRetryDelay; _logger = loggerFactory.CreateLogger(); } diff --git a/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs b/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs index 05fc3c57b177..3c11514033ef 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/KeyManagementOptions.cs @@ -102,11 +102,11 @@ internal static TimeSpan MaxServerClockSkew /// /// Settable for testing. /// - internal int MaximumDefaultKeyResolverRetries { get; set; } = 10; + internal int MaximumTotalDefaultKeyResolverRetries { get; set; } = 10; /// /// Wait this long before each default key resolution decryption retry. - /// + /// /// /// /// Settable for testing. 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 870a3201fc3a..581ff83b9b24 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 @@ -335,7 +335,7 @@ public void CreateEncryptor_FirstAttemptIsNotARetry(int maxRetries) // Arrange var options = Options.Create(new KeyManagementOptions() { - MaximumDefaultKeyResolverRetries = maxRetries, + MaximumTotalDefaultKeyResolverRetries = maxRetries, DefaultKeyResolverRetryDelay = TimeSpan.Zero, }); @@ -412,7 +412,7 @@ public void CreateEncryptor_SucceedsOnRetry() // Arrange var options = Options.Create(new KeyManagementOptions() { - MaximumDefaultKeyResolverRetries = 3, + MaximumTotalDefaultKeyResolverRetries = 3, DefaultKeyResolverRetryDelay = TimeSpan.Zero, }); From 33275edfa45d026d317749f5ea5575e4ffd14fe0 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 19 Apr 2024 10:07:57 -0700 Subject: [PATCH 5/9] Make comment more explicit --- .../DataProtection/src/KeyManagement/DefaultKeyResolver.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index ed7fd57b918e..1fdddd2f9d04 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -109,7 +109,7 @@ private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining) // Reset the descriptor to allow for a retry (key as Key)?.ResetDescriptor(); - // Don't retry immediately + // Don't retry immediately - allow a little time for the transient problem to clear Thread.Sleep(_decryptRetryDelay); } } From 4d09b1a799d2a43fd22d116864290bd6e79514ab Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 19 Apr 2024 10:08:58 -0700 Subject: [PATCH 6/9] Use collection initializer syntax --- .../DataProtection/src/KeyManagement/DefaultKeyResolver.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index 1fdddd2f9d04..ff4bfcbd8314 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -87,7 +87,7 @@ private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining) if (exceptions is null) { // The first failure doesn't count as a retry - exceptions = new(); + exceptions = []; } else { From 8285273756338bfdec6bf5d9172f018cada10b2d Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 19 Apr 2024 10:09:55 -0700 Subject: [PATCH 7/9] Make comment less obtuse --- src/DataProtection/DataProtection/src/KeyManagement/Key.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/Key.cs b/src/DataProtection/DataProtection/src/KeyManagement/Key.cs index e8f5a02f6d3c..cc09b9312af3 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/Key.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/Key.cs @@ -130,7 +130,7 @@ public IAuthenticatedEncryptorDescriptor Descriptor // We could check for _descriptorException here, but there's no reason to optimize that case // (i.e. by avoiding taking the lock) - if (_descriptor is not null) // Can only become less null, so losing a race here doesn't matter + if (_descriptor is not null) // Can only go from null to non-null, so losing a race here doesn't matter { Debug.Assert(_descriptorException is null); // Mutually exclusive with _descriptor return _descriptor; From 22de4bf523d1a8463ff643ab0c812186d0b19ed5 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 19 Apr 2024 11:13:08 -0700 Subject: [PATCH 8/9] Add appcontext switch for changing max retries --- .../src/KeyManagement/DefaultKeyResolver.cs | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs index ff4bfcbd8314..903e41b73e31 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/DefaultKeyResolver.cs @@ -20,6 +20,13 @@ namespace Microsoft.AspNetCore.DataProtection.KeyManagement; /// internal sealed class DefaultKeyResolver : IDefaultKeyResolver { +#if NETFRAMEWORK + // Sadly, numerical AppContext data support was not added until 4.7 and we target 4.6 + private const string MaximumTotalDefaultKeyResolverRetriesSwitchKey = "Microsoft.AspNetCore.DataProtection.KeyManagement.DisableDefaultKeyResolverRetries"; +#else + private const string MaximumTotalDefaultKeyResolverRetriesDataKey = "Microsoft.AspNetCore.DataProtection.KeyManagement.MaximumTotalDefaultKeyResolverRetries"; +#endif + /// /// The window of time before the key expires when a new key should be created /// and persisted to the keyring to ensure uninterrupted service. @@ -54,11 +61,35 @@ public DefaultKeyResolver(IOptions keyManagementOptions, I { _keyPropagationWindow = KeyManagementOptions.KeyPropagationWindow; _maxServerToServerClockSkew = KeyManagementOptions.MaxServerClockSkew; - _maxDecryptRetries = keyManagementOptions.Value.MaximumTotalDefaultKeyResolverRetries; + _maxDecryptRetries = GetMaxDecryptRetriesFromAppContext() ?? keyManagementOptions.Value.MaximumTotalDefaultKeyResolverRetries; _decryptRetryDelay = keyManagementOptions.Value.DefaultKeyResolverRetryDelay; _logger = loggerFactory.CreateLogger(); } + private static int? GetMaxDecryptRetriesFromAppContext() + { +#if NETFRAMEWORK + // Force to zero if retries are disabled. Otherwise, use the configured value + return AppContext.TryGetSwitch(MaximumTotalDefaultKeyResolverRetriesSwitchKey, out var areRetriesDisabled) && areRetriesDisabled ? 0 : null; +#else + var data = AppContext.GetData(MaximumTotalDefaultKeyResolverRetriesDataKey); + + // Programmatically-configured values are usually ints + if (data is int count) + { + return count; + } + + // msbuild-configured values are usually strings + if (data is string countStr && int.TryParse(countStr, out var parsed)) + { + return parsed; + } + + return null; +#endif + } + private bool CanCreateAuthenticatedEncryptor(IKey key, ref int retriesRemaining) { List? exceptions = null; From f0f0217e7bc83211d9c6d273c31b8231861ea7aa Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Fri, 19 Apr 2024 11:57:18 -0700 Subject: [PATCH 9/9] Mention retries in log warning --- src/DataProtection/DataProtection/src/LoggingExtensions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/DataProtection/DataProtection/src/LoggingExtensions.cs b/src/DataProtection/DataProtection/src/LoggingExtensions.cs index f5f6587b4cf4..553a4f5e1f60 100644 --- a/src/DataProtection/DataProtection/src/LoggingExtensions.cs +++ b/src/DataProtection/DataProtection/src/LoggingExtensions.cs @@ -73,7 +73,7 @@ private static bool IsLogLevelEnabledCore([NotNullWhen(true)] ILogger? logger, L [LoggerMessage(11, LogLevel.Debug, "Using managed symmetric algorithm '{FullName}'.", EventName = "UsingManagedSymmetricAlgorithm")] public static partial void UsingManagedSymmetricAlgorithm(this ILogger logger, string fullName); - [LoggerMessage(12, LogLevel.Warning, "Key {KeyId:B} is ineligible to be the default key because its {MethodName} method failed.", EventName = "KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed")] + [LoggerMessage(12, LogLevel.Warning, "Key {KeyId:B} is ineligible to be the default key because its {MethodName} method failed after the maximum number of retries.", EventName = "KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed")] public static partial void KeyIsIneligibleToBeTheDefaultKeyBecauseItsMethodFailed(this ILogger logger, Guid keyId, string methodName, Exception exception); [LoggerMessage(13, LogLevel.Debug, "Considering key {KeyId:B} with expiration date {ExpirationDate:u} as default key.", EventName = "ConsideringKeyWithExpirationDateAsDefaultKey")]