Skip to content

Distinguish credential unavailability from failure#9372

Merged
chlowell merged 6 commits intoAzure:masterfrom
chlowell:credential-unavailable-error
Feb 7, 2020
Merged

Distinguish credential unavailability from failure#9372
chlowell merged 6 commits intoAzure:masterfrom
chlowell:credential-unavailable-error

Conversation

@chlowell
Copy link
Copy Markdown
Member

@chlowell chlowell commented Jan 8, 2020

This adds CredentialUnavailableError to indicate a credential didn't attempt to authenticate because it lacks required data or state. For example, EnvironmentCredential raises it when environment configuration is incomplete, and SharedTokenCacheCredential raises it when the shared cache isn't present. Credentials previously raised ClientAuthenticationError in such cases. That exception now indicates a credential attempted to authenticate but failed due to an unexpected error.

ChainedTokenCredential (and DefaultAzureCredential) uses this to make authentication more predictable. When one of its credentials raises, ChainedTokenCredential only tries the next credential when the raised exception is CredentialUnavailableError. This prevents unexpected authentication. For example, if environment variables specify a service principal with an invalid secret, DefaultAzureCredential won't continue on to managed identity after authenticating as that principal fails.

Closes #8166

@chlowell chlowell added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Jan 8, 2020
@chlowell chlowell requested a review from schaabs as a code owner January 8, 2020 21:44
@chlowell chlowell self-assigned this Jan 8, 2020
bryevdv
bryevdv previously approved these changes Jan 8, 2020
Copy link
Copy Markdown
Contributor

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

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

Great tests, only one small suggestion

Comment thread sdk/identity/azure-identity/azure/identity/_credentials/environment.py Outdated
bryevdv
bryevdv previously approved these changes Jan 8, 2020
Comment thread sdk/identity/azure-identity/azure/identity/aio/_credentials/chained.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CredentialUnavailableError needs to be a sub-class of ClientAuthenticationError. Or it will be a breaking change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ClientAuthenticationError inherits HttpResponseError, which is misleading as a base here because CredentialUnavailableError is raised when no request was sent.

I agree changing the hierarchy is a larger issue. For this PR I'll have CredentialUnavailableError inherit ClientAuthenticationError, and we can revisit the hierarchy later.

Copy link
Copy Markdown
Member

@xiangyan99 xiangyan99 left a comment

Choose a reason for hiding this comment

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

CredentialUnavailableError needs to be a sub-class of ClientAuthenticationError. Or it will be a breaking change.

@chlowell chlowell force-pushed the credential-unavailable-error branch from e186bb5 to ff5fdfb Compare January 14, 2020 22:43
@chlowell chlowell requested a review from xiangyan99 January 21, 2020 20:55
@chlowell chlowell requested a review from johanste February 3, 2020 21:55
@chlowell chlowell force-pushed the credential-unavailable-error branch from ff5fdfb to a3da347 Compare February 6, 2020 23:01
xiangyan99
xiangyan99 previously approved these changes Feb 6, 2020
@chlowell chlowell merged commit f5d602a into Azure:master Feb 7, 2020
@chlowell chlowell deleted the credential-unavailable-error branch February 7, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Azure.Identity Client This issue points to a problem in the data-plane of the library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Credentials should distinguish misconfiguration from unavailability

3 participants