feat(azkv): Skipping key-version will get latest key

[daogilvie]

Firstly: thanks for maintaining such a great tool! It's made my life a lot easier.

Hopefully this PR is helpful in some way; I checked open/closed issues and PRs for something similar being rejected, and couldn't see it.

Anyway, what's the change? I've edited the AZKV implementation to support not including a version in the URL. I.e this makes https://${VAULT_URL}/keys/${KEY_NAME}/ a valid AZKV URL.

This is a supported behaviour for Azure Key Vault, and is very helpful when it comes to doing rotation of those keys. Updated versions will automatically get fetched by sops for new files with no changes to the config in the repo. This brings AZKV in line with sops' AWS/GCP integrations, which do not require you to specify a key version.

EDIT adding clarification after @felixfontein's comment:
If you don't specify a version in the URL, then sops will use the azkey client (already bundled, of course), to retrieve the specific version of the key, and puts that into the struct instead. I.e even with no version supplied, the NewMasterKeyFromURL function always returns a version, so any files edited/encrypted will still have the specific version stored in the metadata.

Note

I am not sure how to test this in the automated test suites, as I can't see any comparable test in azkv/keysource_test.go that would mock or otherwise inject the az client.
I am more than happy to be told this is not something sops should even support, or that I need to redo some or all of the work.
I don't really use golang much day-to-day, so I may be completely missing out on some sensible approach or pattern.

Fixes #1924

Manual Tests

When no version set in URL, SOPS...

1. SHOULD fetch latest version when encrypting a file, and save the version in the file ✅ (seems to parse the URL multiple times though?)
2. SHOULD NOT fetch the latest version when editing the file ✅
3. SHOULD NOT fetch the latest version when decrypting a file ✅
4. SHOULD fetch the latest and update the file when using updatekeys ✅

When a version is provided in the URL, SOPS...

1. SHOULD NOT fetch it via the client ✅

[felixfontein]

Not sure whether I understand this correctly, but isn't always using the latest version not a bad idea? If you encrypt a file with a specific key, and then later try to decrypt after the key has been rotated, with your change SOPS would try to use the latest key to decrypt, wouldn't that not work, because it needs the previous one?

So I guess it would be OK if .sops.yaml can skip the version, but once a file is encrypted, the key including the version used for encryption must be stored in the encrypted file?

[daogilvie]

Not sure whether I understand this correctly, but isn't always using the latest version not a bad idea? If you encrypt a file with a specific key, and then later try to decrypt after the key has been rotated, with your change SOPS would try to use the latest key to decrypt, wouldn't that not work, because it needs the previous one?

So I guess it would be OK if .sops.yaml can skip the version, but once a file is encrypted, the key including the version used for encryption must be stored in the encrypted file?

Thanks for having a look @felixfontein! Sorry — I should have been clearer.

Most of the lines of code added here are actually to correctly identify the version of the key and store that in the metadata in the yaml of the file. sops edit my.sops.yaml for example, will have the version in the metadata correctly, so subsequent sops decrypt or sops edit calls work.
I tested this by editing a file into existence, altering .sops.yaml to refer to a different key, and checking the decrypt/edit command still works.

It is entirely possible that my limited understanding of sops means I've broken other use-cases though.

[felixfontein]

Ah, that's good to hear! I think that makes sense.

One thing I'm worrying about is that retrieving the version might be done too often, i.e. not only when it's actually needed. Did you try adding debug statements which log retrieval and check what happens during various kind of operations (encrypt, decrypt, edit, update keys, ...)?

Also did you test whether updatekeys works (I would expect it to update the key's version)?

[felixfontein]

Generally, you need to sign-off your commits (to accept the DCO), otherwise we cannot merge this. (On the command line, you can do git commit --sign-off --amend to modify the commit, and then force-push.)

[daogilvie]

Ah, that's good to hear! I think that makes sense.

One thing I'm worrying about is that retrieving the version might be done too often, i.e. not only when it's actually needed. Did you try adding debug statements which log retrieval and check what happens during various kind of operations (encrypt, decrypt, edit, update keys, ...)?

I did not, no! That is a very fair question. "When it's actually needed" here being when we are encrypting/decrypting and the version cannot be sourced from anywhere else? I.e it's not in the url, it's not in the metadata somewhere?

Also did you test whether updatekeys works (I would expect it to update the key's version)?

That is also something I did not test, no — a big oversight on my part. I'll do that

I did this PR as a side-effect of a rush to get something done at work; I will aim to do this testing this week. After that I will be changing jobs and won't have the pleasure of dealing with Azure any more, thus losing access to my testing environment. I'll redo the commits with sign-off then. Thank you!

[felixfontein]

Ah, that's good to hear! I think that makes sense.
One thing I'm worrying about is that retrieving the version might be done too often, i.e. not only when it's actually needed. Did you try adding debug statements which log retrieval and check what happens during various kind of operations (encrypt, decrypt, edit, update keys, ...)?

I did not, no! That is a very fair question. "When it's actually needed" here being when we are encrypting/decrypting and the version cannot be sourced from anywhere else? I.e it's not in the url, it's not in the metadata somewhere?

It should only query the version when encrypting a new file (not when editing), or when running updatekey (and then it should replace older versions by the latest version). If it queries the latest version more than once in these cases, that would be OK I guess (at least for now), but if it also queries the version in other situations (like when decrypting) this would be bad. (The encrypted file should always contain AZKV URLs with specific versions, namely the version which can be used to decrypt the key.)

Also did you test whether updatekeys works (I would expect it to update the key's version)?

That is also something I did not test, no — a big oversight on my part. I'll do that

Thanks! As I wrote above, the expected behavior would be that it replaces the old version with the latest version of the AZKV URL.

(I think this should 'just work' with your PR, but without testing it... who knows :) )

I did this PR as a side-effect of a rush to get something done at work; I will aim to do this testing this week.

That would be great!

After that I will be changing jobs and won't have the pleasure of dealing with Azure any more, thus losing access to my testing environment.

Hehe, sounds good :) I'm lucky that I don't have to use Azure except for AZP in another open source project (and honestly I find the UI/UX there so bad that I'm really wondering why anyone pays money to use that).

I'll redo the commits with sign-off then. Thank you!

Thank you for your contribution!

[daogilvie]

Ok, I've manually tested the behaviours @felixfontein asked for, and it all looks good.
It does seem that the .sops.yaml URL is parsed more than once before the file is actually encrypted, and so the version is fetched each time that happens, but once it's saved in the file then sops only ever fetches the version again when doing updatekeys, so that seems to be good. Commit is squashed, rebased and signed off; hopefully ready for code review!
@felixfontein — I altered the behaviour of the URL parsing function slightly from when you last looked, to make it so users can omit the trailing slash in version-less URLs.

[r4vi]

@felixfontein if you need someone to keep testing stuff on Azure with regards to this PR - just @ me. I work[ed] with @daogilvie and I still have the misfortune of having access to Azure

[felixfontein]

Regarding the config being loaded multiple times: I'm trying to fix that in #1939.

Besides the comment, I think this looks good!

[felixfontein]

I'm going to merge this and then rebase #1939; @r4vi would be great if you could test whether that PR/branch reduces the number of ensureKeyHasVersion() calls per key to one.

[felixfontein]

@daogilvie thanks for your contribution!
@r4vi thanks for helping!

[maonat]

@felixfontein when is the next release planned to include this feature?