Skip to content

{Auth} Do not wrap MSIAuthenticationWrapper with CredentialAdaptor#30962

Merged
jiasli merged 1 commit intoAzure:devfrom
jiasli:mi-cred
Mar 6, 2025
Merged

{Auth} Do not wrap MSIAuthenticationWrapper with CredentialAdaptor#30962
jiasli merged 1 commit intoAzure:devfrom
jiasli:mi-cred

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Mar 6, 2025 •
edited
Loading

Related command
az login --identity

Description
Thanks to @FumingZhang for reporting this issue.

#29955 breaks managed identity authentication:

2025-03-04T23:29:32.3029634Z   File "/opt/azEnv/lib/python3.12/site-packages/azure/core/pipeline/policies/_authentication.py", line 96, in _request_token
2025-03-04T23:29:32.3030187Z     self._token = cast(***Credential, self._credential).get_token(*scopes, **kwargs)
2025-03-04T23:29:32.3030542Z                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2025-03-04T23:29:32.3030884Z   File "/opt/azure-cli/src/azure-cli-core/azure/cli/core/auth/credential_adaptor.py", line 35, in get_token
2025-03-04T23:29:32.3031327Z     return build_sdk_access_token(self._credential.acquire_token(list(scopes), **filtered_kwargs))
2025-03-04T23:29:32.3031666Z                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2025-03-04T23:29:32.3032010Z AttributeError: 'MSIAuthenticationWrapper' object has no attribute 'acquire_token'

This is because MSIAuthenticationWrapper is unnecessarily wrapped by CredentialAdaptor, causing CredentialAdaptor to call acquire_token on it.

Testing Guide
az login --identity

@jiasli jiasli requested review from bebound and evelyn-ys as code owners March 6, 2025 08:04
@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Mar 6, 2025 •
edited
Loading

️✔️AzureCLI-FullTest
️✔️acr
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️acs
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️advisor
️✔️latest
️✔️3.12
️✔️3.9
️✔️ams
️✔️latest
️✔️3.12
️✔️3.9
️✔️apim
️✔️latest
️✔️3.12
️✔️3.9
️✔️appconfig
️✔️latest
️✔️3.12
️✔️3.9
️✔️appservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️aro
️✔️latest
️✔️3.12
️✔️3.9
️✔️backup
️✔️latest
️✔️3.12
️✔️3.9
️✔️batch
️✔️latest
️✔️3.12
️✔️3.9
️✔️batchai
️✔️latest
️✔️3.12
️✔️3.9
️✔️billing
️✔️latest
️✔️3.12
️✔️3.9
️✔️botservice
️✔️latest
️✔️3.12
️✔️3.9
️✔️cdn
️✔️latest
️✔️3.12
️✔️3.9
️✔️cloud
️✔️latest
️✔️3.12
️✔️3.9
️✔️cognitiveservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️compute_recommender
️✔️latest
️✔️3.12
️✔️3.9
️✔️computefleet
️✔️latest
️✔️3.12
️✔️3.9
️✔️config
️✔️latest
️✔️3.12
️✔️3.9
️✔️configure
️✔️latest
️✔️3.12
️✔️3.9
️✔️consumption
️✔️latest
️✔️3.12
️✔️3.9
️✔️container
️✔️latest
️✔️3.12
️✔️3.9
️✔️containerapp
️✔️latest
️✔️3.12
️✔️3.9
️✔️core
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.12
️✔️3.9
️✔️databoxedge
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️dls
️✔️latest
️✔️3.12
️✔️3.9
️✔️dms
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventgrid
️✔️latest
️✔️3.12
️✔️3.9
️✔️eventhubs
️✔️latest
️✔️3.12
️✔️3.9
️✔️feedback
️✔️latest
️✔️3.12
️✔️3.9
️✔️find
️✔️latest
️✔️3.12
️✔️3.9
️✔️hdinsight
️✔️latest
️✔️3.12
️✔️3.9
️✔️identity
️✔️latest
️✔️3.12
️✔️3.9
️✔️iot
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️keyvault
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️lab
️✔️latest
️✔️3.12
️✔️3.9
️✔️managedservices
️✔️latest
️✔️3.12
️✔️3.9
️✔️maps
️✔️latest
️✔️3.12
️✔️3.9
️✔️marketplaceordering
️✔️latest
️✔️3.12
️✔️3.9
️✔️monitor
️✔️latest
️✔️3.12
️✔️3.9
️✔️mysql
️✔️latest
️✔️3.12
️✔️3.9
️✔️netappfiles
️✔️latest
️✔️3.12
️✔️3.9
️✔️network
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️policyinsights
️✔️latest
️✔️3.12
️✔️3.9
️✔️privatedns
️✔️latest
️✔️3.12
️✔️3.9
️✔️profile
️✔️latest
️✔️3.12
️✔️3.9
️✔️rdbms
️✔️latest
️✔️3.12
️✔️3.9
️✔️redis
️✔️latest
️✔️3.12
️✔️3.9
️✔️relay
️✔️latest
️✔️3.12
️✔️3.9
️✔️resource
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️role
️✔️latest
️✔️3.12
️✔️3.9
️✔️search
️✔️latest
️✔️3.12
️✔️3.9
️✔️security
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicebus
️✔️latest
️✔️3.12
️✔️3.9
️✔️serviceconnector
️✔️latest
️✔️3.12
️✔️3.9
️✔️servicefabric
️✔️latest
️✔️3.12
️✔️3.9
️✔️signalr
️✔️latest
️✔️3.12
️✔️3.9
️✔️sql
️✔️latest
️✔️3.12
️✔️3.9
️✔️sqlvm
️✔️latest
️✔️3.12
️✔️3.9
️✔️storage
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.12
️✔️3.9
️✔️telemetry
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9
️✔️util
️✔️latest
️✔️3.12
️✔️3.9
️✔️vm
️✔️2018-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2019-03-01-hybrid
️✔️3.12
️✔️3.9
️✔️2020-09-01-hybrid
️✔️3.12
️✔️3.9
️✔️latest
️✔️3.12
️✔️3.9

@azure-client-tools-bot-prd
Copy link

azure-client-tools-bot-prd bot commented Mar 6, 2025 •
edited
Loading

️✔️AzureCLI-BreakingChangeTest
️✔️Non Breaking Changes

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 6, 2025

Thank you for your contribution! We will review the pull request and get back to you soon.

@github-actions
Copy link

github-actions bot commented Mar 6, 2025

The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR.

Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions).
After that please run the following commands to enable git hooks:

pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>

@microsoft-github-policy-service microsoft-github-policy-service bot added the Auto-Assign Auto assign by bot label Mar 6, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot added the Account az login/account label Mar 6, 2025
@jiasli
Copy link
Member Author

jiasli commented Mar 6, 2025

I initially made a more complex attempt by letting SubscriptionFinder.find_using_common_tenant and SubscriptionFinder.find_using_specific_tenant take an sdk_credential (CredentialAdaptor).

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Line 830 in 3b70655

def find_using_common_tenant(self, username, credential=None):

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Line 903 in 3b70655

def find_using_specific_tenant(self, tenant, credential, tenant_id_description=None):

But this requires all invocations to find_using_common_tenant and find_using_specific_tenant to wrap the MSAL credential with CredentialAdaptor:

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Lines 206 to 209 in 3b70655

if tenant:
subscriptions = subscription_finder.find_using_specific_tenant(tenant, credential)
else:
subscriptions = subscription_finder.find_using_common_tenant(username, credential)

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Line 326 in 3b70655

subscriptions = subscription_finder.find_using_specific_tenant(tenant, cred)

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Line 353 in 3b70655

subscriptions = subscription_finder.find_using_specific_tenant(tenant, cred)

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Lines 716 to 720 in 3b70655

if is_service_principal:
subscriptions = subscription_finder.find_using_specific_tenant(tenant, identity_credential)
else:
# pylint: disable=protected-access
subscriptions = subscription_finder.find_using_common_tenant(user_name, identity_credential)

except for Profile.login_with_managed_identity

azure-cli/src/azure-cli-core/azure/cli/core/_profile.py

Line 298 in 3b70655

subscriptions = subscription_finder.find_using_specific_tenant(tenant, msi_creds)

This requires much more changes than this PR's implementation.

@jiasli jiasli requested a review from FumingZhang March 6, 2025 08:20
FumingZhang
FumingZhang approved these changes Mar 6, 2025
View reviewed changes
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I tested the change via AKS live test pipeline, and the identity login is functioning as expected.

@jiasli jiasli merged commit 939778e into Azure:dev Mar 6, 2025
53 checks passed
@jiasli jiasli deleted the mi-cred branch March 6, 2025 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evelyn-ys evelyn-ys evelyn-ys approved these changes

@FumingZhang FumingZhang FumingZhang approved these changes

@bebound bebound Awaiting requested review from bebound bebound is a code owner

@yonzhan yonzhan Awaiting requested review from yonzhan

Assignees

@jiasli jiasli

Labels

Account az login/account Auto-Assign Auto assign by bot

Projects

None yet

Milestone

April 2025 (2025-04-01)

Development

Successfully merging this pull request may close these issues.

4 participants

@jiasli @yonzhan @evelyn-ys @FumingZhang