Repository: getsops/sops (language: python)

Issue:
Allow shamir_threshold: 1
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


Discussion / hints:
>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
Well, this allows grouping multiple keys together, which is currently not possible:

```yaml
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.
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:


```yaml
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:

```yaml
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
```


I would opt for the `merge` target. Are you open for a new PR in this regard @felixfontein ?
`merge` sounds good to me!

Fix patch:
diff --git a/config/config.go b/config/config.go
index 7565091fd..9620c0da9 100644
--- a/config/config.go
+++ b/config/config.go
@@ -94,6 +94,7 @@ type configFile struct {
 }
 
 type keyGroup struct {
+	Merge   []keyGroup
 	KMS     []kmsKey
 	GCPKMS  []gcpKmsKey  `yaml:"gcp_kms"`
 	AzureKV []azureKVKey `yaml:"azure_keyvault"`
@@ -184,38 +185,72 @@ type Config struct {
 	OmitExtensions          bool
 }
 
+func deduplicateKeygroup(group sops.KeyGroup) sops.KeyGroup {
+	var deduplicatedKeygroup sops.KeyGroup
+
+	unique := make(map[string]bool)
+	for _, v := range group {
+		key := fmt.Sprintf("%T/%v", v, v.ToString())
+		if _, ok := unique[key]; ok {
+			// key already contained, therefore not unique
+			continue
+		}
+
+		deduplicatedKeygroup = append(deduplicatedKeygroup, v)
+		unique[key] = true
+	}
+
+	return deduplicatedKeygroup
+}
+
+func extractMasterKeys(group keyGroup) (sops.KeyGroup, error) {
+	var keyGroup sops.KeyGroup
+	for _, k := range group.Merge {
+		subKeyGroup, err := extractMasterKeys(k)
+		if err != nil {
+			return nil, err
+		}
+		keyGroup = append(keyGroup, subKeyGroup...)
+	}
+
+	for _, k := range group.Age {
+		keys, err := age.MasterKeysFromRecipients(k)
+		if err != nil {
+			return nil, err
+		}
+		for _, key := range keys {
+			keyGroup = append(keyGroup, key)
+		}
+	}
+	for _, k := range group.PGP {
+		keyGroup = append(keyGroup, pgp.NewMasterKeyFromFingerprint(k))
+	}
+	for _, k := range group.KMS {
+		keyGroup = append(keyGroup, kms.NewMasterKeyWithProfile(k.Arn, k.Role, k.Context, k.AwsProfile))
+	}
+	for _, k := range group.GCPKMS {
+		keyGroup = append(keyGroup, gcpkms.NewMasterKeyFromResourceID(k.ResourceID))
+	}
+	for _, k := range group.AzureKV {
+		keyGroup = append(keyGroup, azkv.NewMasterKey(k.VaultURL, k.Key, k.Version))
+	}
+	for _, k := range group.Vault {
+		if masterKey, err := hcvault.NewMasterKeyFromURI(k); err == nil {
+			keyGroup = append(keyGroup, masterKey)
+		} else {
+			return nil, err
+		}
+	}
+	return deduplicateKeygroup(keyGroup), nil
+}
+
 func getKeyGroupsFromCreationRule(cRule *creationRule, kmsEncryptionContext map[string]*string) ([]sops.KeyGroup, error) {
 	var groups []sops.KeyGroup
 	if len(cRule.KeyGroups) > 0 {
 		for _, group := range cRule.KeyGroups {
-			var keyGroup sops.KeyGroup
-			for _, k := range group.Age {
-				keys, err := age.MasterKeysFromRecipients(k)
-				if err != nil {
-					return nil, err
-				}
-				for _, key := range keys {
-					keyGroup = append(keyGroup, key)
-				}
-			}
-			for _, k := range group.PGP {
-				keyGroup = append(keyGroup, pgp.NewMasterKeyFromFingerprint(k))
-			}
-			for _, k := range group.KMS {
-				keyGroup = append(keyGroup, kms.NewMasterKeyWithProfile(k.Arn, k.Role, k.Context, k.AwsProfile))
-			}
-			for _, k := range group.GCPKMS {
-				keyGroup = append(keyGroup, gcpkms.NewMasterKeyFromResourceID(k.ResourceID))
-			}
-			for _, k := range group.AzureKV {
-				keyGroup = append(keyGroup, azkv.NewMasterKey(k.VaultURL, k.Key, k.Version))
-			}
-			for _, k := range group.Vault {
-				if masterKey, err := hcvault.NewMasterKeyFromURI(k); err == nil {
-					keyGroup = append(keyGroup, masterKey)
-				} else {
-					return nil, err
-				}
+			keyGroup, err := extractMasterKeys(group)
+			if err != nil {
+				return nil, err
 			}
 			groups = append(groups, keyGroup)
 		}


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 857b50cb8..abf2c66a5 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -120,6 +120,125 @@ creation_rules:
       - 'https://baz.vault:8200/v1/baz/keys/baz-key'
 `)
 
+var sampleConfigWithMergeType = []byte(`
+creation_rules:
+  - path_regex: ""
+    key_groups:
+    # key00
+    - hc_vault:
+      - 'https://foo.vault:8200/v1/foo/keys/foo-key'
+    - merge:
+      - merge:
+        - kms:
+          # key01
+          - arn: foo
+            aws_profile: foo
+          pgp:
+          # key02
+          - foo
+          gcp_kms:
+          # key03
+          - resource_id: foo
+          azure_keyvault:
+          # key04
+          - vaultUrl: https://foo.vault.azure.net
+            key: foo-key
+            version: fooversion
+          hc_vault:
+          # key05
+          - 'https://bar.vault:8200/v1/bar/keys/bar-key'
+        - kms:
+          # key06
+          - arn: bar
+            aws_profile: bar
+          pgp:
+          # key07
+          - bar
+          gcp_kms:
+          # key08
+          - resource_id: bar
+          # key09
+          - resource_id: baz
+          azure_keyvault:
+          # key10
+          - vaultUrl: https://bar.vault.azure.net
+            key: bar-key
+            version: barversion
+          hc_vault:
+          # key01 - duplicate#1
+          - 'https://baz.vault:8200/v1/baz/keys/baz-key'
+        kms:
+        # key11
+        - arn: baz
+          aws_profile: baz
+        pgp:
+        # key12
+        - baz
+        gcp_kms:
+        # key03 - duplicate#2
+        # --> should be removed when loading config
+        - resource_id: bar
+        azure_keyvault:
+        # key04 - duplicate#3
+        - vaultUrl: https://foo.vault.azure.net
+          key: foo-key
+          version: fooversion
+        hc_vault:
+        # key13 - duplicate#4 - but from different key_group
+        # --> should stay
+        - 'https://foo.vault:8200/v1/foo/keys/foo-key'
+      - kms:
+        # key14
+        - arn: qux
+          aws_profile: qux
+        # key14 - duplicate#5
+        - arn: baz
+          aws_profile: bar
+        pgp:
+        # key15
+        - qux
+        gcp_kms:
+        # key16
+        - resource_id: qux
+        # key17
+        - resource_id: fnord
+        azure_keyvault:
+        # key18
+        - vaultUrl: https://baz.vault.azure.net
+          key: baz-key
+          version: bazversion
+        hc_vault:
+        # key19
+        - 'https://qux.vault:8200/v1/qux/keys/qux-key'
+      # everything below this should be loaded,
+      # since it is not in a merge block
+      kms:
+      # duplicated key06
+      - arn: bar
+        aws_profile: bar
+      # key20
+      - arn: fnord
+        aws_profile: fnord
+      pgp:
+      # duplicated key07
+      - bar
+      gcp_kms:
+      # duplicated key08
+      - resource_id: bar
+      # key21
+      - resource_id: fnord
+      azure_keyvault:
+      # duplicated key10
+      - vaultUrl: https://bar.vault.azure.net
+        key: bar-key
+        version: barversion
+      hc_vault:
+      # duplicated 'key01 - duplicate#2'
+      - 'https://baz.vault:8200/v1/baz/keys/baz-key'
+      # key22
+      - 'https://fnord.vault:8200/v1/fnord/keys/fnord-key'
+`)
+
 var sampleConfigWithSuffixParameters = []byte(`
 creation_rules:
   - path_regex: foobar*
@@ -340,6 +459,14 @@ func TestLoadConfigFileWithGroups(t *testing.T) {
 	assert.Equal(t, expected, conf)
 }
 
+func TestLoadConfigFileWithMerge(t *testing.T) {
+	conf, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithMergeType, t), "/conf/path", "whatever", nil)
+	assert.Nil(t, err)
+	assert.Equal(t, 2, len(conf.KeyGroups))
+	assert.Equal(t, 1, len(conf.KeyGroups[0]))
+	assert.Equal(t, 22, len(conf.KeyGroups[1]))
+}
+
 func TestLoadConfigFileWithNoMatchingRules(t *testing.T) {
 	_, err := parseCreationRuleForFile(parseConfigFile(sampleConfigWithNoMatchingRules, t), "/conf/path", "foobar2000", nil)
 	assert.NotNil(t, err)