-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Core] Use MSAL for Cloud Shell authentication #29637
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
Core |
managed_identity_type = MsiAccountTypes.system_assigned | ||
# Cloud Shell | ||
from .auth.msal_authentication import CloudShellCredential | ||
cred = CloudShellCredential() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name creds
conflicts with the "credential" concept (msal_credentials.py
), so I rename it to better reflect its content.
We can test
|
@@ -20,6 20,7 @@ | |||
from knack.util import CLIError | |||
from msal import PublicClientApplication, ConfidentialClientApplication | |||
|
|||
from .constants import AZURE_CLI_CLIENT_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kwargs
sanitization logic is added by #30062.
Related command
az login --identity
Description
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: