Support key deletion in Data Protection (dotnet#53860)

* Add deletion APIs to IKeyManager and IXmlRepository

Author: amcasey

src/DataProtection/DataProtection/src/Error.cs

@@ -102,4 +102,9 @@ public static InvalidOperationException KeyRingProvider_RefreshFailedOnOtherThre
     {
         return new InvalidOperationException(Resources.KeyRingProvider_RefreshFailedOnOtherThread, inner);
     }
+
+    public static NotSupportedException XmlKeyManager_DoesNotSupportKeyDeletion()
+    {
+        return new NotSupportedException(Resources.XmlKeyManager_DoesNotSupportKeyDeletion);
+    }
 }

src/DataProtection/DataProtection/src/KeyManagement/IDeletableKeyManager.cs

@@ -0,0 +1,48 @@
+// 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.Threading;
+
+namespace Microsoft.AspNetCore.DataProtection.KeyManagement;
+
+/// <summary>
+/// An extension of <see cref="IKeyManager"/> that supports key deletion.
+/// </summary>
+public interface IDeletableKeyManager : IKeyManager
+{
+    /// <summary>
+    /// Indicates whether this key manager and the underlying <see cref="Repositories.IXmlRepository"/> support key deletion.
+    /// </summary>
+    /// <seealso cref="DeleteKeys"/>
+    bool CanDeleteKeys { get; }
+
+    /// <summary>
+    /// Deletes keys matching a predicate.
+    ///
+    /// Use with caution as deleting active keys will normally cause data loss.
+    /// </summary>
+    /// <param name="shouldDelete">
+    /// A predicate applied to each key.
+    /// Returning true will cause the key to be deleted.
+    /// </param>
+    /// <returns>
+    /// True if all attempted deletions succeeded.
+    /// </returns>
+    /// <remarks>
+    /// Deletion is stronger than revocation.  A revoked key is retained and can even be (forcefully) applied.
+    /// A deleted key is indistinguishable from a key that never existed.
+    /// 
+    /// Generally, keys should only be deleted to save space.  If space is not a concern, keys
+    /// should be revoked or allowed to expire instead.
+    ///
+    /// This method will not mutate existing IKey instances. After calling this method,
+    /// all existing IKey instances should be discarded, and GetAllKeys should be called again.
+    /// </remarks>
+    /// <exception cref="NotSupportedException">
+    /// If <see cref="CanDeleteKeys"/> is false.
+    /// </exception>
+    bool DeleteKeys(Func<IKey, bool> shouldDelete);
+}

src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs

@@ -164,19 +164,38 @@ private static string DateTimeOffsetToFilenameSafeString(DateTimeOffset dateTime
     public IReadOnlyCollection<IKey> GetAllKeys()
     {
         var allElements = KeyRepository.GetAllElements();
+        var processed = ProcessAllElements(allElements, out _);
+        return processed.OfType<IKey>().ToList().AsReadOnly();
+    }
+
+    /// <summary>
+    /// Returns an array paralleling <paramref name="allElements"/> but:
+    ///  1. Key elements become IKeys (with revocation data)
+    ///  2. KeyId-based revocations become Guids
+    ///  3. Date-based revocations become DateTimeOffsets
+    ///  4. Unknown elements become null
+    /// </summary>
+    private object?[] ProcessAllElements(IReadOnlyCollection<XElement> allElements, out DateTimeOffset? mostRecentMassRevocationDate)
+    {
+        var elementCount = allElements.Count;
+
+        var results = new object?[elementCount];
 
-        // We aggregate all the information we read into three buckets
-        Dictionary<Guid, Key> keyIdToKeyMap = new Dictionary<Guid, Key>();
+        Dictionary<Guid, Key> keyIdToKeyMap = [];
         HashSet<Guid>? revokedKeyIds = null;
-        DateTimeOffset? mostRecentMassRevocationDate = null;
 
+        mostRecentMassRevocationDate = null;
+
+        var pos = 0;
         foreach (var element in allElements)
         {
+            object? result;
             if (element.Name == KeyElementName)
             {
                 // ProcessKeyElement can return null in the case of failure, and if this happens we'll move on.
                 // Still need to throw if we see duplicate keys with the same id.
                 var key = ProcessKeyElement(element);
+                result = key;
                 if (key != null)
                 {
                     if (keyIdToKeyMap.ContainsKey(key.KeyId))
@@ -189,19 +208,20 @@ public IReadOnlyCollection<IKey> GetAllKeys()
             else if (element.Name == RevocationElementName)
             {
                 var revocationInfo = ProcessRevocationElement(element);
-                if (revocationInfo is Guid)
+                result = revocationInfo;
+                if (revocationInfo is Guid revocationGuid)
                 {
                     // a single key was revoked
-                    if (revokedKeyIds == null)
+                    revokedKeyIds ??= [];
+                    if (!revokedKeyIds.Add(revocationGuid))
                     {
-                        revokedKeyIds = new HashSet<Guid>();
+                        _logger.KeyRevokedMultipleTimes(revocationGuid);
                     }
-                    revokedKeyIds.Add((Guid)revocationInfo);
                 }
                 else
                 {
                     // all keys as of a certain date were revoked
-                    DateTimeOffset thisMassRevocationDate = (DateTimeOffset)revocationInfo;
+                    var thisMassRevocationDate = (DateTimeOffset)revocationInfo;
                     if (!mostRecentMassRevocationDate.HasValue || mostRecentMassRevocationDate < thisMassRevocationDate)
                     {
                         mostRecentMassRevocationDate = thisMassRevocationDate;
@@ -212,16 +232,18 @@ public IReadOnlyCollection<IKey> GetAllKeys()
             {
                 // Skip unknown elements.
                 _logger.UnknownElementWithNameFoundInKeyringSkipping(element.Name);
+                result = null;
             }
+
+            results[pos++] = result;
         }
 
         // Apply individual revocations
-        if (revokedKeyIds != null)
+        if (revokedKeyIds is not null)
         {
-            foreach (Guid revokedKeyId in revokedKeyIds)
+            foreach (var revokedKeyId in revokedKeyIds)
             {
-                keyIdToKeyMap.TryGetValue(revokedKeyId, out var key);
-                if (key != null)
+                if (keyIdToKeyMap.TryGetValue(revokedKeyId, out var key))
                 {
                     key.SetRevoked();
                     _logger.MarkedKeyAsRevokedInTheKeyring(revokedKeyId);
@@ -252,7 +274,7 @@ public IReadOnlyCollection<IKey> GetAllKeys()
         }
 
         // And we're finished!
-        return keyIdToKeyMap.Values.ToList().AsReadOnly();
+        return results;
     }
 
     /// <inheritdoc/>
@@ -385,6 +407,76 @@ public void RevokeKey(Guid keyId, string? reason = null)
             reason: reason);
     }
 
+    /// <inheritdoc/>
+    public bool CanDeleteKeys => KeyRepository is IDeletableXmlRepository;
+
+    /// <inheritdoc/>
+    public bool DeleteKeys(Func<IKey, bool> shouldDelete)
+    {
+        if (KeyRepository is not IDeletableXmlRepository xmlRepositoryWithDeletion)
+        {
+            throw Error.XmlKeyManager_DoesNotSupportKeyDeletion();
+        }
+
+        return xmlRepositoryWithDeletion.DeleteElements((deletableElements) =>
+        {
+            // It is important to delete key elements before the corresponding revocation elements,
+            // in case the deletion fails part way - we don't want to accidentally unrevoke a key
+            // and then not delete it.
+            // Start at a non-zero value just to make it a little clearer in the debugger that it
+            // was set explicitly.
+            const int deletionOrderKey = 1;
+            const int deletionOrderRevocation = 2;
+            const int deletionOrderMassRevocation = 3;
+
+            var deletableElementsArray = deletableElements.ToArray();
+
+            var allElements = deletableElements.Select(d => d.Element).ToArray();
+            var processed = ProcessAllElements(allElements, out var mostRecentMassRevocationDate);
+
+            var allKeyIds = new HashSet<Guid>();
+            var deletedKeyIds = new HashSet<Guid>();
+
+            for (var i = 0; i < deletableElementsArray.Length; i++)
+            {
+                var obj = processed[i];
+                if (obj is IKey key)
+                {
+                    var keyId = key.KeyId;
+                    allKeyIds.Add(keyId);
+
+                    if (shouldDelete(key))
+                    {
+                        _logger.DeletingKey(keyId);
+                        deletedKeyIds.Add(keyId);
+                        deletableElementsArray[i].DeletionOrder = deletionOrderKey;
+                    }
+                }
+                else if (obj is DateTimeOffset massRevocationDate)
+                {
+                    if (massRevocationDate < mostRecentMassRevocationDate)
+                    {
+                        // Delete superceded mass revocation elements
+                        deletableElementsArray[i].DeletionOrder = deletionOrderMassRevocation;
+                    }
+                }
+            }
+
+            // Separate loop since deletedKeyIds and allKeyIds need to have been populated.
+            for (var i = 0; i < deletableElementsArray.Length; i++)
+            {
+                if (processed[i] is Guid revocationId)
+                {
+                    // Delete individual revocations of keys that don't (still) exist
+                    if (deletedKeyIds.Contains(revocationId) || !allKeyIds.Contains(revocationId))
+                    {
+                        deletableElementsArray[i].DeletionOrder = deletionOrderRevocation;
+                    }
+                }
+            }
+        });
+    }
+
     private void TriggerAndResetCacheExpirationToken([CallerMemberName] string? opName = null, bool suppressLogging = false)
     {
         if (!suppressLogging)

src/DataProtection/DataProtection/src/LoggingExtensions.cs

@@ -255,4 +255,25 @@ private static bool IsLogLevelEnabledCore([NotNullWhen(true)] ILogger? logger, L
 
     [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);
+	
+    [LoggerMessage(74, LogLevel.Debug, "Deleting file '{FileName}'.", EventName = "DeletingFile")]
+    public static partial void DeletingFile(this ILogger logger, string fileName);
+
+    [LoggerMessage(75, LogLevel.Error, "Failed to delete file '{FileName}'.  Not attempting further deletions.", EventName = "FailedToDeleteFile")]
+    public static partial void FailedToDeleteFile(this ILogger logger, string fileName, Exception exception);
+
+    [LoggerMessage(76, LogLevel.Debug, "Deleting registry key '{RegistryKeyName}', value '{Value}'.", EventName = "RemovingDataFromRegistryKeyValue")]
+    public static partial void RemovingDataFromRegistryKeyValue(this ILogger logger, RegistryKey registryKeyName, string value);
+
+    [LoggerMessage(77, LogLevel.Error, "Failed to delete registry key '{RegistryKeyName}', value '{ValueName}'.  Not attempting further deletions.", EventName = "FailedToRemoveDataFromRegistryKeyValue")]
+    public static partial void FailedToRemoveDataFromRegistryKeyValue(this ILogger logger, RegistryKey registryKeyName, string valueName, Exception exception);
+
+    [LoggerMessage(78, LogLevel.Trace, "Found multiple revocation entries for key {KeyId:B}.", EventName = "KeyRevokedMultipleTimes")]
+    public static partial void KeyRevokedMultipleTimes(this ILogger logger, Guid keyId);
+
+    [LoggerMessage(79, LogLevel.Trace, "Ignoring revocation of keys created before {OlderDate:u} in favor of revocation of keys created before {NewerDate:u}.", EventName = "DateBasedRevocationSuperseded")]
+    public static partial void DateBasedRevocationSuperseded(this ILogger logger, DateTimeOffset olderDate, DateTimeOffset newerDate);
+
+    [LoggerMessage(80, LogLevel.Debug, "Deleting key {KeyId:B}.", EventName = "DeletingKey")]
+    public static partial void DeletingKey(this ILogger logger, Guid keyId);
 }

src/DataProtection/DataProtection/src/PublicAPI.Unshipped.txt

@@ -1 +1,14 @@
 #nullable enable
+Microsoft.AspNetCore.DataProtection.KeyManagement.IDeletableKeyManager
+Microsoft.AspNetCore.DataProtection.KeyManagement.IDeletableKeyManager.CanDeleteKeys.get -> bool
+Microsoft.AspNetCore.DataProtection.KeyManagement.IDeletableKeyManager.DeleteKeys(System.Func<Microsoft.AspNetCore.DataProtection.KeyManagement.IKey!, bool>! shouldDelete) -> bool
+Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.CanDeleteKeys.get -> bool
+Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.DeleteKeys(System.Func<Microsoft.AspNetCore.DataProtection.KeyManagement.IKey!, bool>! shouldDelete) -> bool
+Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement
+Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement.DeletionOrder.get -> int?
+Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement.DeletionOrder.set -> void
+Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement.Element.get -> System.Xml.Linq.XElement!
+Microsoft.AspNetCore.DataProtection.Repositories.IDeletableXmlRepository
+Microsoft.AspNetCore.DataProtection.Repositories.IDeletableXmlRepository.DeleteElements(System.Action<System.Collections.Generic.IReadOnlyCollection<Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement!>!>! chooseElements) -> bool
+virtual Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository.DeleteElements(System.Action<System.Collections.Generic.IReadOnlyCollection<Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement!>!>! chooseElements) -> bool
+virtual Microsoft.AspNetCore.DataProtection.Repositories.RegistryXmlRepository.DeleteElements(System.Action<System.Collections.Generic.IReadOnlyCollection<Microsoft.AspNetCore.DataProtection.Repositories.IDeletableElement!>!>! chooseElements) -> bool

src/DataProtection/DataProtection/src/Repositories/EphemeralXmlRepository.cs

@@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.DataProtection.Repositories;
 /// An ephemeral XML repository backed by process memory. This class must not be used for
 /// anything other than dev scenarios as the keys will not be persisted to storage.
 /// </summary>
-internal sealed class EphemeralXmlRepository : IXmlRepository
+internal sealed class EphemeralXmlRepository : IDeletableXmlRepository
 {
     private readonly List<XElement> _storedElements = new List<XElement>();
 
@@ -54,4 +54,62 @@ public void StoreElement(XElement element, string friendlyName)
             _storedElements.Add(cloned);
         }
     }
+
+    /// <inheritdoc/>
+    public bool DeleteElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
+    {
+        ArgumentNullThrowHelper.ThrowIfNull(chooseElements);
+
+        var deletableElements = new List<DeletableElement>();
+
+        lock (_storedElements)
+        {
+            foreach (var storedElement in _storedElements)
+            {
+                // Make a deep copy so caller doesn't inadvertently modify it.
+                deletableElements.Add(new DeletableElement(storedElement, new XElement(storedElement)));
+            }
+        }
+
+        chooseElements(deletableElements);
+
+        var elementsToDelete = deletableElements
+            .Where(e => e.DeletionOrder.HasValue)
+            .OrderBy(e => e.DeletionOrder.GetValueOrDefault());
+
+        lock (_storedElements)
+        {
+            foreach (var deletableElement in elementsToDelete)
+            {
+                var storedElement = deletableElement.StoredElement;
+                var index = _storedElements.FindIndex(e => ReferenceEquals(e, storedElement));
+                if (index >= 0) // Might not find it if the collection has changed since we started.
+                {
+                    // It would be more efficient to remove the larger indices first, but deletion order
+                    // is important for correctness.
+                    _storedElements.RemoveAt(index);
+                }
+            }
+        }
+
+        return true;
+    }
+
+    private sealed class DeletableElement : IDeletableElement
+    {
+        public DeletableElement(XElement storedElement, XElement element)
+        {
+            StoredElement = storedElement;
+            Element = element;
+        }
+
+        /// <inheritdoc/>
+        public XElement Element { get; }
+
+        /// <summary>The <see cref="XElement"/> from which <see cref="Element"/> was cloned.</summary>
+        public XElement StoredElement { get; }
+
+        /// <inheritdoc/>
+        public int? DeletionOrder { get; set; }
+    }
 }