-
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.
} | ||
|
||
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: