Sort masterkeys according to decryption-order #1345

bkreitch · 2023-11-07T10:42:35Z

This is more generic way to address #305. As suggested in #305 (comment) and #305 (comment) it supports --decryption-order and SOPS_DECRYPTION_ORDER to specify the order in which decryption method are tried. Default order is age,pgp, so it covers functionality of #1121 too.

Couple of additional thoughts:

Sorting just indices as suggested in Sort masterkeys to try offline decrypt methods first #1121 (comment) together with priorities makes code a bit cryptic. Maybe using copy() to clone the group slice and sort the cloned slice could be acceptable alternative (since we don't modify the elements, but only the order)?
Passing DecryptionOrder through number of options and function calls is a bit cumbersome. Would it make sense to include DecryptionOrder and KeyServices together in a struct since they seem to be used mostly together?

cmd/sops/codes/codes.go

cmd/sops/main.go

keyservice/keyservice.go

felixfontein

Looks good on a very first glance!

sops_test.go

felixfontein · 2023-11-13T20:02:23Z

The failing sanity test will get fixed by #1350.

It's probably best to remove the changes from go.mod and go.sum to avoid conflicts.

bkreitch · 2023-11-13T20:12:44Z

It's probably best to remove the changes from go.mod and go.sum to avoid conflicts.

@felixfontein Ok, sure, removed.

bkreitch · 2023-11-14T17:19:22Z

It's probably best to remove the changes from go.mod and go.sum to avoid conflicts.

@felixfontein Looks like tests won't pass without committed changes to go.mod and go.sum

felixfontein

I think this looks good! @getsops/maintainers what do you think?

cmd/sops/codes/codes.go

felixfontein · 2023-11-23T20:14:38Z

It's probably best to remove the changes from go.mod and go.sum to avoid conflicts.

@felixfontein Looks like tests won't pass without committed changes to go.mod and go.sum

I'm really confused why these are needed. 😕

bkreitch · 2023-11-23T20:48:36Z

It's probably best to remove the changes from go.mod and go.sum to avoid conflicts.

@felixfontein Looks like tests won't pass without committed changes to go.mod and go.sum

I'm really confused why these are needed. 😕

@felixfontein For now I've just added a small function instead of importing "golang.org/x/exp/slices". Later with Go 1.21 it could be replaced with build-in "slices" package.

Ph0tonic

Juste a small misspell

README.rst

age/keysource.go

azkv/keysource.go

cmd/sops/common/common.go

gcpkms/keysource.go

hcvault/keysource.go

kms/keysource.go

pgp/keysource.go

hiddeco · 2023-12-15T21:57:39Z

sops.go

+var (
+	MasterKeyTypes             = []string{"pgp", "gcp_kms", "hc_vault", "kms", "azure_kv", "age"}
+	SopsDecryptionOrderDefault = []string{"age", "pgp"}
+)


Am really on the fence about this implementation detail. The problem I have with this introduction is that it worsens the scattering of string identifiers throughout the code base, and can easily be forgotten to be updated when a new key source is introduced.

The first solution to mitigate this that comes to mind would be to start making use of the identifier constants exposed in the key source implementation (as I suggested above), but this would cause the abstraction to further leak details as we would now have to import every key implementation within this module. Which isn't a great solution either.

Which makes me think we should either:

Factor this out (but I have no suggestion on how to do this yet...).

As a short-term solution, not maintain a list of all key types, but rather only sort based on what's provided (which means we can either rely on our own defaults with a limited set of identifiers, or rely on the user their input).

@hiddeco I agree, this list is a bit problematic. I've updated the sort to rely only on provided list.

felixfontein · 2023-12-16T20:36:05Z

I think everything's good now (as good as it can get without solving the big issues @hiddeco mentioned, but these are out of scope for this PR). Except that now there's a conflict since I merged another PR. Sorry for that :( Should be pretty easy to resolve though...

cmd/sops/main.go

gcpkms/keysource.go

Co-authored-by: Gabriel Martinez <19713226+GMartinez-Sisti@users.noreply.github.com> Co-authored-by: Felix Fontein <felix@fontein.de> Co-authored-by: Bastien Wermeille <bastien.wermeille@gmail.com> Co-authored-by: Hidde Beydals <hiddeco@users.noreply.github.com> Signed-off-by: Boris Kreitchman <bkreitch@gmail.com>

hiddeco

Thank you very much for your contribution @bkreitch. Made some small changes after looking at it for a 4th time, please let me know if you agree with them and I will merge 🙇

bkreitch · 2023-12-18T08:28:01Z

Thank you very much for your contribution @bkreitch. Made some small changes after looking at it for a 4th time, please let me know if you agree with them and I will merge 🙇

@hiddeco Yep, sure, thanks :)

felixfontein · 2023-12-18T17:47:08Z

Thanks to everyone involved getting this ready and merged :)

bkreitch force-pushed the sort-decrypt-methods branch 9 times, most recently from cba9b72 to 0860e9d Compare November 8, 2023 08:45

bkreitch marked this pull request as ready for review November 8, 2023 09:12

bkreitch mentioned this pull request Nov 8, 2023

Sort masterkeys to try offline decrypt methods first #1121

Closed

GMartinez-Sisti reviewed Nov 8, 2023

View reviewed changes

cmd/sops/codes/codes.go Outdated Show resolved Hide resolved

GMartinez-Sisti reviewed Nov 8, 2023

View reviewed changes

cmd/sops/main.go Outdated Show resolved Hide resolved

GMartinez-Sisti reviewed Nov 8, 2023

View reviewed changes

keyservice/keyservice.go Outdated Show resolved Hide resolved

bkreitch force-pushed the sort-decrypt-methods branch from cef900d to 4a3ebf4 Compare November 8, 2023 12:32

felixfontein reviewed Nov 12, 2023

View reviewed changes

sops_test.go Outdated Show resolved Hide resolved

bkreitch force-pushed the sort-decrypt-methods branch 2 times, most recently from 6e216ff to 896113e Compare November 13, 2023 07:09

bkreitch force-pushed the sort-decrypt-methods branch from 896113e to 5009c98 Compare November 13, 2023 20:10

bkreitch force-pushed the sort-decrypt-methods branch from 5009c98 to c829df7 Compare November 22, 2023 08:09

felixfontein reviewed Nov 23, 2023

View reviewed changes

cmd/sops/codes/codes.go Outdated Show resolved Hide resolved

bkreitch force-pushed the sort-decrypt-methods branch from 643c54c to 27dfadf Compare November 23, 2023 20:41

felixfontein approved these changes Nov 24, 2023

View reviewed changes

Ph0tonic suggested changes Nov 24, 2023

View reviewed changes

Ph0tonic reviewed Nov 24, 2023

View reviewed changes

README.rst Outdated Show resolved Hide resolved

Ph0tonic reviewed Nov 24, 2023

View reviewed changes

README.rst Outdated Show resolved Hide resolved

Ph0tonic reviewed Nov 24, 2023

View reviewed changes

README.rst Outdated Show resolved Hide resolved

bkreitch force-pushed the sort-decrypt-methods branch from 0a59dd8 to 33ce632 Compare December 1, 2023 06:28

hiddeco reviewed Dec 15, 2023

View reviewed changes

bkreitch force-pushed the sort-decrypt-methods branch 2 times, most recently from ec6de65 to 791ebe2 Compare December 16, 2023 12:02

bkreitch requested a review from hiddeco December 16, 2023 12:20

bkreitch force-pushed the sort-decrypt-methods branch from 791ebe2 to 4ec7e69 Compare December 17, 2023 17:49

hiddeco reviewed Dec 17, 2023

View reviewed changes

cmd/sops/main.go Outdated Show resolved Hide resolved

gcpkms/keysource.go Outdated Show resolved Hide resolved

bkreitch force-pushed the sort-decrypt-methods branch from a312a48 to 1bed9ea Compare December 17, 2023 22:08

bkreitch requested a review from hiddeco December 17, 2023 22:14

hiddeco force-pushed the sort-decrypt-methods branch from 1bed9ea to ba7d1a2 Compare December 18, 2023 07:05

hiddeco force-pushed the sort-decrypt-methods branch from ba7d1a2 to c822b55 Compare December 18, 2023 07:38

hiddeco approved these changes Dec 18, 2023

View reviewed changes

hiddeco merged commit a785bc9 into getsops:main Dec 18, 2023
10 checks passed

bkreitch deleted the sort-decrypt-methods branch December 18, 2023 09:29

darkowlzz mentioned this pull request Jul 30, 2024

Update SOPS to v3.9.0 fluxcd/kustomize-controller#1212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort masterkeys according to decryption-order #1345

Sort masterkeys according to decryption-order #1345

bkreitch commented Nov 7, 2023 •

edited

Loading

felixfontein left a comment

felixfontein commented Nov 13, 2023

bkreitch commented Nov 13, 2023

bkreitch commented Nov 14, 2023

felixfontein left a comment

felixfontein commented Nov 23, 2023

bkreitch commented Nov 23, 2023

Ph0tonic left a comment

hiddeco Dec 15, 2023

bkreitch Dec 16, 2023

felixfontein commented Dec 16, 2023

hiddeco left a comment

bkreitch commented Dec 18, 2023

felixfontein commented Dec 18, 2023

Sort masterkeys according to decryption-order #1345

Sort masterkeys according to decryption-order #1345

Conversation

bkreitch commented Nov 7, 2023 • edited Loading

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented Nov 13, 2023

bkreitch commented Nov 13, 2023

bkreitch commented Nov 14, 2023

felixfontein left a comment

Choose a reason for hiding this comment

felixfontein commented Nov 23, 2023

bkreitch commented Nov 23, 2023

Ph0tonic left a comment

Choose a reason for hiding this comment

hiddeco Dec 15, 2023

Choose a reason for hiding this comment

bkreitch Dec 16, 2023

Choose a reason for hiding this comment

felixfontein commented Dec 16, 2023

hiddeco left a comment

Choose a reason for hiding this comment

bkreitch commented Dec 18, 2023

felixfontein commented Dec 18, 2023

bkreitch commented Nov 7, 2023 •

edited

Loading