[Core] Drop Track 1 SDK authentication #29631
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Auto-Assign
| @@ -97,6 +97,7 @@ def scopes_to_resource(scopes): | |||
|
|
|||
|
|
|||
| def _normalize_scopes(scopes): | |||
| # TODO: Drop this function | |||
There was a problem hiding this comment.
This function is to support very old Track 2 SDKs that doesn't comply with the current authentication interface. We should consider dropping their support as well.
|
Track 1 SDK authentication is still used by |
|
Storage track1 use customized token updater which calls So it won't block |
| def __init__(self, credential, auxiliary_credentials=None): | ||
| """Cross-tenant credential adaptor. It takes a main credential and auxiliary credentials. | ||
|
|
||
| It implements Track 2 SDK's azure.core.credentials.TokenCredential by exposing get_token. |
There was a problem hiding this comment.
Even though Track 1 SDK auth support is dropped, CredentialAdaptor is preserved as it will be repurposed as an SDK-MSAL adaptor in #29955.
| profile = Profile(cli_ctx=cli_ctx) | ||
| credential, _, _ = profile.get_login_credentials() | ||
| bearer_token = credential.get_token().token | ||
| scopes = resource_to_scopes(cli_ctx.cloud.endpoints.active_directory_resource_id) | ||
| bearer_token = credential.get_token(*scopes).token |
There was a problem hiding this comment.
get_token should never be called without scopes. After Track 1 SDK auth support removal, these tests fails
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::AppServiceLogTest::test_download_win_web_log
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::WebappZipDeployScenarioTest::test_deploy_zip
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::WebappDeploymentLogsScenarioTest::test_webapp_list_deployment_logs
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::WebappDeploymentLogsScenarioTest::test_webapp_show_deployment_logs
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::WebappOneDeployScenarioTest::test_one_deploy_arm
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::WebappOneDeployScenarioTest::test_one_deploy_scm
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_deployment_source_disable_tracking_runtimestatus
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_deployment_source_track_runtimestatus_buildfailed
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_deployment_source_track_runtimestatus_runtimefailed
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_deployment_source_track_runtimestatus_runtimesucessful
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_track_runtimestatus_buildfailed
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_track_runtimestatus_runtimefailed
FAILED src/azure-cli/azure/cli/command_modules/appservice/tests/latest/test_webapp_commands.py::TrackRuntimeStatusTest::test_webapp_track_runtimestatus_runtimesucessful
self = <azure.cli.testsdk.patches.patch_retrieve_token_for_user.<locals>.get_user_credential_mock.<locals>.UserCredentialMock object at 0x7f7307d80fe0>
scopes = (), kwargs = {}
def get_token(self, *scopes, **kwargs): # pylint: disable=unused-argument
# Old Track 2 SDKs are no longer supported. ***/pull/29690
> assert len(scopes) == 1, "'scopes' must contain only one element."
E AssertionError: 'scopes' must contain only one element.
src/azure-cli-testsdk/azure/cli/testsdk/patches.py:73: AssertionError
There was a problem hiding this comment.
The access token is used to call scm_url (such as https://webapp-win-log000002.scm.azurewebsites.net), but the token's audience is ARM (https://management.core.windows.net/). This seems incorrect.
There was a problem hiding this comment.
@kamperiadis Could you please help take a look at this question or involve the person who can help take a look?
There was a problem hiding this comment.
Tagging @amberwang113 since this is related to webapps
There was a problem hiding this comment.
I created another PR #30797 to address this.
|
|
Will be fixed by #30771 |
| cred, subscription_id, _ = profile.get_login_credentials( | ||
| subscription_id=subscription_id, | ||
| resource=cli_ctx.cloud.endpoints.active_directory_data_lake_resource_id) | ||
| cred, subscription_id, _ = profile.get_login_credentials(subscription_id=subscription_id) |
There was a problem hiding this comment.
This will be covered by #30770.
| @@ -130,8 +130,7 @@ def cf_log_analytics_data_plane(cli_ctx, _): | |||
| from azure.monitor.query import LogsQueryClient | |||
| from azure.cli.core._profile import Profile | |||
| profile = Profile(cli_ctx=cli_ctx) | |||
| cred, _, _ = profile.get_login_credentials( | |||
| resource=cli_ctx.cloud.endpoints.log_analytics_resource_id) | |||
| cred, _, _ = profile.get_login_credentials() | |||
There was a problem hiding this comment.
Please double check if cli_ctx.cloud.endpoints.log_analytics_resource_id needs to be set as credential_scopes when creating LogsQueryClient.
There was a problem hiding this comment.
azure.monitor.query._logs_query_client.LogsQueryClient uses audience to accept scopes instead of credential_scopes.
When audience is not provided, it falls back to the domain of the URL.
def __init__(self, credential: TokenCredential, **kwargs: Any) -> None:
endpoint = kwargs.pop("endpoint", "https://api.loganalytics.io/v1")
if not endpoint.startswith("https://") and not endpoint.startswith("http://"):
endpoint = "https://" + endpoint
parsed_endpoint = urlparse(endpoint)
audience = kwargs.pop("audience", f"{parsed_endpoint.scheme}://{parsed_endpoint.netloc}")
self._endpoint = endpoint
auth_policy = kwargs.pop("authentication_policy", None)
self._client = MonitorQueryClient(
credential=credential,
authentication_policy=auth_policy or get_authentication_policy(credential, audience),So resource=cli_ctx.cloud.endpoints.log_analytics_resource_id actually never worked.
|
In summary, there are several incorrect/incomplete Track 2 migrations:
They are all caused by not passing credential scopes, but different data-plane SDKs have different implementations and forms of credential scopes. |
| @@ -385,16 +385,13 @@ def logout_all(self): | |||
| identity.logout_all_users() | |||
| identity.logout_all_service_principal() | |||
|
|
|||
| def get_login_credentials(self, resource=None, subscription_id=None, aux_subscriptions=None, aux_tenants=None): | |||
| def get_login_credentials(self, subscription_id=None, aux_subscriptions=None, aux_tenants=None): | |||
There was a problem hiding this comment.
For Track 1 SDK, the resource of the access token is managed by Azure CLI, not Track 1 SDK. resource is kept as a property of the credential returned by get_login_credentials(). When SDK client calls signed_session() to acquire the access token, CLI provides the access token for resource.
returns
get_login_credentials(resource) ------> credential[resource]
^
| calls signed_session()
|
Track 1 SDK client()
For Track 2 SDK, the scopes(resource/.default) of the access token is managed by SDK client instead. The scopes is passed to Track 2 SDK via credential_scopes argument when creating the client instance. The SDK client keeps it and passes it to get_token() when acquiring the access token from the credential.
returns
get_login_credentials() -------------> credential()
^
| calls get_token(scopes)
|
Track 2 SDK client(credential_scopes)
Since Track 1 SDK is no longer supported, Azure CLI doesn't need to manage resource anymore, so resource argument is dropped.
jiasli
requested review from
evelyn-ys,
calvinhzy,
bebound,
jsntcy,
kairu-ms and
necusjz
as code owners
jiasli
changed the title
| logger.debug("CredentialAdaptor.get_token: scopes=%r, kwargs=%r", scopes, kwargs) | ||
|
|
||
| # Discard unsupported kwargs: tenant_id, enable_cae | ||
| filtered_kwargs = {} | ||
| if 'data' in kwargs: | ||
| filtered_kwargs['data'] = kwargs['data'] | ||
|
|
||
| token, _ = self._get_token(scopes, **filtered_kwargs) | ||
| return token | ||
| return self._credential.get_token(*scopes, **filtered_kwargs) |
There was a problem hiding this comment.
I noticed that SSLError try catch part is removed. Any reason?
There was a problem hiding this comment.
SSLError should be handled by azure.cli.core.util.handle_exception:
azure-cli/src/azure-cli-core/azure/cli/core/util.py
Lines 77 to 83 in 05a5d8f
SSLError handling comes from ADAL age: #11093. I can't remember why I added it to src/azure-cli-core/azure/cli/core/adal_authentication.py instead of src/azure-cli-core/azure/cli/core/util.py.
Description
Close #20460
After migrating the last Track 1 SDK
azure-batchto Track 2 (#30501), Azure CLI no longer depends on any Track 1 SDK. This PR drops Track 1 SDK authentication support.But before this, there are several incorrect/incomplete Track 2 migrations that must be fixed:
credential_scopeswhen creatingBatchClient#30785scopeswhen creatingAzureDLFileSystem#30786audiencewhen creatingLogsQueryClient#30787credential_scopeswhen creating clients #30788resourceargument #30798get_mgmt_service_clientto createServiceLinkerManagementClient#30799They are all caused by passing
resourceviaget_login_credentials(), instead of passing credential scopes when creating the client. To make things worse, different data-plane SDKs have different implementations and forms of credential scopes.