Allow Azure server node attestor and Key Vault plugins to fetch credentials using Workload Identity

[shashankram]

It's possible that SPIRE running in AKS needs to attest workloads in Azure. To fetch credentials to authenticate with the Azure API, two types of mechanisms are currently supported in SPIRE:

1. Client secret (static)
2. MSI

When the server is running in k8s, workload identity provides an elegant way to fetch API credentials. At a high level, this would allow the server to fetch API credentials using its k8s service account token, which would map to an identity in Azure to which IAM roles can be assigned.

It would make sense to extend the Azure server plugins to support this.
The core API to fetch a credential using workload identity is azidentity.NewWorkloadIdentityCredential, as documented.

Some options on enabling this:

1. no config change: use the credential chaining API in the Azure Go SDK with the use_msi plugin flag such that if a credential can be authenticated using azidentity.NewManagedIdentityCredential or azidentity.NewWorkloadIdentityCredential, then we use that credential. The API sequentially tries to authenticate using the given credentials until one succeeds.
2. no config change: use azidentity.NewDefaultAzureCredential which tries to fetch the credentials using all possible ways: Environment variables, workload identity, MSI, CLI credentials, in that order. This could be undesirable if we want to be strict on not allowing env vars.
3. new config flag: similar to use_msi, add a use_workload_identity config flag to the plugins and conditionally fetch the credential using azidentity.NewWorkloadIdentityCredential.

I am happy to help with this if it sounds reasonable.

[shashankram]

Was there a decision made on how to enable this?
If it's a completely new config, it may make sense to support multiple tenants to be authenticated based on the AdditionallyAllowedTenants config

[azdagron]

Thanks so much for the ping, @shashankram. That's an interesting feature. We'll huddle today and hopefully have a decision made in the next day or so on what shape of config we'd like to use. Appreciate your patience!

[azdagron]

Ok, @shashankram, I think we've arrived at consensus :) Right now, the plugin enforces that either you 1) use msi, or 2) provide static credentials, and fails to configure if you don't indicate either. The behavior we think makes the most sense is that, instead of failing if neither is configured, we change the plugin to use azidentity.NewDefaultAzureCredential.

In summary:

1. if use_msi is true, use azidentity.NewManagedIdentityCredential
2. if static creds are provided, use those creds
3. otherwise, use azidentity.NewDefaultAzureCredential

How does that feel to you?

[shashankram]

In summary:

1. if use_msi is true, use `azidentity.NewManagedIdentityCredential`

2. if static creds are provided, use those creds

3. otherwise, use `azidentity.NewDefaultAzureCredential`

How does that feel to you?

I think that's fine as an immediate solution, though we may need to revisit this for multi-tenant Az credentials via Workload Identity: AdditionallyAllowedTenants config.

[evan2645]

We'll almost certainly want to support that in the future, though it will probably require some changes the the shape of our config? Are you able to help with a proposal on what that might look like? In the meantime, this seems to take a step in the right direction without shaking too many things up ... happy to assign this issue to you if you're ok taking it

[shashankram]

We'll almost certainly want to support that in the future, though it will probably require some changes the the shape of our config? Are you able to help with a proposal on what that might look like? In the meantime, this seems to take a step in the right direction without shaking too many things up ... happy to assign this issue to you if you're ok taking it

That sounds good. We can start with a zero config solution for single tenants, and figure out the config structure for multi-tenant credential access. Feel free to assign this one to me, thanks.

[shashankram]

@azdagron @evan2645 I explored implementing a fallback to using the SDK default creds if client creds or use_msi=false, and it seems a specific behavior that is noted as needing to be deprecated, breaks.

Currently, this change breaks the test TestAttestSuccessWithNoClientCredentials because it expects the client to be nil and the code simply ignores resolving the selectors and returns an attestation response:

spire/pkg/server/plugin/nodeattestor/azuremsi/msi.go

Lines 297 to 300 in cb6ce22

	if client == nil {
	p.log.Warn("No client credentials available for tenant. Selectors will not be produced by the node attestor for this node. This will be an error in a future release.",
	"tenant", tenantID)
	}

spire/pkg/server/plugin/nodeattestor/azuremsi/msi.go

Line 206 in cb6ce22

if tenant.client != nil {

So with default creds, attestation will not succeed unless the resolution of selectors is successful. Are we okay with this behavior?

[evan2645]

I see ... thank you very much for raising this.

If nothing is set and we use azidentity.NewDefaultAzureCredential, can we detect if a credential was successfully loaded or not?

Since we need to do a deprecation now, I think we may have to assign this work to the 1.9.0 release. In which case, it may make sense for us to explore the larger changes around supporting multi-tenant auth

I've gone through a few possibilities in my head around how to do this while maintaining compat for a patch release, and am having a hard time. Given the current behavior, empty config seems equivalent to local validation w/o API checks. We can detect if we found a credential at configure time, but we probably won't know if it works until we try to use it?

[shashankram]

We could verify whether the default credentials work by calling GetToken() on it. If we get an error, we fallback to the current implementation of not resolving selectors. How does that sound? I haven't tested this though.

[shashankram]

We could verify whether the default credentials work by calling GetToken() on it. If we get an error, we fallback to the current implementation of not resolving selectors. How does that sound? I haven't tested this though.

Unfortunately this didn't work. GetToken() succeeded in the TestAttestSuccessWithNoClientCredentials test so there's no way to validate this till we actually try to resolve the selectors.

We have 2 options:

1. Deprecate the behavior to skip resolving selectors when we can't fetch credentials. This is already a TODO in the code.
2. Create a config to use multi-tenant auth with Azure workload identity. I envision this being very similar to the use_msi flag, so we could introduce a use_workload_identity flag for this.

[evan2645]

In light of the blocker/delay for the easy path, I think option 2 is probably best at this point .. then we can figure out after that how to right-size the config as a whole.

What config inputs are needed to support multi-tenant auth?

[shashankram]

I don't believe we would need additional config. Like MSI, we would create a client per tenant, and use that when the use_workload_identity flag is set. It would support multiple tenants based on the existing config structure just by instantiating a different client per tenant. This fits well with the existing config structure.

[evan2645]

Ah I see ... for some reason I was under the impression that we would need more than just a flag to enable it. Apologies

That seems super reasonable to me. Might also be a good idea to go ahead and start logging deprecation warnings for the case in which no config is set... default config should be the more secure thing, not the less secure. Then once those changes are in, we can decide the future of use_msi

[shashankram]

Using default cred will also imply the current behavior that has already been marked for deprecation will break as described in #4485 (comment).

I agree using default cred makes sense, we won't need separate flags for MSI vs workload identity, both will be tried implicitly.

[shashankram]

Draft PR for the changes #4568

[shashankram]

Resolved by #4568