Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable environmental authentication for Azure Key Vault var provider #2678

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

devigned
Copy link
Contributor

This PR is related to #2566 in that it provides similar authentication behaviors for Azure Key Vault var provider. This change should be additive and not cause any breakage to existing applications.

It would probably be wise to follow on with some integration tests to guard against future breakage. I'm not clear on the project stance on spinning up cloud resources for testing, but if help is wanted, I'd be delighted to lend a hand.

To create the infra and test this, simply follow https://developer.fermyon.com/spin/v2/dynamic-configuration#azure-key-vault-application-variable-provider-example. You can skip over creating the service principal and instead use your current identity provided through Azure CLI to authenticate.

Runtime config should look like the following with ${your_vault_name] replaced:

[[config_provider]]
type = "azure_key_vault"
vault_url = "https://${your_vault_name}.vault.azure.net/"

On a side note, fetching secrets from Key Vault upon each request is pretty slow. Might want to consider some form of memoization / caching for a period of time.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a couple of non-blocking suggestions / questions, but otherwise LGTM. Thanks!

(And you're right about the caching. That's a cross-cutting concern though and definitely on the radar!)

crates/variables/src/provider/azure_key_vault.rs Outdated Show resolved Hide resolved

if any_none && any_some {
// some of the service principal auth options were specified, but not enough to authenticate.
return Err(anyhow!("Azure Key Vault provider requires each client_id, client_secret, and tenant_id to be provided. If none are provided, the provider will attempt to authenticate using ambient authentication (e.g. env vars, Azure CLI, Managed Identity, Workload Identity"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a go at wordsmithing this to focus more on "what is the problem and what is the remedy." I'm not super happy with the result but sharing it in the hope that it will inspire you to come up with the perfect wording...

Suggested change
return Err(anyhow!("Azure Key Vault provider requires each client_id, client_secret, and tenant_id to be provided. If none are provided, the provider will attempt to authenticate using ambient authentication (e.g. env vars, Azure CLI, Managed Identity, Workload Identity"));
return Err(anyhow!("The current runtime config specifies some but not all of the Azure KeyVault 'client_id', 'client_secret', and 'tenant_id' values. Provide the missing values to authenticate to Azure KeyVault with the given service principal, or remove all these values to authenticate using ambient authentication (e.g. env vars, Azure CLI, Managed Identity, Workload Identity)."));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yours is an improvement on what I had. I think it is clear, but might benefit from a link to auth docs when they are authored. I don't think any brief error message is going to provide the clarity needed to understand all of the options implied.

@devigned devigned force-pushed the az-key-vault branch 2 times, most recently from 9ac1baf to a467a52 Compare July 24, 2024 15:04
@rylev
Copy link
Collaborator

rylev commented Aug 14, 2024

@itowlson is this waiting on anything else to merge?

@squillace
Copy link

someone's vacation, I imagine. :-P

@itowlson
Copy link
Contributor

@squillace UNCANNY

@itowlson itowlson merged commit 10bfdfd into fermyon:main Aug 18, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants