[Core] Use MSAL for managed identity authentication #25959
Conversation
❌AzureCLI-FullTest
|
|
Hi @jiasli, |
ghost
added
ghost
requested a review
from
| 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.
| 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?
| 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.
| 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 |
| 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.
| # 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.
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 |
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