-
Notifications
You must be signed in to change notification settings - Fork 827
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
Fixing updatekeys command to respect configured shamir_threshold with groups #1509
base: main
Are you sure you want to change the base?
Fixing updatekeys command to respect configured shamir_threshold with groups #1509
Conversation
f4dd2b1
to
02614a7
Compare
02614a7
to
7c4fc0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
@@ -593,7 +593,6 @@ func main() { | |||
for _, path := range c.Args() { | |||
err := updatekeys.UpdateKeys(updatekeys.Opts{ | |||
InputPath: path, | |||
GroupQuorum: c.Int("shamir-secret-sharing-quorum"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should have used the option shamir-secret-sharing-threshold
. My guess is that this was simply forgotten in 63708c6, resp. that the option was renamed in parallel to a PR that added/modified this, and thus the old name got re-introduced here. (Turns out the PR was created after merging the rename: #255. If you look at the commits though, you can see that the first commit which already made use of the old name was created before the rename PR got merged.)
Please re-add GroupQuorum
with the correct option value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the feedback!
Just to double-check: My understanding is that updatekeys
should use the .sops.yaml
config file as the source for key updates. Since the config file has the option to set the Shamir threshold through the shamir_threshold
key, shouldn't the user be encouraged to use that?
I think allowing both the shamir_threshold
key in the config file and the shamir-secret-sharing-threshold
command line option would require prompting the user to resolve the conflict if both are set and differ. Currently, that is not implemented. In fact, the shamir_threshold
in .sops.yaml was not respected at all, which seem to run counter to the intention of the updatekeys
command in the first place.
Furthermore, neither shamir-secret-sharing-threshold
nor shamir-secret-sharing-quorum
are on the list of allowed options for the command. This led me to believe that the option is not meant to be in use.
IMHO the implementation and use of updatekeys
is simpler and more consistent when relying on the configuration file only for the Shamir threshold, but I can try to reintroduce (and rename) the shamir-secret-sharing-threshold
option, but adding:
shamir-secret-sharing-threshold
to the list of allowed options- some notification to the user about the conflict, if both the option and the configuration key are set and differ.
Your input on this will be greatly appreciated.
} | ||
|
||
return diff | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the logic to compute the new shamir theshold should be part of a diff function. I think it should be a separate function, that can be used in the diff function (though I would avoid that and simply pass old and new value on after the new value has been computed).
Also please note that you need to sign-off your commits for the DCO. |
I noticed a bug when running
sops updatekeys <filename>
after changing the.sops.yaml
configuration from a single master key to 3 groups with master keys and ashamir_threshold
below the groups size. The threshold configuration was not respected, and the group size was always used. There was also a problem with increasing theshamir_threshold
. If the threshold was still below the group size, the old threshold were used.This PR aims to solve the following issues:
shamir-secret-sharing-quorum
, and the relatedGroupQuorum
option that was always0
shamir_threshold
defined in the configurationExample to reproduce the problem
Use the following
.sops.yaml
to encrypt a fileThen change
.sops.yaml
toAfter running
sops updatekeys <filename>
, the resulting encrypted file has a metadata section starting with:I.e
shamir_threshold
in.sops.yaml
was not respected, and the number of groups was used instead.User experience after change
Message when running
sops updatekeys <filename>
after changing.sops.yaml
from no groups to 3 groups, withshamir_threshold
configured as 2After confirmation the the resulting encrypted file has a metadata section starting with:
Message when changing
shamir_threshold
from 2 to 3 (equal to removingshamir_threshold
from configuration)After confirmation the the resulting encrypted file has a metadata section starting with: