Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support key deletion in Data Protection #53860

Merged
merged 40 commits into from
Jul 18, 2024
Merged

Support key deletion in Data Protection #53860

merged 40 commits into from
Jul 18, 2024

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Feb 6, 2024

Add the ability to delete keys to support #52916. The approach is more complicated than you'd expect to account for the fact that IXmlRepository offers only two (possibly slow) operations: enumerate and add - there's no random access.

Deleting keys remains discouraged.

Fixes #53880

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-dataprotection Includes: DataProtection label Feb 6, 2024
@ghost ghost added the area-dataprotection Includes: DataProtection label Feb 6, 2024
@amcasey amcasey marked this pull request as ready for review February 7, 2024 22:37
@amcasey amcasey requested a review from a team as a code owner February 7, 2024 22:37
@@ -238,15 +238,36 @@ private static bool IsLogLevelEnabledCore([NotNullWhen(true)] ILogger? logger, L
[LoggerMessage(60, LogLevel.Warning, "Storing keys in a directory '{path}' that may not be persisted outside of the container. Protected data will be unavailable when container is destroyed. For more information go to https://aka.ms/aspnet/dataprotectionwarning", EventName = "UsingEphemeralFileSystemLocationInContainer")]
public static partial void UsingEphemeralFileSystemLocationInContainer(this ILogger logger, string path);

[LoggerMessage(61, LogLevel.Trace, "Ignoring configuration '{PropertyName}' for options instance '{OptionsName}'", EventName = "IgnoringReadOnlyConfigurationForNonDefaultOptions")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess 60 was missed, so it appears after 66, which seems to have confused whoever added the following items. Can I update these, given that they're wrong now?

@amcasey
Copy link
Member Author

amcasey commented Feb 7, 2024

I had to move to per-target-platform API manifests because DIM isn't available on framework.

@amcasey
Copy link
Member Author

amcasey commented Feb 7, 2024

I'll add tests once the API is reviewed, but the shape has changed several times already.

@amcasey
Copy link
Member Author

amcasey commented May 2, 2024

Force push is a rebase - I'll address API Review feedback separately.

@amcasey amcasey removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 3, 2024
@amcasey
Copy link
Member Author

amcasey commented May 3, 2024

I also prepared a version where the key manager gets to return an ordered list of IDeletableElements to delete, but I was unhappy with the fact that the implementation could return duplicate elements or elements that were not in the list it received. These are both solvable problems, but every repository implementation would need these safeguards, which I found less appealing than having every repository sort.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 10, 2024
@amcasey
Copy link
Member Author

amcasey commented Jul 15, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tratcher have you taken a look at this recently? Does it meet your needs?

@amcasey
Copy link
Member Author

amcasey commented Jul 17, 2024

I'm definitely a little curious how my new tests could pass only on Windows. Investigating.

Edit: Opening a file with FileShare.None doesn't prevent its deletion on Linux (where there is also no registry 🤦).

amcasey added 3 commits July 17, 2024 17:33
The only way I could find to make FileSystemInfo.Delete throw was to remove write permission to the parent directory and we don't have enough hooks to do and undo that around a single file deletion attempt (i.e. it's all or none).
@amcasey amcasey enabled auto-merge (squash) July 18, 2024 00:34
@amcasey amcasey merged commit 7e99eea into dotnet:main Jul 18, 2024
26 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 18, 2024
@amcasey amcasey deleted the DeleteKey branch July 18, 2024 15:18
@donnyv
Copy link

donnyv commented Aug 25, 2024

I found this while building a custom repository for Mongodb.

One idea that may be to late, since you look like you are pretty far along. You could simplify this by just adding a DeleteElement(string keyid) to IXmlRepository. Then let devs developing their own repositories to create the delete code. DeleteElement() method could be fired by Data Protection using a TTL. A TTL could be added to every key automatically when its created. Since the minimum expiration that can be set to a Key is 7 days. Data Protection could check the TTL every 7 days and fire deletes for all the expired keys. TTL date could be (expired date + time to live length) = TTL. Time to live length could be a setting with a default of (expired date * 2). I would make it so that devs would have to turn this feature on too.

@amcasey
Copy link
Member Author

amcasey commented Sep 5, 2024

@donnyv The reason we didn't add DeleteElement(string keyid) is that random access is not presently part of the IXmlRepository contract and a number of existing implementations (e.g. file system) couldn't implement it efficiently.

Having said that, it was certainly our intention that IXmlRepository implementers would be able to provide their own implementations. Are you finding that not to be the case?

Unless we hear a lot of demand (feel free to open an issue to collect feedback), I don't think we're likely to add an automated TTL mechanism. Generally speaking, we recommend keys never be deleted, so we want doing so to be a very explicit action on the app author's part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dataprotection Includes: DataProtection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow deletion of Data Protection keys
5 participants