Allow deletion of Data Protection keys

[amcasey]

See revised proposal below.

Background and Motivation

[This supersedes #53502. which turned out to be impractical to implement]

PR: #53860

Data Protection has no facility for deleting keys. The design assumes that they will simply accumulate forever so that data protected with old keys will always be decryptable in an emergency. Per our docs:

Deleting a key is truly destructive behavior, and consequently the data protection system exposes no first-class API for performing this operation.

However, for a very long-running application, there's a risk of long-defunct keys consuming too many resources. As part of #52916, we should consider allowing deletion.

As a bonus, we can use the Delete call as an excuse to GC redundant mass-revocations, etc.

Proposed API

namespace Microsoft.AspNetCore.DataProtection.KeyManagement;

public interface IKeyManager
{
+    /// <summary>
+    /// Indicates whether this key manager supports key deletion.
+    /// </summary>
+    /// <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.
+    /// </remarks>
+    bool CanDeleteKeys => false;

+    /// <summary>
+    /// Deletes keys matching a predicate.
+    /// </summary>
+    /// <param name="shouldDelete">
+    /// A predicate applied to each expired key (and unexpired key if <paramref name="unsafeIncludeUnexpired"/> is true).
+    /// Returning true will cause the key to be deleted.
+    /// </param>
+    /// <param name="unsafeIncludeUnexpired">
+    /// True indicates that unexpired keys (which may be presently or yet to be activated) should also be
+    /// passed to <paramref name="shouldDelete"/>.
+    /// Use with caution as deleting active keys will normally cause data loss.
+    /// </param>
+    /// <returns>
+    /// True if all attempted deletions succeeded.
+    /// </returns>
+    /// <remarks>
+    /// 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, bool unsafeIncludeUnexpired = false) => throw new NotSupportedException();
}

public sealed class XmlKeyManager
{
+    public bool CanDeleteKeys { get; }
+    public bool DeleteKeys(Func<IKey, bool> shouldDelete, bool unsafeIncludeUnexpired = false)
}

namespace Microsoft.AspNetCore.DataProtection.Repositories;

+/// <summary>
+/// Represents an XML element in an <see cref="IXmlRepository"/> that can be deleted.
+/// </summary>
+public interface IDeletableElement
+{
+    /// <summary>The XML element.</summary>
+    public XElement Element { get; }
+    /// <summary>Set to true if the element should be deleted.</summary>
+    public bool ShouldDelete { get; set; }
+}

public interface IXmlRepository
{
+    /// <summary>
+    /// Indicates whether this respository supports removal.
+    /// </summary>
+    bool CanRemoveElements => false;
+
+    /// <summary>
+    /// Removes elements satisfying the given predicate.
+    /// </summary>
+    /// <param name="chooseElements">
+    /// A snapshot of the elements in this repository.
+    /// For each, set <see cref="IDeletableElement.ShouldDelete"/> to true if it should be deleted.
+    /// </param>
+    /// <returns>
+    /// True if all deletions succeeded.
+    /// </returns>
+    /// <exception cref="NotSupportedException">
+    /// If <see cref="CanRemoveElements"/> is false.
+    /// </exception>
+    bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements) => throw new NotSupportedException();
}

public class FileSystemXmlRepository
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

public class RegistryXmlRepository
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

namespace Microsoft.AspNetCore.DataProtection.EntityFrameworkCore;

public class EntityFrameworkCoreXmlRepository<TContext>
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

I didn't do the in-memory or redis repositories, which seemed too ephemeral to benefit from deletion.

Usage Examples

if (keyManager.CanDeleteKeys) 
{
    var date = DateTimeOffset.Now - TimeSpan.FromDays(90);
    keyManager.DeleteKeys(key => key.ExpirationDate < date), unsafeIncludeUnexpired: false); // Force deletion of active key
}

Alternative Designs

IXmlRepository.RemoveElements could use a complicated Func, rather than a mutating Action: https://github.com/dotnet/aspnetcore/pull/53860/commits/20b9eb4bfa4ffbb25b93a5da5cf04cb529de61bd

Risks

As with revocation, in memory representations of the deleted key will be unaffected.

We'll have to be careful how we revise/supplement the docs, since key deletion is a foot-gun.

There's no specific handling of concurrent deletions or externally-driven storage changes. Since this is basically a perf optimization, deletion is best effort and there's logging and a boolean result to tell you if not everything happened.

We can't easily bring the new APIs to .net framework, which lacks default interface methods. We can, optionally, bring some of the concrete implementations to framework.

[halter73]

API Review Notes:

• How can this ever be used safely?

  • Maybe, if you follow our guidance not to store anything long term or only use it indirectly through things like the CookieAuthenticationHandler and Antiforgery.
  • The docs do indicate that DataProtection should only be used for "ephemeral" data.
  • "Warning: It's not advised to use these APIs to protect payloads which require long-term or indefinite persistence. "Can I afford for the protected payloads to be permanently unrecoverable after a month?" can serve as a good rule of thumb; if the answer is no then developers should consider alternative APIs." https://learn.microsoft.com/en-us/aspnet/core/security/data-protection/consumer-apis/limited-lifetime-payloads?view=aspnetcore-8.0
  • It might be safer for usages of ITimeLimitedDataProtector specifically if we know the expiration is protected data is always short enough.
  • That said, @sebastienros said that "I use dataprotection for SAS urls" which are persisted. We assume that's the case for a lot of other users.
  • Should we add "Unsafe" to the method names?

• How bad is it really to never delete keys?

  • Keys are only read once a day and created every 90 days default, so it shouldn't be that costly.

• Why do we need this to be an abstraction vs something implemented on specific stores? Is there ever someone who would want to call this without knowing exactly what IXmlRepository is being used?

  • It seems dangerous for a library to do this considering only application developers would know how long they need to be able to unprotect protected data for.
  • It's possible, but inconvenient, to manually trim the FileSystemXmlRepository if you know the implementation details.

• Should XmlKeyManager implement the new interface methods explicitly?

  • No. Public virtual is fine.

• Do we want to create a new interface rather than use DIMs?

  • DIMs were chosen to avoid the proliferation of too many interfaces, but it does mean that the interface APIs will have to be #ifdefed out of netstandard.

• Should unsafeIncludeUnexpired even be a parameter considering you can filter yourself?

  • You can filter yourself, but it seemed safer to exclude these by default.
  • However, this only prevents you from deleting keys that are expired for the purposes of protecting new data. Expired keys can always be used to decrypt old data if the key has not been revoked.

• Why Action<IReadOnlyCollection<IDeletableElement>> instead of Func<XElement, bool>

  • An IKey cannot be created until you have every XElement so you know whether or not the key is revoked.

• Should we include RedisXmlRepository in this?

  • It wasn't included initially because redis is an in-memory database, but considering redis does have persistence options, we would like to include it to make sure the abstraction works.

After a lot of discussion that focused mostly on whether or not we should add an API to delete data protection keys at all considering the potentially risks and relatively small payoff, we did not come to a consensus either way. We will continue this discussion at a later point.

[amcasey]

It seems dangerous for a library to do this considering only application developers would know how long they need to be able to unprotect protected data for.

I think this comment was about potential future consumers but, in the event that it was about the key manager described in https://github.com/dotnet/aspnetcore/issues/52916, that's a scenario-specific app and not a library.

It's possible, but inconvenient, to manually trim the FileSystemXmlRepository if you know the implementation details.

And, if you've already violated the abstraction that far (by downcasting and doing your own xml parsing), you might as well just access the files on disk.

DIMs were chosen to avoid the proliferation of too many interfaces, but it does mean that the interface APIs will have to be #ifdefed out of netstandard.

It also means having to add a CanDelete property, and those will also proliferate. A bool is likely faster than a runtime type check, but that shouldn't matter at the scale at which it would be called.

[brockallen]

Your updates here got me wondering why not just add APIs to the store level (IXmlRepository) and specifically don't expose delete on the key manager.

[amcasey]

@brockallen Obviously, there's room for debate when a feature is not intended to be conveniently usable, but I'll lay out my personal reasoning: the IXmlRepository is a very thin layer over the backing store. To manipulate keys via the repository, you have to know the XML format (which is documented), not only of individual elements but of how they related (e.g. revocations are stored separately from the keys they revoke). Once you have to implement all that logic yourself (as the app developer who wants to delete keys), it's a small additional step to just manipulate the backing store directly.

Are you aware of a scenario where an app developer would want to delete keys (which requires intimate knowledge of how the app uses them) without also knowing where they're stored (which having an IXmlRepository API would abstract away)?

[brockallen]

Ok, yes I see what you're getting at... mainly because the only non-payload thing we have at this level is friendlyName (and even that isn't really too concrete). It was just a thought to put it at a level where it was sufficiently obscure, but available. But you're right -- in short, there's no identifier at this level. Bummer.

[amcasey]

I thought we might be able to use friendlyName but (a) it's optional and not all impls actually use it and (b) the impls that use it may reject it for containing invalid characters and substitute their own!

[amcasey]

This still feels like a case where a specialized tool could address the same needs. If it were to be a public API, we could make it more intuitive to call but harder to implement by dropping the lambdas and making it appear to have random access. The implementations that need this (now) do/could provide random access.

Still not ready to approve this.

[Tratcher]

1. The key management API is incomplete without the ability to delete keys, it leaks resources without a way to clean them up.
2. The workaround of manually deleting keys requires knowing the structure of each store and file, as well as how entries like revocation are associated with keys. This is a high risk approach for implementation, maintenance, and compat. APIs would be much safer, easier, and forward compatible.
3. If the developer needs to do it, then help them do it right.

[amcasey]

• Not having deletion could be said to make the storage API conceptually incomplete

• Most people will never call this API (since most people never call the existing APIs)

• We could have a simpler, random-access version of this API

  • Not all implementations could accommodate it
  • Deleting multiple keys would likely be slow

• Do we need unsafeIncludeUnexpired?

  • Nope - that was to make it slightly harder to hurt yourself

• Expected to be consumed in 9.0 (i.e. not waiting until 10.0)

• The order in which keys are deleted is important - don't delete a revocation before the corresponding key

  • Could add a priority order to IDeletableElement and delete in priority order
  • Explicitly document that deletion must stop if any step fails
  • Keep ShouldDelete (rather than using a sentinel priority)

• We could call repository deletion twice - once for keys and again for revocations

  • That would cause problems if the set of elements changed in between the calls

• @amcasey to go figure out an ordering API (and remove unsafeIncludeUnexpired) and return

• We're not going to block the API, even though we don't like it

[amcasey]

We should also implement this for the in-memory repository.

[amcasey]

Proposed API

namespace Microsoft.AspNetCore.DataProtection.KeyManagement;

public interface IKeyManager
{
+#if NETCOREAPP
+    /// <summary>
+    /// Indicates whether this key manager supports key deletion.
+    /// </summary>
+    /// <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.
+    /// </remarks>
+    bool CanDeleteKeys => false;
+
+    /// <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>
+    /// 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) => throw new NotSupportedException();
+#endif
}

public sealed class XmlKeyManager
{
+#if NETCOREAPP
+    public bool CanDeleteKeys { get; }
+    public bool DeleteKeys(Func<IKey, bool> shouldDelete)
+#endif
}

namespace Microsoft.AspNetCore.DataProtection.Repositories;

+/// <summary>
+/// Represents an XML element in an <see cref="IXmlRepository"/> that can be deleted.
+/// </summary>
+public interface IDeletableElement
+{
+    /// <summary>The XML element.</summary>
+    public XElement Element { get; }
+    /// <summary>Elements are deleted in increasing DeletionOrder.  <c>null</c> means "don't delete".</summary>
+    public int? DeletionOrder { get; set; }
+}

public interface IXmlRepository
{
+#if NETCOREAPP
+    /// <summary>
+    /// Indicates whether this respository supports removal.
+    /// </summary>
+    bool CanRemoveElements => false;
+
+    /// <summary>
+    /// Removes selected elements from the repository.
+    /// </summary>
+    /// <param name="chooseElements">
+    /// A snapshot of the elements in this repository.
+    /// For each, set <see cref="IDeletableElement.DeletionOrder"/> to a non-<c>null</c> value if it should be deleted.
+    /// Elements are deleted in increasing order.  If any deletion fails, the remaining deletions *MUST* be skipped.
+    /// </param>
+    /// <returns>
+    /// True if all deletions succeeded.
+    /// </returns>
+    /// <exception cref="NotSupportedException">
+    /// If <see cref="CanRemoveElements"/> is false.
+    /// </exception>
+    bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements) => throw new NotSupportedException();
+#endif
}

public class FileSystemXmlRepository
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

public class RegistryXmlRepository
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

namespace Microsoft.AspNetCore.DataProtection.EntityFrameworkCore;

public class EntityFrameworkCoreXmlRepository<TContext>
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

namespace Microsoft.AspNetCore.DataProtection.StackExchangeRedis;

public class RedisXmlRepository
{
+    public virtual bool CanRemoveElements { get; }
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

[amcasey]

I don't actually have an implementation for RedisXmlRepository yet. If it turns out not to be efficiently implementable, it might be preferable to drop that public API and let it return false for now.

[amcasey]

There's no public API for EphemeralXmlRepository because it's internal.

[amcasey]

Make new interfaces, rather than using #if.

[amcasey]

~Approved modulo interface changes.~ Edit: changes incoming.

[amcasey]

Proposed API

namespace Microsoft.AspNetCore.DataProtection.KeyManagement;

+/// <summary>
+/// The basic interface for performing key management operations.
+/// </summary>
+/// <remarks>
+/// Instantiations of this interface are expected to be thread-safe.
+/// </remarks>
+public interface IDeletableKeyManager : IKeyManager
+{
+    /// <summary>
+    /// Indicates whether this key manager supports key deletion.
+    /// </summary>
+    /// <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.
+    /// </remarks>
+    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>
+    /// 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);
+}

// These could be private - they're non-virtual on a sealed type
public sealed class XmlKeyManager
{
+    public bool CanDeleteKeys { get; }
+    public bool DeleteKeys(Func<IKey, bool> shouldDelete)
}

namespace Microsoft.AspNetCore.DataProtection.Repositories;

+ /// <summary>
+ /// Represents an XML element in an <see cref="IXmlRepository"/> that can be deleted.
+ /// </summary>
+ public interface IDeletableElement
+ {
+     /// <summary>The XML element.</summary>
+     public XElement Element { get; }
+     /// <summary>Elements are deleted in increasing DeletionOrder.  <c>null</c> means "don't delete".</summary>
+     public int? DeletionOrder { get; set; }
+ }

+ /// <summary>
+ /// The basic interface for storing and retrieving XML elements.
+ /// </summary>
+ public interface IDeletableXmlRepository : IXmlRepository
+ {
+     /// <summary>
+     /// Removes selected elements from the repository.
+     /// </summary>
+     /// <param name="chooseElements">
+     /// A snapshot of the elements in this repository.
+     /// For each, set <see cref="IDeletableElement.DeletionOrder"/> to a non-<c>null</c> value if it should be deleted.
+     /// Elements are deleted in increasing order.  If any deletion fails, the remaining deletions *MUST* be skipped.
+     /// </param>
+     /// <returns>
+     /// True if all deletions succeeded.
+     /// </returns>
+     bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements);
+ }

-public class FileSystemXmlRepository : IXmlRepository
+public class FileSystemXmlRepository : IDeletableXmlRepository
{
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

-public class RegistryXmlRepository : IXmlRepository
+public class RegistryXmlRepository : IDeletableXmlRepository
{
+    public virtual bool RemoveElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

[Tratcher]

IDeletableKeyManager.CanDeleteKeys seems redundant now that there's a new interface.

[amcasey]

IDeletableKeyManager.CanDeleteKeys seems redundant now that there's a new interface.

It exposes whether the underlying repository implements its new interface.

[amcasey]

API Approved!

[amcasey]

Minor change - "remove" -> "delete" for consistency. Approved offline.

Proposed API

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 supports key deletion.
+    /// </summary>
+    /// <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.
+    /// </remarks>
+    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>
+    /// 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);
+}

// These could be private - they're non-virtual on a sealed type
-public sealed class XmlKeyManager : IKeyManager
+public sealed class XmlKeyManager : IDeletableKeyManager
{
+    public bool CanDeleteKeys { get; }
+    public bool DeleteKeys(Func<IKey, bool> shouldDelete)
}

namespace Microsoft.AspNetCore.DataProtection.Repositories;

+ /// <summary>
+ /// Represents an XML element in an <see cref="IXmlRepository"/> that can be deleted.
+ /// </summary>
+ public interface IDeletableElement
+ {
+     /// <summary>The XML element.</summary>
+     public XElement Element { get; }
+     /// <summary>Elements are deleted in increasing DeletionOrder.  <c>null</c> means "don't delete".</summary>
+     public int? DeletionOrder { get; set; }
+ }

+ /// <summary>
+ /// An extension of <see cref="IXmlRepository"/> that supports deletion of elements.
+ /// </summary>
+ public interface IDeletableXmlRepository : IXmlRepository
+ {
+     /// <summary>
+     /// Deletes selected elements from the repository.
+     /// </summary>
+     /// <param name="chooseElements">
+     /// A snapshot of the elements in this repository.
+     /// For each, set <see cref="IDeletableElement.DeletionOrder"/> to a non-<c>null</c> value if it should be deleted.
+     /// Elements are deleted in increasing order.  If any deletion fails, the remaining deletions *MUST* be skipped.
+     /// </param>
+     /// <returns>
+     /// True if all deletions succeeded.
+     /// </returns>
+     bool DeleteElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements);
+ }

-public class FileSystemXmlRepository : IXmlRepository
+public class FileSystemXmlRepository : IDeletableXmlRepository
{
+    public virtual bool DeleteElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}

-public class RegistryXmlRepository : IXmlRepository
+public class RegistryXmlRepository : IDeletableXmlRepository
{
+    public virtual bool DeleteElements(Action<IReadOnlyCollection<IDeletableElement>> chooseElements)
}