Allow deletion of Data Protection keys #53880
Comments
amcasey
added
area-dataprotection
|
API Review Notes:
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. |
I think this comment was about potential future consumers but, in the event that it was about the key manager described in #52916, that's a scenario-specific app and not a library.
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.
It also means having to add a |
|
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. |
|
@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)? |
|
Ok, yes I see what you're getting at... mainly because the only non-payload thing we have at this level is |
|
I thought we might be able to use |
halter73
added
api-suggestion
amcasey
added
api-ready-for-review
|
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. |
|
|
|
We should also implement this for the in-memory repository. |
amcasey
added
api-needs-work
Proposed APInamespace 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)
} |
|
I don't actually have an implementation for |
|
There's no public API for |
amcasey
added
api-ready-for-review
|
Make new interfaces, rather than using |
|
|
Proposed APInamespace 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)
} |
|
IDeletableKeyManager.CanDeleteKeys seems redundant now that there's a new interface. |
It exposes whether the underlying repository implements its new interface. |
|
API Approved! |
amcasey
added
api-approved
|
Minor change - "remove" -> "delete" for consistency. Approved offline. Proposed APInamespace 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)
} |
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:
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
Alternative Designs
IXmlRepository.RemoveElementscould use a complicatedFunc, rather than a mutatingAction: 20b9eb4Risks
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.
The text was updated successfully, but these errors were encountered: