Token exchange support for ManagedIdentityCredential #19902
Conversation
| # type: (**Any) -> None | ||
| super(TokenExchangeCredential, self).__init__() | ||
| self._available = all(os.environ.get(var) for var in EnvironmentVariables.TOKEN_EXCHANGE_VARS) | ||
| if self._available: |
There was a problem hiding this comment.
Do you think it would be valuable to do any logging in the case where the credential isn't (or is) available? Users will find out if it's unavailable when a token request is made, but EnvironmentCredential logs availability status (though that's part of the DefaultAzureCredential chain, which might have been the motivation to log there)
There was a problem hiding this comment.
Summarizing offline conversation: it's moot because ManagedIdentityCredential is the public API of this credential and already logs the interesting bit (what platform it believes it's running on) and will only instantiate this credential when its required environment variables are set. TokenExchangeCredential can therefore assume its prerequisites are met, and stop trying to raise a useful error message when that isn't the case. Commit to that effect forthcoming.
| return self | ||
|
|
||
| async def close(self) -> None: | ||
| await self._credential.close() |
There was a problem hiding this comment.
Should we do a check for self._credential here (and if we shouldn't, should we do that check in the sync credential)? Also, is there a reason that async credentials follow a close() -> self._x.close() pattern instead of close() -> self.__aexit__() -> self._x.__aexit__() pattern like sync credentials do?
There was a problem hiding this comment.
We should check that, thanks for noticing! There's no good reason for the sync vs. async difference. I prefer close -> exit today but it doesn't really matter.
104f0d4 to
96b790a
Compare
|
|
||
|
|
||
| class TokenExchangeCredential(ClientAssertionCredential, TokenFileMixin): | ||
| def __init__(self, token_file_path, **kwargs): |
There was a problem hiding this comment.
Should this require a tenant_id, client_id, and get_assertion since ClientAssertionCredential does?
There was a problem hiding this comment.
Sure, that will make it easier for future generations to understand how this works.
| async def __aenter__(self): | ||
| await self._client.__aenter__() | ||
| return self | ||
|
|
||
| async def close(self) -> None: | ||
| """Close the credential's transport session.""" | ||
| await self._client.close() |
There was a problem hiding this comment.
Are you keeping the async context management API since it's more relevant for async credentials?
There was a problem hiding this comment.
That and self._client here already implements the API. I removed it from the sync credential because I don't want to block this PR on #19746 or paste part of that PR into this one.
... will return in another PR
7acc97b to
ce4c730
Compare
This is a simplified version of what @chlowell did on this PR: Azure/azure-sdk-for-python#19902 This is based on what I understood. I’ll make sure to circle back with Charles before I get this PR out of draft. Fixes #15800
With this change, ManagedIdentityCredential authenticates via a token exchange when these variables are set:
I implemented this atop a new credential class, ClientAssertionCredential, which authenticates with a JWT assertion. It's quite similar to CertificateCredential but instead of constructing a JWT for the "client_assertion" field, this new credential gets one from a callback. I kept ClientAssertionCredential internal because there's no need to make it public today; we may find one in the future.
Closes #19304