[Core] Use MSAL for Cloud Shell authentication #29637
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Account
| managed_identity_type = MsiAccountTypes.system_assigned | ||
| # Cloud Shell | ||
| from .auth.msal_authentication import CloudShellCredential | ||
| cred = CloudShellCredential() |
There was a problem hiding this comment.
CloudShellCredential only exposes get_token(), so it doesn't work with Track 1 SDKs. Weather wrapping it with CredentialAdaptor works remains to be tested.
| self._msal_app = PublicClientApplication( | ||
| AZURE_CLI_CLIENT_ID, # Use a real client_id, so that cache would work | ||
| # TODO: This PoC does not currently maintain a token cache; | ||
| # Ideally we should reuse the real MSAL app object which has cache configured. | ||
| # token_cache=..., | ||
| ) |
There was a problem hiding this comment.
The current msrestazure-based implementation uses no token cache. We can add token cache in the future.
|
Copied from #25959 (comment) Why we don't use managed identity for Cloud Shell authenticationComment from MSAL team: The old implementation groups Cloud Shell and other managed identity in similar API only because their wire protocol happened to be similar. But that should be an implementation detail. The meanings of them are actually quite different. Other managed identity are fundamentally confidential clients such as service principal. The Cloud Shell identity is a user account. Cloud Shell merely acts as a "broker" to obtain token for the user account. For what it's worth, the windows broker (WAM) in MSAL Python supports the same The new MSAL API is designed this way so that existing apps building on top of It is just unfortunate that Azure CLI had that |
| # A random GUID generated by uuid.uuid4() | ||
| cls.test_cloud_shell_tenant = 'ee59da2c-4d2c-4cfb-8753-ff9df4f31556' | ||
| # Cloud Shell returns a user token which contains the unique_name claim | ||
| cls.test_cloud_shell_access_token = _build_test_jwt({ |
There was a problem hiding this comment.
Tokens are now dynamically generated to avoid saving a real token in this test file.
| 'EvZdD57j3h8iGE0Tw5IzG86uNS2AQ0A') | ||
|
|
||
| # A random GUID generated by uuid.uuid4() | ||
| cls.test_cloud_shell_tenant = 'ee59da2c-4d2c-4cfb-8753-ff9df4f31556' |
There was a problem hiding this comment.
A random GUID is now used as the tenant ID.
|
|
||
| # action | ||
| cred, subscription_id, tenant_id = profile.get_raw_token(resource=self.adal_resource) | ||
| token_tuple, subscription_id, tenant_id = profile.get_raw_token(scopes=self.msal_scopes) |
There was a problem hiding this comment.
The name cred conflicts with the CloudShellCredential concept, so I rename it to better reflect its content. Aligned with #29637 (comment).
| # (tokenType, accessToken, tokenEntry) | ||
| creds = 'Bearer', sdk_token.token, token_entry | ||
| # Build a tuple of (token_type, token, token_entry) | ||
| token_tuple = 'Bearer', sdk_token.token, token_entry |
There was a problem hiding this comment.
The name creds conflicts with the "credential" concept (msal_credentials.py), so I rename it to better reflect its content.
jiasli
requested review from
bebound,
calvinhzy,
evelyn-ys,
jsntcy,
kairu-ms,
necusjz and
zhoxing-ms
as code owners
|
We can test
|
| from knack.util import CLIError | ||
| from msal import PublicClientApplication, ConfidentialClientApplication | ||
|
|
||
| from .constants import AZURE_CLI_CLIENT_ID |
There was a problem hiding this comment.
As identity.py imports from msal_credentials.py, AZURE_CLI_CLIENT_ID is moved from identity.py to constants.py to avoid circular import (msal_credentials.py imports from identity.py).
Although circular imports can also be avoided by importing modules within a function or method, this is not as good as the current solution.
jiasli
force-pushed
the
cloud-shell-auth
branch
from
7198cb5 to
65fb051
Compare
| # kwargs is already sanitized by CredentialAdaptor, so it can be safely passed to MSAL | ||
| result = self._msal_app.acquire_token_interactive(list(scopes), prompt="none", **kwargs) |
There was a problem hiding this comment.
kwargs sanitization logic is added by #30062.
| self._msal_app = PublicClientApplication( | ||
| AZURE_CLI_CLIENT_ID, # Use a real client_id, so that cache would work | ||
| # TODO: We currently don't maintain an MSAL token cache as Cloud Shell already has its own token cache. | ||
| # Ideally we should also use an MSAL token cache. | ||
| # token_cache=... | ||
| ) |
There was a problem hiding this comment.
I personally don't agree with MSAL's design and explanation (#29637 (comment)). Squeezing Cloud Shell authentication into PublicClientApplication looks awkward to me.
Even though MSAL uses PublicClientApplication for Cloud Shell authentication, the way of calling it is very different from user or service principal authentication which talks to eSTS. For Cloud Shell authentication, authority should not be passed, but for user or service principal authentication, authority is mandatory, causing inconsistent calling pattern.
Also, AZURE_CLI_CLIENT_ID is passed here only for caching purpose - it is not the app ID that is used to acquire the access token. The returned access token is actually for Azure Portal. Making the input and output contradictory.
4 participants
Related command
az login --identityDescription
This PR is separated from #25959.
Use MSAL for Cloud Shell authentication (AzureAD/microsoft-authentication-library-for-python#420).
Testing Guide
Install Azure CLI in Cloud Shell:
Log in:
Some basic testing:
Test getting SSH certificate:
Test mgmt-plane Track 1 SDK:
Test data-plane Track 1 SDK: