feat: Add support for Azure Key Vault secrets.

[catay]

No description provided.

[halostatue]

This seems like a reasonably good start.

You should add some tests for this, but I’m not entirely sure how you would do so because the closest analogue to what you’ve implemented is pkg/cmd/awssecretsmanagertemplatefuncs.go which also does not have tests.

[twpayne]

You should add some tests for this, but I’m not entirely sure how you would do so because the closest analogue to what you’ve implemented is pkg/cmd/awssecretsmanagertemplatefuncs.go which also does not have tests.

I think it's OK to skip tests for this, as the functions are straightforward and it's not easy to mock out the Azure service.

[halostatue]

I have things that I would like to see, but do not think that we need to have for a first version of this.

I think we can merge this tomorrow afternoon if you don’t make the changes I have suggested for multiple vaults, because there’s nothing wrong with single vault support.

[catay]

Thanks. Give me until the end of the week to look into the multiple vault support.
The main complexity I see is with the template function and optional parameters but not impossible.

[halostatue]

Thanks. Give me until the end of the week to look into the multiple vault support.

I’ll ping this on Friday afternoon if you haven’t update this before then with the plan of merging it Friday afternoon. If you prefer, we can merge this and add on the multiple vault support later.

Like I said, nice (because I’m a bit of a completionist for this sort of thing), but not necessary.

[twpayne]

Could I request a change to the structure? Instead of:

azureKeyVault:
  vaultUrls:
    default: https://my-default-key-vault-url/
    sekrit_project: https://my-sekrit-project-vault-url/

Could you make it:

azureKeyVault:
  vaults:
  - url: https://my-default-key-vault-url/
  - name: sekrit_project
    url: https://my-sekrit-project-vault-url/

name defaults to the empty string, and if no vault is specified in the call to the azureKeyVault template function, then the first vault with an empty name should be used.

For handling optional arguments to template functions, see for, example how they are handled in the include template function.

[catay]

Thanks for the input. Will implement like you suggested.
I was playing with the same approach for the structure and was also looking into using a variadic function for optional parameters.

[catay]

After implementing the approach discussed I believe there is more elegant way on how to implement this.
Taking into account the earlier feedback from @halostatue. See below for the rational.

Note:

• I only committed the code for now, if considered ok i'll fix the documentation tomorrow.
  • 2950bfe option 1 with the initially proposal.
  • 355a435 with this proposal.
• Should I rebase to a single commit & force push or will you squash merge to master?

An Azure Key Vault comes with a globally unique hostname under the Microsoft owned domain vault.azure.net.

The hostname is the name of the resource in the Azure portal and cannot be created if not globally unique.

Examples:

kv-weu-t-sec-01.vault.azure.net
kv-random-name-01.vault.azure.net

This means we have enough with only hostname / vault name to construct the URL.

The proposal is using the below structure in the config.

azureKeyVault:
  vaults:
  - name: my-vault-01
  - name: my-vault-02
    alias: vault2

The parameters for the template function would look like this:

{{ azureKeyVault "my-secret-name" ["vault-name" | "vault-alias"] }}

Example of different template function implementations.

A secret {{ azureKeyVault my-secret }} using default my-vault-01 (no alias set).
A secret {{ azureKeyVault my-secret vault2 }} using my-vault-02.
A secret {{ azureKeyVault my-secret my-vault-03 }} using my-vault-03 vault. 

The name is the hostname of the vault, the alias is a custom ref that can be used in the template function. If the alias is not set, this will be the default vault in case no vault name or alias is provided in the template function.

If the alias is set in the template function it will lookup the correct vault name.
If the alias set in the template function is not found, it will assume it's a vault name.

[twpayne]

Thank you, your design is excellent. I really like it.

The main / test-website check failure is unrelated to this PR.

I think it's OK to remove the aliasing functionality as the vault names are short strings and the aliasing can be achieved with other template functions.

[halostatue]

I agree with @twpayne about the aliasing not being necessary since there’s a fixed name. I’ll put some comments in-line that I think could be used to simplify this.

[halostatue]

This looks amazing.

If we drop aliasing, we lose the value of having a list of defined vaults — and that’s OK, because you’ve confirmed that there’s one vault.azure.net base domain for all instances.

My recommendation is to change the exposed configuration to DefaultVault string (so azureKeyVault.defaultVault = "my-default-key-vault"), but mostly keep your azureKeyVault structure and functions because they do make most of it much easier.

I’m happy to make a follow-on PR if you’re wanting to be done with this originally simple change.

[catay]

The rational behind the alias was the following assuming you use multiple vaults:

Some users do not want to have the real vault name stored in version control and reference the alias defined in the configuration file. Security through obscurity basically.

Also trivial, but if you want migrate to another vault and use aliases in all your templates, you only need to change the name in the configuration.

But maybe that's just overkill. :)

If we decide to drop the aliases we probably also can simplify the structure back to this.

azureKeyVault:
  defaultVault: my-vault-02

We only have to specify a default and support for another vault would be the second parameter of the template function.

My preference is to stick with the layout containing the alias as it leaves more flexibility for later if you need to add an additional option for a specific vault.

[halostatue]

I have no strong feelings either way on this, and your rationale is sound about the flexibility:

My preference is to stick with the layout containing the alias as it leaves more flexibility for later if you need to add an additional option for a specific vault.

If, like with AWS, you are encouraged to have more than one Azure account or profile for separation of concerns, then I could see later adding a configuration option to identify the account or credential identifier. If this is possible, then we should keep the current structure even if we just drop alias support.

With respect to aliasing, it is possible to do that with data references:

azureKeyVault:
  defaultVault: my-default-vault

data:
  akv:
    innocuous: take-over-the-world

{{ azureKeyVault "system-password" .akv.innocuous }}

This is sort of what I do with some of my secrets, as home/private_dot_config/gh/private_dot_hosts.yml.tmpl shows.

[catay]

Please check out the latest commit.

The summary is that I basically removed the alias approach and simplified it more based on the provided feedback.
Also all the documentation is now updated to the latest design.

I think it's ready to get merged to main now.

Thanks for the really good feedback and patience.

[halostatue]

This is very good and you’ve been quite patient with a picky code review.

I have suggested a couple of small wording changes that aren’t urgent or required, but may improve the documentation / UX.

[twpayne]

Thank you very much for this excellent code @catay and for the thorough reviews @halostatue!

The main / test-website build error is mkdocs/mkdocs#3310 / fralau/mkdocs-mermaid2-plugin#80 and unrelated to this PR.

I'll merge this when the remaining checks are complete.

[catay]

Thanks.

I just pushed a PR for the workflow fix but it seems you beat me to it :)

[halostatue]

@catay I’m not sure what the "This branch cannot be rebased due to too many changes", but I suspect that if you were to rebase this and squash the various commits together, we can have one good commit that can be merged/rebase-merged cleanly.

[twpayne]

OK, #3120 is now merged. Would you mind rebasing on the latest master (not that I force-pushed to master to edit the commit message and re-order the *_VERSION environment variables) and squashing the commits in this PR into a single commit?

[catay]

Rebase branch on master done.