Skip to content

Allow shamir_threshold: 1 #1123

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Allow shamir_threshold: 1 #1123

wants to merge 1 commit into from

Conversation

puiterwijk
Copy link

This allows the case where multiple keygroups are used, but any one of the groups needs to be able to decrypt the secret. This is for example useful if some people use age keys and some use pgp, but each of them should be able to decrypt the secret by themselves.

Fixes: #878

@puiterwijk
Allow shamir_threshold: 1
c110e5b 
This allows the case where multiple keygroups are used, but any one of
the groups needs to be able to decrypt the secret.
This is for example useful if some people use age keys and some use pgp,
but each of them should be able to decrypt the secret by themselves.

Fixes: getsops#878
@D4ndellion
Copy link

D4ndellion commented Dec 18, 2022 •
edited
Loading

if some people use age keys and some use pgp, but each of them should be able to decrypt the secret by themselves.

You can actually mix PGP and age keys together in a single group to accomplish this.

key_groups:
- age:
  - *user1
  - *host2
  pgp:
  - *user2

Note the lack of a - on the pgp line to put them in the same group.

In case someone stumbles upon this PR after trying to use both age and gpg keys at the same time. Putting them in different groups is NOT a requirement

@tonobo
Copy link

tonobo commented Feb 27, 2023 •
edited
Loading

Well, this allows grouping multiple keys together, which is currently not possible:

keys:
  dept_a: &dept_a
    age:
      - &user_a age...
      - &user_b age
  dept_b: &dept_b
    age:
      - &user_c age...
      - &user_d age


creation_rules:
  - path_regex: \.*flux/.*
    encrypted_regex: '^(stringData|data)$'
    shamir_threshold: 1
    key_groups:
      - *dept_a
      - *dept_b

To explain the issue in more detail: The keys are a resource that could be managed across projects and might be updated automatically and creation_rules are just referring those keys (more project specific). Organizing the keys is kind of bad at the moment as key_groups are incredibly strict. it it supports multiple nesting nesting levels such as flatten all the keys withing the section might help as well, but this PR might fix our case also.

@hiddeco hiddeco added this to the v3.9.0 milestone Jul 4, 2023
felixfontein
felixfontein reviewed Aug 23, 2023
View reviewed changes
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

The change looks correct.

I'm still not convinced that this is a really good idea, since there is no good reason to allow the threshold to be 1, except as a hack. It might be better to extend the config file format instead to make joining lists for the same key type using YAML anchors easier.

On the other hand, allowing a threshold of 1 doesn't really hurt. The worst case I can think of is that some user notices that a threshold of 1 works and thinks that it is the smallest value that forces that keys from more than one group. (I do hope nobody tries that, but on the other hand some folks also decided to use an exponent of 1 for RSA, so... :) )

shamir/shamir_test.go
for _, share := range out {
if len(share) != len(secret)+1 {
t.Fatalf("bad: %v", out)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably also check that the first four bytes of each share are identical to secret.

@pbsds
Copy link

pbsds commented Oct 15, 2023

Using a threshold=1 as a means to merge key groups is a hack, which also removes the ability to use shamir thresholding should it be needed. I'm rather in favor of either flattening lists, or introducing a merge type:

Flattening only works within each type of key:

keys:
  dept_a: &dept_a
    - &user_a age...
    - &user_b age...
  dept_b: &dept_b
    - &user_c age...
    - &user_d age...

creation_rules:
  - path_regex: \.*flux/.*
    encrypted_regex: '^(stringData|data)$'
    shamir_threshold: 1
    key_groups:
    - age:
      - *dept_a
      - *dept_b

While a merge target can deal with multiple kinds of keys, and be used agnostically:

keys:
  dept_a: &dept_a
    age:
      - &user_a age...
      - &user_b age...
  dept_b: &dept_b
    age:
      - &user_c age...
      - &user_d age...

creation_rules:
  - path_regex: \.*flux/.*
    encrypted_regex: '^(stringData|data)$'
    shamir_threshold: 1
    key_groups:
    - merge:
      - *dept_a
      - *dept_b

@jonasbadstuebner
Copy link
Contributor

I would opt for the merge target. Are you open for a new PR in this regard @felixfontein ?

@felixfontein
Copy link
Contributor

merge sounds good to me!

jonasbadstuebner added a commit to jonasbadstuebner/sops that referenced this pull request Apr 23, 2024
@jonasbadstuebner
feat(config): merge key_groups
9fbc9ea 
closes getsops#1123
@jonasbadstuebner jonasbadstuebner mentioned this pull request Apr 23, 2024
Merged
jonasbadstuebner added a commit to jonasbadstuebner/sops that referenced this pull request Apr 23, 2024
@jonasbadstuebner
feat(config): merge key_groups
372d21b 
closes getsops#1123

Signed-off-by: Jonas Badstübner <jonas.badstuebner@hetzner-cloud.de>
jonasbadstuebner added a commit to jonasbadstuebner/sops that referenced this pull request Jun 3, 2024
@jonasbadstuebner
feat(config): merge key_groups
8b39237 
closes getsops#1123

Signed-off-by: Jonas Badstübner <jonas.badstuebner@hetzner-cloud.de>
jonasbadstuebner added a commit to jonasbadstuebner/sops that referenced this pull request Jun 4, 2024
@jonasbadstuebner
feat(config): merge key_groups
ec03063 
closes getsops#1123

Signed-off-by: Jonas Badstübner <jonas.badstuebner@hetzner-cloud.de>
jonasbadstuebner added a commit to jonasbadstuebner/sops that referenced this pull request Jun 10, 2024
@jonasbadstuebner
feat(config): merge key_groups
78b2ab1 
closes getsops#1123

Signed-off-by: Jonas Badstübner <jonas.badstuebner@hetzner-cloud.de>
@felixfontein felixfontein modified the milestones: v3.9.0, 3.10.0 Jun 26, 2024
@felixfontein
Copy link
Contributor

While the problem sketched in #878 can now be solved with merge groups, I'm not sure about what was mentioned in this PR:

This allows the case where multiple keygroups are used, but any one of the groups needs to be able to decrypt the secret. This is for example useful if some people use age keys and some use pgp, but each of them should be able to decrypt the secret by themselves.

@puiterwijk I think this can also all be covered with merge groups, but maybe I'm missing something?

@felixfontein felixfontein removed this from the 3.10.0 milestone Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to set shamir_threshold to 1 or disable it.
7 participants
@puiterwijk @D4ndellion @tonobo @pbsds @jonasbadstuebner @felixfontein @hiddeco