Feature: merge key for key groups and *make keys unique*

[jonasbadstuebner]

This closes #1123

The test is not very great (as in: "It doesn't check very much"), but since it was not too hard to implement, I think this is okay.

If you have any comments, please let me know.

[jonasbadstuebner]

Just so I have said it: This does not ensure that a key is included only once. Maybe this is a desirable feature when people start merging key_groups now?

But that is not any different from the overall key_groups behavior. And as far as I can tell, sops can handle it if there are multiple recipients matching. (I think it just takes the first matching one, I haven't checked this any futher)

[jonasbadstuebner]

Should also mention:
Fixes #878

[jonasbadstuebner]

Would be nice to get an ETA of when this will be looked at and maybe an ETA for a new patch release that includes this feature?

[jonasbadstuebner]

@felixfontein Sorry to bother you again, but what is the current state of sops, it seems to be very slow moving and the last patch release has been a long time ago.

Why does a small MR like this take so much time to review and merge and when is the next patch release planned? 3.9.0 is on the road map for a very long time already, do you need help or is it something else?

[felixfontein]

It seems that most maintainers currently have no time to review PRs. Since I don't want to merge PRs that I don't fully understand without a second review, this means that rarely any PRs get merged currently.

I was hoping for a soonish 3.9.0 release for some time already, I have no idea when it will happen.

[jonasbadstuebner]

If you need maintainers, I would be happy to support sops. Who would be in charge of deciding this and what would I have to do?

[felixfontein]

I'm not sure how the process works, but joining the #sops-dev channel on the CNCF Slack (https://github.com/getsops/sops/blob/main/CONTRIBUTING.md#communication, the second link allows you to get an invite for the CNCF Slack, and the former then allows you to join that channel) is probably a good idea.

[felixfontein]

Some thoughts about recursion ;)

Just so I have said it: This does not ensure that a key is included only once. Maybe this is a desirable feature when people start merging key_groups now?

I tend to say yes. We definitely have to make sure to avoid breaking backwards compatibility, though. Maybe it should only be used for everything inside merge, but not for the ones added on top-level? I.e. remove duplicates in line 193, after processing group.Merge and before processing group.Age.

[jonasbadstuebner]

Some thoughts about recursion ;)

Just so I have said it: This does not ensure that a key is included only once. Maybe this is a desirable feature when people start merging key_groups now?

I tend to say yes. We definitely have to make sure to avoid breaking backwards compatibility, though. Maybe it should only be used for everything inside merge, but not for the ones added on top-level? I.e. remove duplicates in line 193, after processing group.Merge and before processing group.Age.

This implementation would allow

key_groups:
- key1
  merge:
  - key1

Which would result in duplicated key1. Is this the desired behavior?

[jonasbadstuebner]

I implemented the uniqueness as requested. The top-level is not touched.

[felixfontein]

Why not use v.ToString() instead of a string representation of v.ToMap()?

The only problem I see with that is that kms.ToString() only uses arn and not role, context, and awsProfile. I have no idea whether that's OK or not... From https://docs.aws.amazon.com/kms/latest/developerguide/concepts.html#key-id it seems that the ARN uniquely identifies the key.

Does anyone from @getsops/maintainers have some insight here?

[jonasbadstuebner]

This difference on (at least) the kms keys is the reason.

I found this other place, where keyGroups are compared to each other:

sops/cmd/sops/common/common.go

Line 401 in 1c46d24

func DiffKeyGroups(ours, theirs []sops.KeyGroup) []Diff {

and here ToString is used:

sops/cmd/sops/common/common.go

Line 423 in 1c46d24

theirKeys[key.ToString()] = struct{}{}

To have the same behavior, I have changed my code to use ToString too.

[felixfontein]

This implementation would allow

key_groups:
- key1
  merge:
  - key1

Which would result in duplicated key1. Is this the desired behavior?

I wouldn't say "desired", but I think it's a good compromise between backwards compatibility (you were able to duplicate keys before) and not deduplicating at all.

Another possibility would be to deduplicate everything once group.Merge contains something. In that case no deduplication is done for backwards compatibility if the merge feature isn't used, but once it is used the result is always deduplicated.

[jonasbadstuebner]

To me, changing the behavior of the top-level based on a key being present would be unintuitive.
Does removing duplicates on the top-level really break backwards compatibility though? If you have 2 keys in this list that are basically the same, it should not break your en- or decryption to only have the key included once. But I did not test this very deeply.

[felixfontein]

Having thought some more about this, I tend to agree that removing duplication in generally should be OK.

It's mainly breaking in the sense that if you re-encrypt a file that has duplicated keys (like when editing the encrypted content), suddenly some keys are removed, which can be quite confusing for users. But as long as we explicitly announce deduplication in the changelog, I think it should be OK.

(I would also mention it in the PR's title, so it shows up prominently in the git commit log.)

[felixfontein]

Looks good to me. Let's wait a bit more whether someone comments on #1493 (comment).