Skip to content

Conversation

@dooferlad
Copy link

Prior to this change, if we wanted to add or remove a master key, we had to rotate the data key. While there is nothing wrong with rotating the data key, having to do so at the same time as adding or removing a master key makes git history noisy.

This change adds a manage command, which allows separate master key management to data key rotation.

Prior to this change, if we wanted to add or remove a master key, we had to rotate the data key. While there is nothing wrong with rotating the data key, having to do so at the same time as adding or removing a master key makes git history noisy.

This change adds a manage command, which allows separate master key management to data key rotation.
Previously we allowed removing master keys, but that implies that the removed key is still secure. Better to assume that it isn't secure so it can no longer open the file.
@dooferlad dooferlad requested a review from felixfontein August 16, 2022 15:52
},
cli.BoolFlag{
Name: "manage, m",
Usage: "manage master keys without reencrypting all values with a new data key",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that I didn't think about this earlier, but adding new master keys without cycling the data password also has potential security implications that can be dangerous. Think of a devops git repo where a file is readable by users A and B and contains some high-value secrets. At some point it is decided that parts of that file also need to be readable by C, so the things that should not be read by C are moved to another file, and then this command is used to give C access to this file.

Since the data key has not been cycled, C now has access to the data key and to the git history, allowing C to also read the parts that C was not supposed to be able to read.

This is probably why so far there's only rotate, which always rotates the data key.

Copy link
Author

Choose a reason for hiding this comment

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

That is reasonable - do you think this could land with a big fat warning? In my case we wanted to add a second key so we had a backup key and there isn't a history problem. I really don't mind dropping this since I have already used it locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think it is ok with a big fat warning, since there are cases (like the one you are describing) where this is really useful. But I'm just a random person making comments here, my opinion doesn't count that much ;) @ajvb what do you think?

@hiddeco hiddeco added this to the v3.9.0 milestone Jul 4, 2023
@felixfontein felixfontein modified the milestones: v3.9.0, 3.10.0 Jun 26, 2024
@felixfontein felixfontein modified the milestones: 3.10.0, 3.11.0 Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants