Allow deletion of Data Protection keys

[amcasey]

Background and Motivation

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.

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 a specific key and its revocations.
+    /// </summary>
+    /// <param name="keyId">The id of the key to delete.</param>
+    /// <param name="evenIfActive">True to force deletion of still-active keys.  Subsequent consumers of the key will throw.</param>
+    /// <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>
+    /// <exception cref="InvalidOperationException">
+    /// If the key is not found or if the key is active and <paramref name="evenIfActive"/> is false.
+    /// </exception>
+    void DeleteKey(Guid keyId, bool evenIfActive = false) => throw new NotSupportedException();
}

Usage Examples

if (keyManager.CanDeleteKeys) 
{
    keyManager.DeleteKey(keyId, evenIfActive: true); // Force deletion of active key
}

Alternative Designs

We talked about TryDelete, but I didn't like that it was returning a value for all keys, rather than the specific argument.

We don't presently see a need for DeleteAllKeys and it could be added later as a default interface method.

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.

[josephdecock]

I'm generally skeptical of this, and strongly agree that "key deletion is a foot gun"! I suppose the xmldoc saying not to use this unless space constraints are forcing you to do so is good. I'm just having a hard time imagining an environment that is so space constrained that this is needed. It seems like a pretty extreme edge case, and for the very small number of use cases, there's nothing stopping you from deleting the keys in the store outside the manager.

The advantage of NOT doing this is that it makes it obvious to the user how destructive this is. You can have in the docs that this is so destructive that we don't even have an api for it.

[Tratcher]

I'm aware of the risks, that's why I included suggestions like the evenIfActive option to warn you not to delete anything still in use.

Let me give you the higher-level picture:

• I need to automate key management for many instances.
• These instances can run for years and accumulate any number of keys. No, space isn't an issue at first, but we can't let them build up forever. Even if it's not the storage cost, there's added clutter to investigations and maintenance if something goes wrong.
• We don't plan to have apps delete keys, but to do it via a separate key manager job. See Define a pattern for a component that ensures a data protection keyring has an active key #52916.
• To delete keys from the store requires manual administrative work and knowing the storage format details. You risk breaking something even worse if you mess up the storage. Doing manual work does not scale to many instances.

Overall, having a delete API gives us the safest, most compatible way to interact with storage, and lets us keep a clean house (or many) indefinitely.

[amcasey]

We can do the usual thing of adding scary doc comments and prepending "Unsafe" to the name, but I can't say I've ever seen that make a difference in practice.

[josephdecock]

I like the idea of having a flag like evenIfActive to help prevent data loss, but I worry about the case where a key is retired but the data it protected is still needed. In IdentityServer, we use data protection to protect some fairly long lived data. For example:

• Signing Keys are rotated every 90 days (or a configurable amount of time)
• Refresh tokens have a lifetime of 30 days (or again a configurable lifetime, 6 months-1 year+ is not unheard of!)

Maybe instead of a flag, you could do something like TimeSpan retiredFor = TimeSpan.FromDays(365). The idea would be, deletion won't proceed unless the key was retired at least that much time in the past, with a TimeSpan of length 0 acting as the override to delete an active key. Doing it this way you still get the ability to bypass the safety mechanism and the default is to have the safety in place for a longer period of time.

[Tratcher]

Interesting idea. I don't think method TimeSpan args can have default parameters; they have to be consts?

[amcasey]

You could make it nullable and use a (computed?) default value on null.

[Tratcher]

Maybe instead of a flag, you could do something like TimeSpan retiredFor = TimeSpan.FromDays(365). The idea would be, deletion won't proceed unless the key was retired at least that much time in the past, with a TimeSpan of length 0 acting as the override to delete an active key.

By that logic 0 would allow deleting any expired key and you'd need a negative value to delete an active key 😆. I like it.

[MackinnonBuck]

API review notes:

• In practice, we will need to add #ifdefs around default interface members for compatibility with netstandard2.0 and .NET Framework.
• Passing negative values to delete keys might be strange
  • Using TimeSpan seems like a decent alternative
• We could replace evenIfActive with an expiredFor parameter:
  • A default value of default means it's up to the implementer to define a sensible policy
  • A value of 0 could mean:
    • Allow deletion of active keys
    • Allow deletion of keys that have been expired for any amount of time
  • Should there be a default value for the expiredFor parameter?
    • Conclusion: That seems fine
• Note: XmlKeyManager will also need similar changes to its API
• Note: We may also need to make breaking changes to IInternalXmlKeyManager.

Updated API:

namespace Microsoft.AspNetCore.DataProtection.KeyManagement;

public interface IKeyManager
{
+    bool CanDeleteKeys => false;
+    void DeleteKey(Guid keyId, TimeSpan? expiredFor = default) => throw new NotSupportedException();
}

API approved!

[brockallen]

I guess new APIs will be needed on things like IXmlRepository too?

[amcasey]

@brockallen Can you elaborate? What APIs would be needed on the repository?

[brockallen]

@brockallen Can you elaborate? What APIs would be needed on the repository?

Well, some sort of delete, right? Or am I misunderstanding where that fits into the DP stack?

[amcasey]

Nope, you're quite right. That's what I get for making the proposal issue without a backing PR. The manager uses the repository, which is also a public API. Thanks!

Edit: There's no revoke member on the repository because revocation is represented as a form of storage. Obviously, that won't be the case for deletion, since the goal is to avoid excessive storage.
Edit 2: IXmlRepository doesn't support random access, so something fancier will be required.

[amcasey]

Not having random access completely changes the shape of the API. I'll file a new issue once I've got a concrete implementation we can discuss.

[brockallen]

Edit 2: IXmlRepository doesn't support random access, so something fancier will be required.

I know of many implementations of IXmlRepository in the wild, so please keep this in mind as new features are added/designed.

[amcasey]

@brockallen Are any of them running of framework? That's harder because we can't depend on default interface implementation.

[brockallen]

of framework

You mean "on"? As in older .NET Framework? I don't think so -- most of them would have been .NET Core and beyond.

[amcasey]

Yep, sorry for the typo.