Skip to content
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

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Aug 2, 2024

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:

git clone https://github.com/jiasli/azure-cli/
cd azure-cli
git switch cloud-shell-auth
python3 -m venv env
. env/bin/activate
pip install azdev
azdev setup -c

Log in:

az login --identity

Some basic testing:

az account get-access-token
az group show -n nonexist

Test getting SSH certificate:

az extension add -n ssh
az ssh vm --resource-group xxx --name xxx --debug

Test mgmt-plane Track 1 SDK:

az dls account list -g $RG_NAME

Test data-plane Track 1 SDK:

# Change to 2019-03-01-hybrid profile
az cloud set -n AzureCloud --profile 2019-03-01-hybrid
# Create a storage account and then test data plane command
az storage account create -n $SA_NAME -g $RG_NAME
az storage container list --account-name $SA_NAME --auth-mode login
# Revert to latest profile
az cloud set -n AzureCloud --profile latest

# Create a managed hsm
az keyvault create --hsm-name $HSM_NAME -g $RG_NAME -l uksouth --administrators $USER_OBJECT_ID --retention-days 7
# Prepare 3 cert files
openssl req -newkey rsa:2048 -nodes -keyout cert_0.key -x509 -days 365 -out cert_0.cer
openssl req -newkey rsa:2048 -nodes -keyout cert_1.key -x509 -days 365 -out cert_1.cer
openssl req -newkey rsa:2048 -nodes -keyout cert_2.key -x509 -days 365 -out cert_2.cer
# Test data plane command
az keyvault security-domain download --hsm-name $HSM_NAME --sd-wrapping-keys cert_0.cer cert_1.cer cert_2.cer --sd-quorum 2 --security-domain-file mhsm-SD.json

Copy link

azure-client-tools-bot-prd bot commented Aug 2, 2024

️✔️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
️✔️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
️✔️dla
️✔️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
️✔️kusto
️✔️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

Copy link

azure-client-tools-bot-prd bot commented Aug 2, 2024

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

@yonzhan
Copy link
Collaborator

yonzhan commented Aug 2, 2024

Core

managed_identity_type = MsiAccountTypes.system_assigned
# Cloud Shell
from .auth.msal_authentication import CloudShellCredential
cred = CloudShellCredential()
Copy link
Member Author

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.

Comment on lines 147 to 152
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=...,
)
Copy link
Member Author

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.

@jiasli
Copy link
Member Author

jiasli commented Sep 14, 2024

Copied from #25959 (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.

# 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({
Copy link
Member Author

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'
Copy link
Member Author

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)
Copy link
Member Author

@jiasli jiasli Sep 14, 2024

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
Copy link
Member Author

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.

@jiasli jiasli marked this pull request as ready for review September 14, 2024 11:24
@evelyn-ys
Copy link
Member

evelyn-ys commented Sep 18, 2024

We can test

  • mgmt plane track1 with az dls account list
  • data plane track1 with az storage blob list --auth-mode login (in hybrid profile) or az keyvault security-domain download

@@ -20,6 20,7 @@
from knack.util import CLIError
from msal import PublicClientApplication, ConfidentialClientApplication

from .constants import AZURE_CLI_CLIENT_ID
Copy link
Member Author

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.

Comment on lines 130 to 131
# 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)
Copy link
Member Author

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.

@jiasli jiasli merged commit 259fd8a into Azure:dev Oct 25, 2024
52 checks passed
@jiasli jiasli deleted the cloud-shell-auth branch October 25, 2024 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Account az login/account Auto-Assign Auto assign by bot Core CLI core infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants