From 01d9f5e22538d21024df1be26b6975e594ff285e Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Thu, 29 Feb 2024 13:28:19 -0800 Subject: [PATCH] Throw if the default key is revoked Under somewhat contrived circumstances, it's possible that the keyring, on determining that no suitable default key is available, will generate a new, immediately-activated key and that that key will also be immediately-revoked. For example, it's possible to revoke all keys created before a given date and clock skew between servers could result in that being in the future for some servers. It's also possible that a third-party `IDefaultKeyResolver` could select a revoked key, since the contract doesn't state that it should not. Having said that, we haven't really hardened against other misbehavior by resolvers, so this isn't terribly compelling. Still, no harm in throwing - better than using a revoked key to encrypt data. --- .../DataProtection/src/Error.cs | 6 +++ .../src/KeyManagement/KeyRingProvider.cs | 7 ++++ .../DataProtection/src/LoggingExtensions.cs | 3 ++ .../DataProtection/src/Resources.resx | 3 ++ .../KeyManagement/KeyRingProviderTests.cs | 41 +++++++++++++++++++ 5 files changed, 60 insertions(+) diff --git a/src/DataProtection/DataProtection/src/Error.cs b/src/DataProtection/DataProtection/src/Error.cs index 839e8f1a2e71..6de1d7aa3bd6 100644 --- a/src/DataProtection/DataProtection/src/Error.cs +++ b/src/DataProtection/DataProtection/src/Error.cs @@ -91,4 +91,10 @@ public static InvalidOperationException XmlKeyManager_DuplicateKey(Guid keyId) var message = string.Format(CultureInfo.CurrentCulture, Resources.XmlKeyManager_DuplicateKey, keyId); return new InvalidOperationException(message); } + + public static InvalidOperationException KeyRingProvider_DefaultKeyRevoked(Guid id) + { + var message = string.Format(CultureInfo.CurrentCulture, Resources.KeyRingProvider_DefaultKeyRevoked, id); + return new InvalidOperationException(message); + } } diff --git a/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs b/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs index 0cd7c668831a..c3da92f22661 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/KeyRingProvider.cs @@ -127,6 +127,13 @@ private CacheableKeyRing CreateCacheableKeyRingCoreStep2(DateTimeOffset now, Can // Invariant: our caller ensures that CreateEncryptorInstance succeeded at least once Debug.Assert(defaultKey.CreateEncryptor() != null); + // This can happen if there's a date-based revocation that's in the future (e.g. because of clock skew) + if (defaultKey.IsRevoked) + { + _logger.KeyRingDefaultKeyIsRevoked(defaultKey.KeyId); + throw Error.KeyRingProvider_DefaultKeyRevoked(defaultKey.KeyId); + } + _logger.UsingKeyAsDefaultKey(defaultKey.KeyId); var nextAutoRefreshTime = now + GetRefreshPeriodWithJitter(KeyManagementOptions.KeyRingRefreshPeriod); diff --git a/src/DataProtection/DataProtection/src/LoggingExtensions.cs b/src/DataProtection/DataProtection/src/LoggingExtensions.cs index 3522725b837b..cd4e38918b6d 100644 --- a/src/DataProtection/DataProtection/src/LoggingExtensions.cs +++ b/src/DataProtection/DataProtection/src/LoggingExtensions.cs @@ -249,4 +249,7 @@ private static bool IsLogLevelEnabledCore([NotNullWhen(true)] ILogger? logger, L [LoggerMessage(64, LogLevel.Debug, "Not enabling read-only key access because an XML encryptor has been specified", EventName = "NotUsingReadOnlyKeyConfigurationBecauseOfEncryptor")] public static partial void NotUsingReadOnlyKeyConfigurationBecauseOfEncryptor(this ILogger logger); + + [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); } diff --git a/src/DataProtection/DataProtection/src/Resources.resx b/src/DataProtection/DataProtection/src/Resources.resx index c69efbb97bc2..aa1eb24fea14 100644 --- a/src/DataProtection/DataProtection/src/Resources.resx +++ b/src/DataProtection/DataProtection/src/Resources.resx @@ -187,6 +187,9 @@ The key ring does not contain a valid default protection key. The data protection system cannot create a new key because auto-generation of keys is disabled. For more information go to https://aka.ms/aspnet/dataprotectionwarning + + The key ring's default data protection key {0:B} has been revoked. + {0} must not be negative. diff --git a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs index 55afbb4cefff..169e7633e5db 100644 --- a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs +++ b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs @@ -140,6 +140,47 @@ public void CreateCacheableKeyRing_GenerationRequired_NoDefaultKey_CreatesNewKey Assert.Equal(new[] { "GetCacheExpirationToken", "GetAllKeys", "ResolveDefaultKeyPolicy", "CreateNewKey", "GetCacheExpirationToken", "GetAllKeys", "ResolveDefaultKeyPolicy" }, callSequence); } + [Fact] + public void CreateCacheableKeyRing_GenerationRequired_NoDefaultKey_CreatesNewKeyWithImmediateActivation_NewKeyIsRevoked() + { + // Arrange + var callSequence = new List(); + + var now = (DateTimeOffset)StringToDateTime("2015-03-01 00:00:00Z"); + var allKeys1 = Array.Empty(); + + // This could happen if there were a date-based revocation newer than 2015-03-01 + var newKey = CreateKey("2015-03-01 00:00:00Z", "2016-03-01 00:00:00Z", isRevoked: true); + var allKeys2 = new[] { newKey }; + + var keyRingProvider = SetupCreateCacheableKeyRingTestAndCreateKeyManager( + callSequence: callSequence, + getCacheExpirationTokenReturnValues: new[] { CancellationToken.None, CancellationToken.None }, + getAllKeysReturnValues: new[] { allKeys1, allKeys2 }, + createNewKeyCallbacks: new[] { + Tuple.Create(now, now + TimeSpan.FromDays(90), newKey) + }, + resolveDefaultKeyPolicyReturnValues: new[] + { + Tuple.Create(now, (IEnumerable)allKeys1, new DefaultKeyResolution() + { + DefaultKey = null, // Since there are no keys + ShouldGenerateNewKey = true + }), + Tuple.Create(now, (IEnumerable)allKeys2, new DefaultKeyResolution() + { + DefaultKey = null, // Since all keys are revoked + ShouldGenerateNewKey = true + }) + }); + + // Act/Assert + Assert.Throws(() => keyRingProvider.GetCacheableKeyRing(now)); // The would-be default key is revoked + + // Still make the usual calls - just throw before creating a keyring + Assert.Equal(new[] { "GetCacheExpirationToken", "GetAllKeys", "ResolveDefaultKeyPolicy", "CreateNewKey", "GetCacheExpirationToken", "GetAllKeys", "ResolveDefaultKeyPolicy" }, callSequence); + } + [Fact] public void CreateCacheableKeyRing_GenerationRequired_NoDefaultKey_CreatesNewKeyWithImmediateActivation_StillNoDefaultKey_ReturnsNewlyCreatedKey() {