AZKV: Also allow to omit version for AZKV keys specified in key groups

[felixfontein]

Follow-up to #1919.

While working on #1946, I noticed that #1919 did not handle keys specified in key groups (which already have to be split up into their three parts).

CC @daogilvie @r4vi

[r4vi]

I think this might lead to unexpected behaviour you were worried about in the original PR.

Afaik the 3 part definition is used inside a sops encrypted file, and if you omit the version then it might lead to unexpected behaviour when there is a new key version available. You'd want the sops file to reference the exact version it was encrypted with.

While this PR won't by itself introduce that as a problem. I assume the code for creating a sops encrypted file will add all three parts by default.

A user might remove it thinking it'll be safe and everything will continue to work until they create a new azkv key version. After that point if someone tries to decrypt the sops file it will break.

[felixfontein]

I don't think this is relevant: when reading SOPS encrypted files, the metadata from the file is convert to a azkv.MasterKey using

sops/stores/stores.go

Lines 395 to 407 in d3bb109

	func (azkvKey *azkvkey) toInternal() (*azkv.MasterKey, error) {
	creationDate, err := time.Parse(time.RFC3339, azkvKey.CreatedAt)
	if err != nil {
	return nil, err
	}
	return &azkv.MasterKey{
	VaultURL: azkvKey.VaultURL,
	Name: azkvKey.Name,
	Version: azkvKey.Version,
	EncryptedKey: azkvKey.EncryptedDataKey,
	CreationDate: creationDate,
	}, nil
	}

, which does not call any of the helper functions in azkv/keysource.go (that would query the API). So if a user deletes the version from a SOPS encrypted file, SOPS will no longer be able to decrypt that file (assuming there are no other keys that can be used) since MasterKey.version is never filled in (resp. set to its default "").

[r4vi]

I trust your intuition more than mine, my assumption was solely based on where I've seen the 3 part definition rather than any familiarity with the code base

[daogilvie]

Oh great spot, yes I didn't even think about testing key groups. However, wouldn't this line already cover this case?

[felixfontein]

Oh great spot, yes I didn't even think about testing key groups. However, wouldn't this line already cover this case?

That line covers AZKV keys outside key groups :) Key groups are handled in

sops/config/config.go

Lines 355 to 363 in 286dffc

	if len(cRule.KeyGroups) > 0 {
	for _, group := range cRule.KeyGroups {
	keyGroup, err := extractMasterKeys(group)
	if err != nil {
	return nil, err
	}
	groups = append(groups, keyGroup)
	}
	} else {

further up in this function. This else branch handles the case that key_groups has not been specified (or is empty), and uses the other fields to compose a single key group.

[daogilvie]

Oh great spot, yes I didn't even think about testing key groups. However, wouldn't this line already cover this case?

That line covers AZKV keys outside key groups :) Key groups are handled in

sops/config/config.go

Lines 355 to 363 in 286dffc

	if len(cRule.KeyGroups) > 0 {
	for _, group := range cRule.KeyGroups {
	keyGroup, err := extractMasterKeys(group)
	if err != nil {
	return nil, err
	}
	groups = append(groups, keyGroup)
	}
	} else {

further up in this function. This else branch handles the case that key_groups has not been specified (or is empty), and uses the other fields to compose a single key group.

Ah, thank you — Sorry, go is not my daily driver and I found reading that function a little confusing. So, to be clear:

1. Any key in an AZKV KeyGroup MUST currently be already broken into the 3 constituent values, not the URL form
2. This PR just means that if the Version field is a blank string, that particular keygroup key will be resolved to the latest version as per what we did for the URL handling

This PR itself is not putting URLs into the KeyGroup format, right? I looked at your docs PR, and here you have written it as though URLs will work, but I don't see how that is the case 🤔 It looks like the only thing doing any parsing before the bit you've changed is the YAML unmarshal, and that won't allow raw URLs because the azkv struct is a map of 3 strings... I think? I really want to like Go but I keep bouncing off the syntax for some reason 😂 sorry if I've missed something obvious again

[felixfontein]

Ah, thank you — Sorry, go is not my daily driver and I found reading that function a little confusing. So, to be clear:

No worries :) I'm (almost) only working with Go when working on SOPS, so I know that feeling very well (though I got more familiar with it over the years)...

1. Any key in an AZKV KeyGroup MUST currently be already broken into the 3 constituent values, not the URL form

Yes.

2. This PR just means that if the Version field is a blank string, that particular keygroup key will be resolved to the latest version as per what we did for the URL handling

Yes.

This PR itself is not putting URLs into the KeyGroup format, right?

No, it's not.

I looked at your docs PR, and here you have written it as though URLs will work, but I don't see how that is the case 🤔

Well, that example isn't using key groups (key_groups is missing - see https://github.com/getsops/sops/pull/1946/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168fR1798-R1826 for an example), but defines a single key group by not using key_groups (this is the most common form to provide keys, and very likely was there before key groups were added). In that form, you always have to provide URLs.

But for key groups (directly, not with the "only one key-group" "hack"/backwards compatibility thing), you always have to supply 2-3 fields (it was all three until this PR, now with this PR you can leave the version away). So basically this PR gives you feature parity between the backwards compatible way (and the most common way keys are provided) and the key_groups way (which you need if you want to use Shamir's secret sharing, or if you want to use merge groups with YAML anchors).

It looks like the only thing doing any parsing before the bit you've changed is the YAML unmarshal, and that won't allow raw URLs because the azkv struct is a map of 3 strings... I think?

Yes, that's correct. You always have to provide the three fields (or now also two, as you can leave version empty with this PR). But that's only the case for inside key_groups, and for destination rules (which are for publishing, I think).

I really want to like Go but I keep bouncing off the syntax for some reason 😂 sorry if I've missed something obvious again

This is probably more a confusion with SOPS, since it defines some of these structures multiple times (for multiple situations: config file syntax, where you have two forms to provide keys depending on where you provide them; SOPS file metadata; and the actual AZKV keystore code). Almost all of these forms have the three fields (except the URL one outside of key_groups in creation rules), and only some of them allow version to be empty (namely in the config file; in the AZKV keystore code and in the SOPS metadata version has to be filled out). It also confused me, and when working through all data structures to compile the information for #1946 it at least got clearer to me :)

[daogilvie]

Thank you for such a thorough response! No further questions from me; this makes sense. I'll leave an approval, for whatever that is worth.

[daogilvie]

Thanks for adding this case!

[hiddeco]

Two minor nits, but other than this it LGTM.

[hiddeco]

Wondering if instead of introducing a breaking change — we should introduce a NewMasterKeyWithVersion.

[felixfontein]

Technically no since the only public Go part of SOPS is the decrypt package, but I've changed it back in 51c97fa. I've decided to keep the private (newMasterKey) and public (NewMasterKey) function, which currently do both the same though.

[hiddeco]

Typically, you would return nil on error.

Suggested change

	err := key.ensureKeyHasVersion(context.Background())
	return key, err
	if err := key.ensureKeyHasVersion(context.Background()); err != nil {
	return nil, err
	}
	return key, nil

[felixfontein]

Fixed in 51c97fa.

[felixfontein]

@hiddeco thanks again!

[felixfontein]

@r4vi @daogilvie also thanks a lot for testing and reviewing this, and for your work in #1919!