[Core] Use MSAL for managed identity authentication #25959
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
❌AzureCLI-FullTest
|
|
Hi @jiasli, |
ghost
added
ghost
requested a review
from
jiasli
commented
Mar 28, 2023
Comment on lines
108
to
129
| def _normalize_expires_on(expires_on): | ||
| """ | ||
| The expires_on field returned by managed identity differs on Azure VM (epoch str) and App Service (datetime str). | ||
| Normalize to epoch int. | ||
| """ | ||
| try: | ||
| # Treat as epoch string "1605238724" | ||
| expires_on_epoch_int = int(expires_on) | ||
| except ValueError: | ||
| import datetime | ||
|
|
||
| # Python 3.6 doesn't recognize timezone as +00:00. | ||
| # These lines can be dropped after Python 3.6 is dropped. | ||
| # https://stackoverflow.com/questions/30999230/how-to-parse-timezone-with-colon | ||
| if expires_on[-3] == ":": | ||
| expires_on = expires_on[:-3] + expires_on[-2:] | ||
|
|
||
| # Treat as datetime string "11/05/2021 15:18:31 +00:00" | ||
| expires_on_epoch_int = int(datetime.datetime.strptime(expires_on, '%m/%d/%Y %H:%M:%S %z').timestamp()) | ||
|
|
||
| logger.debug("Normalize expires_on: %r -> %r", expires_on, expires_on_epoch_int) | ||
| return expires_on_epoch_int |
There was a problem hiding this comment.
We are now calling the new API on App Service: https://learn.microsoft.com/en-us/azure/app-service/overview-managed-identity?tabs=portal%2Chttp#connect-to-azure-services-in-app-code
So I assume expires_on is now an epoch int, but more test is still required to verify if this logic is still needed.
Comment on lines
71
to
95
| def set_token(self): | ||
| import traceback | ||
| from azure.cli.core.azclierror import AzureConnectionError, AzureResponseError | ||
| try: | ||
| super().set_token() | ||
| except requests.exceptions.ConnectionError as err: | ||
| logger.debug('throw requests.exceptions.ConnectionError when doing MSIAuthentication: \n%s', | ||
| traceback.format_exc()) | ||
| raise AzureConnectionError('Failed to connect to MSI. Please make sure MSI is configured correctly ' | ||
| 'and check the network connection.\nError detail: {}'.format(str(err))) | ||
| except requests.exceptions.HTTPError as err: | ||
| logger.debug('throw requests.exceptions.HTTPError when doing MSIAuthentication: \n%s', | ||
| traceback.format_exc()) | ||
| try: | ||
| raise AzureResponseError('Failed to connect to MSI. Please make sure MSI is configured correctly.\n' | ||
| 'Get Token request returned http error: {}, reason: {}' | ||
| .format(err.response.status, err.response.reason)) | ||
| except AttributeError: | ||
| raise AzureResponseError('Failed to connect to MSI. Please make sure MSI is configured correctly.\n' | ||
| 'Get Token request returned: {}'.format(err.response)) | ||
| except TimeoutError as err: | ||
| logger.debug('throw TimeoutError when doing MSIAuthentication: \n%s', | ||
| traceback.format_exc()) | ||
| raise AzureConnectionError('MSI endpoint is not responding. Please make sure MSI is configured correctly.\n' | ||
| 'Error detail: {}'.format(str(err))) |
There was a problem hiding this comment.
Should we keep such error handler?
Comment on lines
683
to
781
| system_assigned = 'MSI' # Not necessarily system-assigned. It merely means no ID is provided. | ||
| user_assigned_client_id = 'MSIClient' | ||
| user_assigned_object_id = 'MSIObject' | ||
| user_assigned_resource_id = 'MSIResource' |
There was a problem hiding this comment.
Better to rename these attributes.
| return build_sdk_access_token(result) | ||
|
|
||
|
|
||
| class CloudShellCredential: |
There was a problem hiding this comment.
Why we don't use managed identity for Cloud Shell authentication
Comment 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 acquire_token_interactive(..., prompt="none") usage to obtain a token for the already-signed-in user without prompt.
The new MSAL API is designed this way so that existing apps building on top of acquire_token_interactive(...) could smoothly utilize Cloud Shell or WAM, with no/few source code change. Just as Azure CLI needed minimal change to pick up WAM.
It is just unfortunate that Azure CLI had that az login --identity usage pattern and now stuck with it, so you can't fully reap the benefit, but that is not enough a reason for MSAL Python to revert to the old API.
jiasli
commented
Mar 28, 2023
Comment on lines
218
to
219
| user = _USER_ASSIGNED_IDENTITY if '/Microsoft.ManagedIdentity/userAssignedIdentities' in resource_id \ | ||
| else _SYSTEM_ASSIGNED_IDENTITY |
There was a problem hiding this comment.
We need to test if xms_mirid exists in all types of managed identities' access tokens. (#13188)
There was a problem hiding this comment.
Out of curiosity, why would you need to know whether the token acquired is for a system-assigned managed identity or a user-assigned one?
From MSAL's perspective, it just use the input parameters (which would indeed contain a client_id/resource_id/etc when and only when using a user-assigned identity). Once a token is returned, the job is done. Why would a caller need to care whether it is a system-assigned or user-assigned based on the token outcome?
There was a problem hiding this comment.
Azure CLI has been showing system-assigned managed identity or user-assigned one since managed identity was initially supported. I don't tend to change its behavior. However, the logic for determining system-assigned or user-assigned is incorrect.
See
|
MSAL adoption |
jiasli
commented
Mar 29, 2023
Comment on lines
570
to
652
| if not msi_info: | ||
| msi_info = account[_SUBSCRIPTION_NAME] # fall back to old persisting way |
There was a problem hiding this comment.
In versions <= 2.0.50 (November 6, 2018), _SUBSCRIPTION_NAME is used to denote the managed identity ID type. This is super confusing, so _ASSIGNED_IDENTITY_INFO was added in #7744 and these lines are added as an adaptor. However, such logic is still difficult to maintain and can easily leads to unwanted code path. Even its creator admits:
the code is a bit messy here to support both old and new styles.
#7744 (comment)
There was a problem hiding this comment.
If we remove this adaptor, we can add such history notes:
[BREAKING CHANGE] No long be compatible with login profile created by az login --identity of Azure CLI versions <= 2.0.50. If you are updating from those versions, please run az login --identity again to refresh the login profile.
jiasli
commented
Mar 29, 2023
Comment on lines
558
to
563
| # We no longer support login profile created by versions < 2.0.51, which uses _SUBSCRIPTION_NAME as | ||
| # _ASSIGNED_IDENTITY_INFO. | ||
| # https://github.com/Azure/azure-cli/pull/7744 | ||
| if not identity_info: | ||
| raise CLIError(f'{_ASSIGNED_IDENTITY_INFO} property is missing from the current account. ' | ||
| 'Please run `az login --identity`.') |
There was a problem hiding this comment.
I have replaced the adaptor with a more general not-None check.
3 tasks
This was referenced Nov 10, 2023
This was referenced Jun 17, 2024
Closed
This was referenced Aug 2, 2024
jiasli
changed the title
|
| rule | cmd_name | rule_message | suggest_message |
|---|---|---|---|
| login | cmd login added parameter client_id |
||
| login | cmd login added parameter object_id |
||
| login | cmd login added parameter resource_id |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
az login --identityDescription
msrestazureto MSAL #25860az login --identityfor Azure Arc #16573az loginwith managed identity indicating system assigned when the identity is user assigned #13188Changes:
Use
msal.ManagedIdentityfor managed identity authentication.Use
PublicClientApplication.acquire_token_interactive(prompt="none")for Cloud Shell authentication.Limit the functionality of
azure.cli.core._profile.Profile._create_credentialandazure.cli.core.auth.identity.Identityto only user and service principal, because their implementation are very different from that of managed identity and Cloud Shell, which only talks to the local machine's metadata endpoint.Rename
MSIin source code tomanaged identity. However, the text inaz account listresult is still preserved until further discussion -MSI,MSIClient-{id},MSIObject-{id},MSIResource-{id}.Testing Guide