diff --git a/src/DataProtection/DataProtection/src/KeyManagement/DeferredKey.cs b/src/DataProtection/DataProtection/src/KeyManagement/DeferredKey.cs deleted file mode 100644 index 6e69ef589164..000000000000 --- a/src/DataProtection/DataProtection/src/KeyManagement/DeferredKey.cs +++ /dev/null @@ -1,60 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; -using System.Xml.Linq; -using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption; -using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ConfigurationModel; -using Microsoft.AspNetCore.DataProtection.KeyManagement.Internal; -using Microsoft.AspNetCore.DataProtection.XmlEncryption; - -namespace Microsoft.AspNetCore.DataProtection.KeyManagement; - -/// -/// The basic implementation of , where the incoming XML element -/// hasn't yet been fully processed. -/// -internal sealed class DeferredKey : KeyBase -{ - public DeferredKey( - Guid keyId, - DateTimeOffset creationDate, - DateTimeOffset activationDate, - DateTimeOffset expirationDate, - IInternalXmlKeyManager keyManager, - XElement keyElement, - IEnumerable encryptorFactories) - : base(keyId, - creationDate, - activationDate, - expirationDate, - new Lazy(GetLazyDescriptorDelegate(keyManager, keyElement)), - encryptorFactories) - { - } - - private static Func GetLazyDescriptorDelegate(IInternalXmlKeyManager keyManager, XElement keyElement) - { - // The element will be held around in memory for a potentially lengthy period - // of time. Since it might contain sensitive information, we should protect it. - var encryptedKeyElement = keyElement.ToSecret(); - - try - { - return GetLazyDescriptorDelegate; - } - finally - { - // It's important that the lambda above doesn't capture 'descriptorElement'. Clearing the reference here - // helps us detect if we've done this by causing a null ref at runtime. - keyElement = null!; - } - - IAuthenticatedEncryptorDescriptor GetLazyDescriptorDelegate() - { - return keyManager.DeserializeDescriptorFromKeyElement(encryptedKeyElement.ToXElement()); - } - } -} diff --git a/src/DataProtection/DataProtection/src/KeyManagement/Key.cs b/src/DataProtection/DataProtection/src/KeyManagement/Key.cs index 2bdf7e579faa..31870015c7f5 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/Key.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/Key.cs @@ -3,17 +3,28 @@ using System; using System.Collections.Generic; +using System.Xml.Linq; using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption; using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ConfigurationModel; +using Microsoft.AspNetCore.DataProtection.KeyManagement.Internal; +using Microsoft.AspNetCore.DataProtection.XmlEncryption; namespace Microsoft.AspNetCore.DataProtection.KeyManagement; /// -/// The basic implementation of , where the -/// has already been created. +/// The basic implementation of . /// -internal sealed class Key : KeyBase +internal sealed class Key : IKey { + private readonly Lazy _lazyDescriptor; + private readonly IEnumerable _encryptorFactories; + + private IAuthenticatedEncryptor? _encryptor; + + /// + /// The basic implementation of , where the + /// has already been created. + /// public Key( Guid keyId, DateTimeOffset creationDate, @@ -21,7 +32,7 @@ public Key( DateTimeOffset expirationDate, IAuthenticatedEncryptorDescriptor descriptor, IEnumerable encryptorFactories) - : base(keyId, + : this(keyId, creationDate, activationDate, expirationDate, @@ -29,4 +40,119 @@ public Key( encryptorFactories) { } + + /// + /// The basic implementation of , where the incoming XML element + /// hasn't yet been fully processed. + /// + public Key( + Guid keyId, + DateTimeOffset creationDate, + DateTimeOffset activationDate, + DateTimeOffset expirationDate, + IInternalXmlKeyManager keyManager, + XElement keyElement, + IEnumerable encryptorFactories) + : this(keyId, + creationDate, + activationDate, + expirationDate, + new Lazy(GetLazyDescriptorDelegate(keyManager, keyElement)), + encryptorFactories) + { + } + + private Key( + Guid keyId, + DateTimeOffset creationDate, + DateTimeOffset activationDate, + DateTimeOffset expirationDate, + Lazy lazyDescriptor, + IEnumerable encryptorFactories) + { + KeyId = keyId; + CreationDate = creationDate; + ActivationDate = activationDate; + ExpirationDate = expirationDate; + _lazyDescriptor = lazyDescriptor; + _encryptorFactories = encryptorFactories; + } + + public DateTimeOffset ActivationDate { get; } + + public DateTimeOffset CreationDate { get; } + + public DateTimeOffset ExpirationDate { get; } + + public bool IsRevoked { get; private set; } + + public Guid KeyId { get; } + + public IAuthenticatedEncryptorDescriptor Descriptor + { + get + { + return _lazyDescriptor.Value; + } + } + + public IAuthenticatedEncryptor? CreateEncryptor() + { + if (_encryptor == null) + { + foreach (var factory in _encryptorFactories) + { + var encryptor = factory.CreateEncryptorInstance(this); + if (encryptor != null) + { + _encryptor = encryptor; + break; + } + } + } + + return _encryptor; + } + + internal void SetRevoked() + { + IsRevoked = true; + } + + internal Key Clone() + { + return new Key( + keyId: KeyId, + creationDate: CreationDate, + activationDate: ActivationDate, + expirationDate: ExpirationDate, + lazyDescriptor: _lazyDescriptor, + encryptorFactories: _encryptorFactories) + { + IsRevoked = IsRevoked, + }; + } + + private static Func GetLazyDescriptorDelegate(IInternalXmlKeyManager keyManager, XElement keyElement) + { + // The element will be held around in memory for a potentially lengthy period + // of time. Since it might contain sensitive information, we should protect it. + var encryptedKeyElement = keyElement.ToSecret(); + + try + { + return GetLazyDescriptorDelegate; + } + finally + { + // It's important that the lambda above doesn't capture 'descriptorElement'. Clearing the reference here + // helps us detect if we've done this by causing a null ref at runtime. + keyElement = null!; + } + + IAuthenticatedEncryptorDescriptor GetLazyDescriptorDelegate() + { + return keyManager.DeserializeDescriptorFromKeyElement(encryptedKeyElement.ToXElement()); + } + } } diff --git a/src/DataProtection/DataProtection/src/KeyManagement/KeyBase.cs b/src/DataProtection/DataProtection/src/KeyManagement/KeyBase.cs deleted file mode 100644 index 03b7f2a8b6fe..000000000000 --- a/src/DataProtection/DataProtection/src/KeyManagement/KeyBase.cs +++ /dev/null @@ -1,77 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Collections.Generic; -using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption; -using Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ConfigurationModel; - -namespace Microsoft.AspNetCore.DataProtection.KeyManagement; - -/// -/// The basic implementation of . -/// -internal abstract class KeyBase : IKey -{ - private readonly Lazy _lazyDescriptor; - private readonly IEnumerable _encryptorFactories; - - private IAuthenticatedEncryptor? _encryptor; - - public KeyBase( - Guid keyId, - DateTimeOffset creationDate, - DateTimeOffset activationDate, - DateTimeOffset expirationDate, - Lazy lazyDescriptor, - IEnumerable encryptorFactories) - { - KeyId = keyId; - CreationDate = creationDate; - ActivationDate = activationDate; - ExpirationDate = expirationDate; - _lazyDescriptor = lazyDescriptor; - _encryptorFactories = encryptorFactories; - } - - public DateTimeOffset ActivationDate { get; } - - public DateTimeOffset CreationDate { get; } - - public DateTimeOffset ExpirationDate { get; } - - public bool IsRevoked { get; private set; } - - public Guid KeyId { get; } - - public IAuthenticatedEncryptorDescriptor Descriptor - { - get - { - return _lazyDescriptor.Value; - } - } - - public IAuthenticatedEncryptor? CreateEncryptor() - { - if (_encryptor == null) - { - foreach (var factory in _encryptorFactories) - { - var encryptor = factory.CreateEncryptorInstance(this); - if (encryptor != null) - { - _encryptor = encryptor; - break; - } - } - } - - return _encryptor; - } - - internal void SetRevoked() - { - IsRevoked = true; - } -} diff --git a/src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs b/src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs index 31b1fc66b4ad..fc9f609cd8b4 100644 --- a/src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs +++ b/src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -56,6 +57,7 @@ public sealed class XmlKeyManager : IKeyManager, IInternalXmlKeyManager private readonly ILogger _logger; private readonly IEnumerable _encryptorFactories; private readonly IDefaultKeyStorageDirectories _keyStorageDirectories; + private readonly ConcurrentDictionary _knownKeyMap = new(); // Grows unboundedly, like the key ring private CancellationTokenSource? _cacheExpirationTokenSource; @@ -157,7 +159,7 @@ public IReadOnlyCollection GetAllKeys() var allElements = KeyRepository.GetAllElements(); // We aggregate all the information we read into three buckets - Dictionary keyIdToKeyMap = new Dictionary(); + Dictionary keyIdToKeyMap = new Dictionary(); HashSet? revokedKeyIds = null; DateTimeOffset? mostRecentMassRevocationDate = null; @@ -254,7 +256,7 @@ public CancellationToken GetCacheExpirationToken() return Interlocked.CompareExchange(ref _cacheExpirationTokenSource, null, null).Token; } - private KeyBase? ProcessKeyElement(XElement keyElement) + private Key? ProcessKeyElement(XElement keyElement) { Debug.Assert(keyElement.Name == KeyElementName); @@ -262,13 +264,20 @@ public CancellationToken GetCacheExpirationToken() { // Read metadata and prepare the key for deferred instantiation Guid keyId = (Guid)keyElement.Attribute(IdAttributeName)!; + + _logger.FoundKey(keyId); + + if (_knownKeyMap.TryGetValue(keyId, out var oldKey)) + { + // Keys are immutable (other than revocation), so there's no need to read it again + return oldKey.Clone(); + } + DateTimeOffset creationDate = (DateTimeOffset)keyElement.Element(CreationDateElementName)!; DateTimeOffset activationDate = (DateTimeOffset)keyElement.Element(ActivationDateElementName)!; DateTimeOffset expirationDate = (DateTimeOffset)keyElement.Element(ExpirationDateElementName)!; - _logger.FoundKey(keyId); - - return new DeferredKey( + var key = new Key( keyId: keyId, creationDate: creationDate, activationDate: activationDate, @@ -276,6 +285,10 @@ public CancellationToken GetCacheExpirationToken() keyManager: this, keyElement: keyElement, encryptorFactories: _encryptorFactories); + + RecordKey(key); + + return key; } catch (Exception ex) { @@ -286,6 +299,18 @@ public CancellationToken GetCacheExpirationToken() } } + private void RecordKey(Key key) + { + if (!_knownKeyMap.TryAdd(key.KeyId, key)) + { + // If we lost the race, the winner inserted an equivalent key + Debug.Assert(_knownKeyMap.TryGetValue(key.KeyId, out var existingKey)); + Debug.Assert(existingKey.CreationDate == key.CreationDate); + Debug.Assert(existingKey.ActivationDate == key.ActivationDate); + Debug.Assert(existingKey.ExpirationDate == key.ExpirationDate); + } + } + // returns a Guid (for specific keys) or a DateTimeOffset (for all keys created on or before a specific date) private object ProcessRevocationElement(XElement revocationElement) { @@ -431,13 +456,17 @@ IKey IInternalXmlKeyManager.CreateNewKey(Guid keyId, DateTimeOffset creationDate TriggerAndResetCacheExpirationToken(); // And we're done! - return new Key( + var key = new Key( keyId: keyId, creationDate: creationDate, activationDate: activationDate, expirationDate: expirationDate, descriptor: newDescriptor, encryptorFactories: _encryptorFactories); + + RecordKey(key); + + return key; } IAuthenticatedEncryptorDescriptor IInternalXmlKeyManager.DeserializeDescriptorFromKeyElement(XElement keyElement) diff --git a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DeferredKeyTests.cs b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DeferredKeyTests.cs index 7914d2b08563..339a2b3ed1a8 100644 --- a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DeferredKeyTests.cs +++ b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DeferredKeyTests.cs @@ -31,7 +31,7 @@ public void Ctor_Properties() var encryptorFactory = Mock.Of(); // Act - var key = new DeferredKey(keyId, creationDate, activationDate, expirationDate, mockInternalKeyManager.Object, XElement.Parse(@""), new[] { encryptorFactory }); + var key = new Key(keyId, creationDate, activationDate, expirationDate, mockInternalKeyManager.Object, XElement.Parse(@""), new[] { encryptorFactory }); // Assert Assert.Equal(keyId, key.KeyId); @@ -47,7 +47,7 @@ public void SetRevoked_Respected() // Arrange var now = DateTimeOffset.UtcNow; var encryptorFactory = Mock.Of(); - var key = new DeferredKey(Guid.Empty, now, now, now, new Mock().Object, XElement.Parse(@""), new[] { encryptorFactory }); + var key = new Key(Guid.Empty, now, now, now, new Mock().Object, XElement.Parse(@""), new[] { encryptorFactory }); // Act & assert Assert.False(key.IsRevoked); @@ -70,7 +70,7 @@ public void Get_Descriptor_CachesFailures() var now = DateTimeOffset.UtcNow; var encryptorFactory = Mock.Of(); - var key = new DeferredKey(Guid.Empty, now, now, now, mockKeyManager.Object, XElement.Parse(@""), new[] { encryptorFactory }); + var key = new Key(Guid.Empty, now, now, now, mockKeyManager.Object, XElement.Parse(@""), new[] { encryptorFactory }); // Act & assert ExceptionAssert.Throws(() => key.Descriptor, "How exceptional."); diff --git a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/XmlKeyManagerTests.cs b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/XmlKeyManagerTests.cs index 4858c1d602a6..e15a94d9ff14 100644 --- a/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/XmlKeyManagerTests.cs +++ b/src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/XmlKeyManagerTests.cs @@ -744,6 +744,190 @@ public void RevokeKey_CallsInternalManager() Assert.InRange(actualRevocationDate.Value, minRevocationDate, DateTimeOffset.UtcNow); } + [Fact] + public void CreatedKeyReused() + { + // Arrange + + // The only descriptor we'll use + var descriptor = new Mock(MockBehavior.Strict); + descriptor + .Setup(o => o.ExportToXml()) + // Shouldn't be an interface, but we control the activator and will return a mock + .Returns(new XmlSerializedDescriptorInfo(serializedDescriptor, typeof(IAuthenticatedEncryptorDescriptorDeserializer))); + + // The factory always returns the only descriptor + var authenticatedEncryptorConfiguration = new Mock(MockBehavior.Strict); + authenticatedEncryptorConfiguration + .Setup(o => o.CreateNewDescriptor()) + .Returns(descriptor.Object); + + // Any xml element deserializes to the only descriptor + var descriptorDeserializer = new Mock(MockBehavior.Strict); + descriptorDeserializer + .Setup(o => o.ImportFromXml(It.IsAny())) + .Returns(descriptor.Object); + + // Keep track of how many times a key needs to be decrypted + var decryptionCount = 0; + var decryptor = new Mock(MockBehavior.Strict); + decryptor + .Setup(o => o.Decrypt(It.IsAny())) + .Returns(element => + { + decryptionCount++; + return element; + }); + + // We need a simple encryptor for the newly created key + var encryptor = new Mock(MockBehavior.Strict); + encryptor + .Setup(o => o.Encrypt(It.IsAny())) + // Shouldn't be an interface, but we control the activator and will return a mock + .Returns(element => new EncryptedXmlInfo(element, typeof(IXmlDecryptor))); + + // We control deserialization by hooking activation + var activator = new Mock(MockBehavior.Strict); + activator + .Setup(o => o.CreateInstance(typeof(IXmlDecryptor), It.IsAny())) + .Returns(decryptor.Object); + activator + .Setup(o => o.CreateInstance(typeof(IAuthenticatedEncryptorDescriptorDeserializer), It.IsAny())) + .Returns(descriptorDeserializer.Object); + + var keyManagementOptions = Options.Create(new KeyManagementOptions() + { + AuthenticatedEncryptorConfiguration = authenticatedEncryptorConfiguration.Object, + XmlRepository = new EphemeralXmlRepository(NullLoggerFactory.Instance), // A realistic repository is fine + XmlEncryptor = encryptor.Object, + }); + + var keyManager = new XmlKeyManager(keyManagementOptions, activator.Object); + + var creationDate = DateTimeOffset.UtcNow; + var activationDate = creationDate.AddDays(2); + var expirationDate = creationDate.AddDays(90); + + var createdKey = keyManager.CreateNewKey(activationDate, expirationDate); + + // Force decryption, if encrypted. The real call would be CreateEncryptor(), but that would access Descriptor under the hood + _ = createdKey.Descriptor; + Assert.Equal(0, decryptionCount); + + // Act + var fetchedKeys = keyManager.GetAllKeys(); + + // Assert + var fetchedKey = Assert.Single(fetchedKeys); + Assert.NotSame(createdKey, fetchedKey); // It's mutable, so we don't want to reuse the same instance + Assert.Equal(createdKey.KeyId, fetchedKey.KeyId); // But it should be equivalent + + // Nothing up to this point should have required decryption + Assert.Equal(0, decryptionCount); + + // Force decryption, if encrypted. The real call would be CreateEncryptor(), but that would access Descriptor under the hood + _ = fetchedKey.Descriptor; + Assert.Equal(0, decryptionCount); + + var fetchedKeys2 = keyManager.GetAllKeys(); + + var fetchedKey2 = Assert.Single(fetchedKeys2); + Assert.NotSame(createdKey, fetchedKey2); // It's mutable, so we don't want to reuse the same instance + Assert.NotSame(fetchedKey, fetchedKey2); + Assert.Equal(createdKey.KeyId, fetchedKey2.KeyId); // But it should be equivalent + + // Force decryption, if encrypted. The real call would be CreateEncryptor(), but that would access Descriptor under the hood + _ = fetchedKey2.Descriptor; + Assert.Equal(0, decryptionCount); + } + + [Fact] + public void NovelFetchedKeyRequiresDecryption() + { + // Arrange + var descriptorDeserializer = new Mock(MockBehavior.Strict); + descriptorDeserializer + .Setup(o => o.ImportFromXml(It.IsAny())) + .Returns(new Mock(MockBehavior.Strict).Object); + + // Keep track of how many times a key needs to be decrypted + var decryptionCount = 0; + var decryptor = new Mock(MockBehavior.Strict); + decryptor + .Setup(o => o.Decrypt(It.IsAny())) + .Returns(element => + { + decryptionCount++; + return element; + }); + + var encryptor = new Mock(MockBehavior.Strict); + + // We control deserialization by hooking activation + var activator = new Mock(MockBehavior.Strict); + activator + .Setup(o => o.CreateInstance(typeof(IXmlDecryptor), It.IsAny())) + .Returns(decryptor.Object); + activator + .Setup(o => o.CreateInstance(typeof(IAuthenticatedEncryptorDescriptorDeserializer), It.IsAny())) + .Returns(descriptorDeserializer.Object); + + var keyElement = XElement.Parse(@" + + 2015-01-01T00:00:00Z + 2015-01-02T00:00:00Z + 2015-03-01T00:00:00Z + + + + + + + KeyAsBase64 + + + + + +"); + + var respository = new Mock(); + respository + .Setup(o => o.GetAllElements()) + .Returns([keyElement]); + + var keyManagementOptions = Options.Create(new KeyManagementOptions() + { + XmlRepository = respository.Object, + XmlEncryptor = encryptor.Object, + }); + + var keyManager = new XmlKeyManager(keyManagementOptions, activator.Object); + + // Act + var fetchedKeys = keyManager.GetAllKeys(); + + // Assert + var fetchedKey = Assert.Single(fetchedKeys); + + // Decryption is lazy + Assert.Equal(0, decryptionCount); + + // Force decryption. The real call would be CreateEncryptor(), but that would access Descriptor under the hood + _ = fetchedKey.Descriptor; + Assert.Equal(1, decryptionCount); + + var fetchedKeys2 = keyManager.GetAllKeys(); + + var fetchedKey2 = Assert.Single(fetchedKeys2); + Assert.NotSame(fetchedKey, fetchedKey2); // It's mutable, so we don't want to reuse the same instance + Assert.Equal(fetchedKey.KeyId, fetchedKey2.KeyId); // But it should be equivalent + + // Force decryption, if encrypted. The real call would be CreateEncryptor(), but that would access Descriptor under the hood + _ = fetchedKey2.Descriptor; + Assert.Equal(1, decryptionCount); // Still 1 (i.e. no change) + } + private class MyDeserializer : IAuthenticatedEncryptorDescriptorDeserializer { public IAuthenticatedEncryptorDescriptor ImportFromXml(XElement element)