Fixing updatekeys command to respect configured shamir_threshold with groups #1509

jorn-ola-birkeland · 2024-05-15T21:01:32Z

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 a shamir_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 the shamir_threshold. If the threshold was still below the group size, the old threshold were used.

This PR aims to solve the following issues:

Remove the unused argument shamir-secret-sharing-quorum, and the related GroupQuorum option that was always 0
Correctly respect the shamir_threshold defined in the configuration
Include changes to the Shamir threshold in the evaluation of whether or not the file will change
Present the changes to the Shamir threshold to the user for confirmation

Example to reproduce the problem
Use the following .sops.yaml to encrypt a file

creation_rules:
  - path_regex: <pattern>
    gcp_kms: <gcp_resource>

Then change .sops.yaml to

creation_rules:
  - path_regex: <pattern>
    shamir_threshold: 2
    key_groups:
      - gcp_kms:
          - resource_id: <gcp_resource>
        age:
          - <age key 1>
      - age:
          - <age key 2>
          - <age key 3>
      - age:
          - <age key 4>

After running sops updatekeys <filename>, the resulting encrypted file has a metadata section starting with:

sops:
    shamir_threshold: 3
    key_groups:
        - gcp_kms:
           ...

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, with shamir_threshold configured as 2

The following changes will be made to the file's groups:
+++ shamir_threshold: 2
Group 1
    <gcp_resource>
+++ <age key 1>
Group 2
+++ <age key 2>
+++ <age key 3>
Group 3
+++ <age key 4>
Is this okay? (y/n):

After confirmation the the resulting encrypted file has a metadata section starting with:

sops:
    shamir_threshold: 2
    key_groups:
        - gcp_kms:
           ...

Message when changing shamir_threshold from 2 to 3 (equal to removing shamir_threshold from configuration)

The following changes will be made to the file's groups:
+++ shamir_threshold: 3
--- shamir_threshold: 2
Group 1
    <gcp_resource>
    <age key 1>
Group 2
    <age key 2>
    <age key 3>
Group 3
    <age key 4>
Is this okay? (y/n):

After confirmation the the resulting encrypted file has a metadata section starting with:

sops:
    shamir_threshold: 3
    key_groups:
        - gcp_kms:
           ...

… groups

felixfontein

Thanks for your contribution!

felixfontein · 2024-06-01T21:57:19Z

cmd/sops/main.go

@@ -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"),


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.

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.

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?

Well, I kind of agree, but the updatekeys feature was originally implemented to explicitly allow overriding the Shamir threshold on the command line. (Unfortunately, or fortunately, depending on your POV, that implementation didn't work, due to the option rename.)

One reason is probably that the command is called updatekeys, and not update-according-to-sops.yaml or something like that. It's also not adjusting other options from .sops.yaml (about which parts to encrypt etc.). So there is a reason to allow overriding the Shamir threshold from the command line IMO. Even if it's not too strong.

@getsops/maintainers any opinions on this one? I'm somewhat torn between "let's fix this as it was originally intended to be" and "let's remove it since it never worked anyway, and .sops.yaml should be the single source of truth for updatekeys, also for other things than keys".

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.

I don't think so - the command line option is an override, if it's supplied the value from .sops.yaml is ignored. The same happens in quite a few other cases for other subcommands. (The Shamir threshold is somewhat special since you cannot explicitly disable it fom the command line, you can only enable it.)

I think your reasoning is sound, @felixfontein. Reintroducing the option on the command line is a much smaller change. Using the shamir_threshold in the configuration "feels close" to updating the key groups, but I agree it is just on the spectrum of other things that could be considered by the updatekeys command, which would have been increasingly misnamed.

I think I'll revert to fix the option for now, and that a new update-according-to-sops-yaml command of sorts would be a better solution to what I was trying to achieve.

Sounds good! (And we definitely have to find a better name than update-according-to-sops-yaml if we're going to add that one :D )

felixfontein · 2024-06-01T21:59:25Z

cmd/sops/common/common.go

+	}
+
+	return diff
+}


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).

felixfontein · 2024-06-02T05:00:18Z

Also please note that you need to sign-off your commits for the DCO.

felixfontein · 2024-06-11T21:41:03Z

@jorn-ola-birkeland is there a reason you closed this instead of adjusting the PR? Are you planning to still work on this?

jorn-ola-birkeland · 2024-06-12T16:47:26Z

Yes, I'll create a new branch and PR. (Re-)adding the option is quite a different fix, so I'd rather just start from scratch. BR, jobi tir. 11. jun. 2024, 23:41 skrev Felix Fontein ***@***.***>:

…

@jorn-ola-birkeland <https://github.com/jorn-ola-birkeland> is there a reason you closed this instead of adjusting the PR? Are you planning to still work on this? — Reply to this email directly, view it on GitHub <#1509 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARQTTNKXJCXYPCTGAIPHNBLZG5VINAVCNFSM6AAAAABHY44B76VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRGY2DKNRVGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

The option was renamed while updatekeys was developed, and the name was not updated in the updatekeys PR. See the following for more information: getsops#1509 (comment) Signed-off-by: Felix Fontein <[email protected]>

felixfontein · 2024-09-11T13:49:42Z

I moved some of the code changes to #1608 and #1609 and created an issue (#1610) to track the remainder.

jorn-ola-birkeland force-pushed the fix-updatekeys-shamir_threshold branch from f4dd2b1 to 02614a7 Compare May 16, 2024 08:53

Fixing updatekeys command to respect configured shamir_threshold with…

7c4fc0f

… groups

jorn-ola-birkeland force-pushed the fix-updatekeys-shamir_threshold branch from 02614a7 to 7c4fc0f Compare May 16, 2024 11:04

hanskristiansande approved these changes May 16, 2024

View reviewed changes

jorn-ola-birkeland marked this pull request as ready for review May 17, 2024 09:44

Merge branch 'main' into fix-updatekeys-shamir_threshold

3d82b94

felixfontein reviewed Jun 1, 2024

View reviewed changes

jorn-ola-birkeland closed this Jun 11, 2024

This was referenced Sep 11, 2024

updatekeys subcommand: actually use option --shamir-secret-sharing-threshold #1608

Merged

updatekeys subcommand: show changes in shamir_threshold #1609

Merged

updatekeys subcommand: allow to use shamir_threshold from configuration #1610

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing updatekeys command to respect configured shamir_threshold with groups #1509

Fixing updatekeys command to respect configured shamir_threshold with groups #1509

jorn-ola-birkeland commented May 15, 2024 •

edited

Loading

felixfontein left a comment

felixfontein Jun 1, 2024

jorn-ola-birkeland Jun 3, 2024 •

edited

Loading

felixfontein Jun 3, 2024

jorn-ola-birkeland Jun 5, 2024

felixfontein Jun 5, 2024

felixfontein Jun 1, 2024

felixfontein commented Jun 2, 2024

felixfontein commented Jun 11, 2024

jorn-ola-birkeland commented Jun 12, 2024 via email

felixfontein commented Sep 11, 2024

Fixing updatekeys command to respect configured shamir_threshold with groups #1509

Fixing updatekeys command to respect configured shamir_threshold with groups #1509

Conversation

jorn-ola-birkeland commented May 15, 2024 • edited Loading

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein Jun 1, 2024

Choose a reason for hiding this comment

jorn-ola-birkeland Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

felixfontein Jun 3, 2024

Choose a reason for hiding this comment

jorn-ola-birkeland Jun 5, 2024

Choose a reason for hiding this comment

felixfontein Jun 5, 2024

Choose a reason for hiding this comment

felixfontein Jun 1, 2024

Choose a reason for hiding this comment

felixfontein commented Jun 2, 2024

felixfontein commented Jun 11, 2024

jorn-ola-birkeland commented Jun 12, 2024 via email

felixfontein commented Sep 11, 2024

jorn-ola-birkeland commented May 15, 2024 •

edited

Loading

jorn-ola-birkeland Jun 3, 2024 •

edited

Loading