Skip to content

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Mar 19, 2025

Related command
az ssh vm

Description
#19853 removed Profile.get_msal_token and let ssh extension call profile.get_login_credentials and credential.get_token to get the certificate:

https://github.com/Azure/azure-cli-extensions/blob/695bd02037a7a8abd6b0ac76ae1ac1559ae46c41/src/ssh/azext_ssh/custom.py#L231-L233

        credential, _, _ = profile.get_login_credentials(subscription_id=profile.get_subscription()["id"])
        certificatedata = credential.get_token(*scopes, data=data)
        certificate = certificatedata.token

This turns out to be a bad design as

  1. No SDK is involved, but the SDK token protocol get_token is used.
  2. SDK token protocol get_token doesn't support data argument at all. This is a CLI-specific extension/alteration.
  3. With the support of get_token_info protocol ({Auth} Support get_token_info protocol #30928), get_token is deprecated.

This PR brings back get_msal_token, so that ssh extension can seamlessly switch to the old interface without any update:

https://github.com/Azure/azure-cli-extensions/blob/695bd02037a7a8abd6b0ac76ae1ac1559ae46c41/src/ssh/azext_ssh/custom.py#L229

    if hasattr(profile, "get_msal_token"):
        # we used to use the username from the token but now we throw it away
        _, certificate = profile.get_msal_token(scopes, data)

Testing Guide

az ssh vm --resource-group xxx --vm-name xxx

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

azure-client-tools-bot-prd bot commented Mar 19, 2025

️✔️AzureCLI-FullTest
️✔️acr
️✔️latest
️✔️3.12
️✔️3.9
️✔️acs
️✔️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
️✔️latest
️✔️3.12
️✔️3.9
️✔️cosmosdb
️✔️latest
️✔️3.12
️✔️3.9
️✔️databoxedge
️✔️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
️✔️latest
️✔️3.12
️✔️3.9
️✔️keyvault
️✔️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
️✔️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
️✔️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
️✔️latest
️✔️3.12
️✔️3.9
️✔️synapse
️✔️latest
️✔️3.12
️✔️3.9
️✔️telemetry
️✔️latest
️✔️3.12
️✔️3.9
️✔️util
️✔️latest
️✔️3.12
️✔️3.9
️✔️vm
️✔️latest
️✔️3.12
️✔️3.9

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

Hi @jiasli,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

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

azure-client-tools-bot-prd bot commented Mar 19, 2025

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

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 19, 2025

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

@github-actions
Copy link

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>

"""Get VM SSH certificate. Do not use it for other purposes. To get an access token, use get_raw_token instead.
"""
credential, _, _ = self.get_login_credentials()
certificate_string = credential.get_token(*scopes, data=data).token
Copy link
Member Author

Choose a reason for hiding this comment

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

get_token will be replaces by acquire_token after migrating managed identity authentication to MSAL: #25959

@jiasli jiasli force-pushed the get_msal_token branch 2 times, most recently from fbc0e05 to 7e9af5d Compare June 26, 2025 08:40
Comment on lines 168 to 170
if data is not None:
from azure.cli.core.azclierror import AuthenticationError
raise AuthenticationError("VM SSH currently doesn't support managed identity.")
Copy link
Member Author

Choose a reason for hiding this comment

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

This detection was in adal_authentication.MSIAuthenticationWrapper:

from azure.cli.core.azclierror import AuthenticationError
raise AuthenticationError("VM SSH currently doesn't support managed identity.")

It got dropped by #31577 without being migrated to msal_credentials.ManagedIdentityCredential.

@jiasli jiasli changed the title {Auth} Bring back get_msal_token {Auth} Bring back get_msal_token for acquiring VM SSH certificate Jun 26, 2025
@jiasli jiasli marked this pull request as ready for review June 26, 2025 11:15
@jiasli
Copy link
Member Author

jiasli commented Jun 30, 2025

get_msal_token was introduced in #12999. To be honest, the naming is not ideal. It could have been get_certificate or something similar.

As get_msal_token has been released with ssh extension for years, changing it would be difficult and gives little benefit/profit.

@jiasli jiasli merged commit 8c65f4a into Azure:dev Jun 30, 2025
48 checks passed
@jiasli jiasli deleted the get_msal_token branch June 30, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants