Azure: GetTokenCredential optionally disable certain auth methods

[famarting]

Describe the feature

for all azure based components they use this function for authentication https://github.com/dapr/components-contrib/blob/master/internal/authentication/azure/auth.go#L116

this function tries several authentication methods sequentially and returns the token for the first auth method that succeeds... however there are certain usecases when a user or a system admin does not want to allow certain auth methods to even be tried... and with the current implementation on failure scenarios all auth methods will always be tried, which will produce noisy and confusing errors

this could be improved simply using environment variables, i.e DISABLE_AZURE_MI_AUTH to disable specific auth methods, so if DISABLE_AZURE_MI_AUTH is set to true the azure components would skip and not even try the azure managed identity auth method.

Release Note

RELEASE NOTE: ADD Options to disable adhoc azure auth methods

[guergabo]

Alternatively, the function could be made “smart” and based on the metadata fields the user passes it could identify what authentication method it is attempting and just try that always.

[famarting]

Alternatively, the function could be made “smart” and based on the metadata fields the user passes it could identify what authentication method it is attempting and just try that always.

that may be a breaking change depending on the usecase and the users currently setting unneeded metadata fields but that are able to successfully authenticate thanks to other auth methods being tried

[JoshVanL]

I don't think using environment variables are the right approach because it applies daprd wide rather than being scoped to a single component- which is much more inline with the multiple components to a single dapr model. As @guergabo suggests, I think this should be configured via a component metadata field. I'd expect a single field authMethods (or some similar name) which takes a comma separated list of named authentication methods which will be enabled for the component. An omitted field defaults to all authentication methods being enabled for backwards compatibility.

[famarting]

I don't think using environment variables are the right approach because it applies daprd wide rather than being scoped to a single component- which is much more inline with the multiple components to a single dapr model. As @guergabo suggests, I think this should be configured via a component metadata field. I'd expect a single field authMethods (or some similar name) which takes a comma separated list of named authentication methods which will be enabled for the component. An omitted field defaults to all authentication methods being enabled for backwards compatibility.

authMethods metadata property, empty or absent being all, works for me

[JoshVanL]

authMethods metadata property, empty or absent being all, works for me

I think empty string should mean no auth method is attempted.

[famarting]

authMethods metadata property, empty or absent being all, works for me

I think empty string should mean no auth method is attempted.

you need this to be backwards compatible

[JoshVanL]

you need this to be backwards compatible

It is, previous deployments will not be setting the authMethods field and unset != empty string so we are OK.

[ItalyPaleAle]

Yes I agree this is a good feature.

Also agree we should use a metadata property per-component.

Strongly disagree that "unset != empty". In our metadata we always consider empty to be the same as unset. Not doing so can cause unnecessary confusion as users need to be aware of the 3 possible states (unset, empty, set) rather than just 2 (empty or not empty).

Additionally, it makes no sense to attempt no auth method in this case.

So something like azureADMethods which can have values: client-credentials (includes certificates), mi (alias msi and managed-identity), cli. Multiple values can be passed as comma-separated (and we'll respect the order). Empty or unset preserves the current behavior.

This can be implemented in the shared azure auth package.

If you're up for it, PRs are welcome :)

[famarting]

would you accept using a more generic metadata option, such as authMethods (or authProfiles) so that other component implementations can adhere?

there is a problem across all components in regards to being able to automatically identify which auth profile is being used and a generic metadata such as authMethods could be a common denominator across all components in order to control what auth profile should be attempted.

i.e this is something that the kafka components already implement there is the common authType metadata property that determines which auth profile will be used.

Recognizing that the azure components have this problem and having the kafka component already address this in an elegant way, we could put in place a solution that works for all components

[ItalyPaleAle]

would you accept using a more generic metadata option, such as authMethods (or authProfiles) so that other component implementations can adhere?

there is a problem across all components in regards to being able to automatically identify which auth profile is being used and a generic metadata such as authMethods could be a common denominator across all components in order to control what auth profile should be attempted.

This is a very separate issue from the one that was discussed in the first message :)

I am not sure I agree with this statement:

there is a problem across all components in regards to being able to automatically identify which auth profile is being used

I would say that most components are fine right now, as they can determine the auth method to use depending on what metadata options are passed. For example, take the Cosmos DB state store: this can already detect if Azure AD is to be used depending on whether the "masterkey" is passed or not. Components like Kafka (and to a lesser extent, Postgres and SQL Server which have the "useAzureAD" boolean) are arguably the exception here. Although the current method is more implicit, we haven't heard feedback that it was a significant source of confusion for users so far. I am not sure an explicit method is strictly necessary.

This issue however is very different from what was the original issue opened here, which is that there is a legit need to control certain authentication methods used by Azure AD to retrieve a token. This is very specific to Azure AD and is for some advanced scenarios only.

Now, given this premise and going back to this:

a generic metadata such as authMethods could be a common denominator across all components in order to control what auth profile should be attempted.

I think this is something that should be discussed further, but possibly in a separate issue where all maintainers of this repo can chime in. One thing is adding a metadata property for Azure AD only, but if we are talking something generic that impacts all components, then I'd like to get the majority of maintainers on board with this first.

I think the first step would be to do an audit of what auth methods each component supports, and if there's more than one, how they are chosen.

[famarting]

I agree my last comment is deviated from the original reason of this issue, I was testing the waters 😄

Anyway, to confirm with your proposal:

So something like azureADMethods which can have values: client-credentials (includes certificates), mi (alias msi and managed-identity), cli. Multiple values can be passed as comma-separated (and we'll respect the order). Empty or unset preserves the current behavior.

I agree this can be implemented, its quite localized and could be easy to do. If I find the time I can do it, or anyone else interested is free to jump in.

Lastly, in regards to finding a generic solution across components, I'll try to create an issue to see if it sparks some interest.

[berndverst]

So the only feature we are discussing here is:
Adding an optional configuration via component metadata which specifies the the list and order of Azure auth methods to try. If this is not set we will use the current behavior.

Whatever we do here, lets not change the default. I absolutely want to retain the behavior that Managed Identity Credential and Azure CLI credential will be tried by default.

@famarting, the maintainers that have authored this code are @ItalyPaleAle and I (@berndverst) with myself having made the most recent changes there. For anything related to Azure auth, only the two of us need to sign off on the plan (and in fact other maintainers should not make decisions for the Azure experience).

At a high level the code would be something like the following:

• inspect metadata and look for azureAuthMethods
  • split this string (by comma) into multiple values
  • if any of the values are not known constants, throw an error
• if azureADmethods go to current code using the ChainedTokenCredential as is
• if methods were set, manually construct the ChainedTokenCredential, appending the auth methods in order.

Please do not change any of the details for how any given auth method works. Only connect the ordering of operations to the creation of the ChainedTokenCredential.

As the code states, the ChainedTokenCredential is what actually executes the auth methods in order.

components-contrib/internal/authentication/azure/auth.go

Lines 202 to 205 in 20a46e6

	// The ChainedTokenCredential executes the auth methods (with their given timeouts) in order. No execution occurs before this method.
	// As such there is no option to run the auth methods in parallel.
	// It would be ideal if the Azure SDK for Go team could support a parallel execution option.
	return azidentity.NewChainedTokenCredential(creds, nil)

Thanks!

@ItalyPaleAle I'd called it generically azureAuthMethods to avoid this whole Azure AD vs Entra ID naming :D

[berndverst]

you need this to be backwards compatible

It is, previous deployments will not be setting the authMethods field and unset != empty string so we are OK.

While we can distinguish between empty and unset with a string pointer in the metadata, I agree with @ItalyPaleAle that this is confusing.

Let's just use the special words none, or nil or similar to indicate this is disallowed.

[berndverst]

I'll tackle this tomorrow. Should be super quick. Unless you have started looking into it @famarting :)

[famarting]

I haven't started, thanks @berndverst

[berndverst]

I'm done with the PR. @famarting see #3217