From 8bf5c31d0f035a22100a287adcb7994a6eefe478 Mon Sep 17 00:00:00 2001 From: qianwens Date: Mon, 8 Jun 2020 18:26:34 +0800 Subject: [PATCH 1/6] [MSAL]Add AAD error handling, refine SP logout --- .../azure/cli/core/_identity.py | 20 ++++++------ src/azure-cli-core/azure/cli/core/_msal.py | 2 ++ src/azure-cli-core/azure/cli/core/_profile.py | 13 ++++---- .../azure/cli/core/authentication.py | 31 ++++++++----------- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_identity.py b/src/azure-cli-core/azure/cli/core/_identity.py index 67fc617372e..a6d420143ce 100644 --- a/src/azure-cli-core/azure/cli/core/_identity.py +++ b/src/azure-cli-core/azure/cli/core/_identity.py @@ -88,10 +88,10 @@ def _msal_app(self): # Initialize _msal_app for logout, since Azure Identity doesn't provide the functionality for logout from msal import PublicClientApplication # sdk/identity/azure-identity/azure/identity/_internal/msal_credentials.py:95 - from azure.identity._internal.persistent_cache import load_persistent_cache + from azure.identity._internal.persistent_cache import load_user_cache # Store for user token persistence - cache = load_persistent_cache(self.allow_unencrypted) + cache = load_user_cache(self.allow_unencrypted) # Build the authority in MSAL style msal_authority = "https://{}/{}".format(self.authority, self.tenant_id) return PublicClientApplication(authority=msal_authority, client_id=self.client_id, token_cache=cache) @@ -258,12 +258,12 @@ def _decode_managed_identity_token(credential, resource): decoded = json.loads(decoded_str) return decoded - def get_user(self, user_or_sp=None): - accounts = self._msal_app.get_accounts(user_or_sp) if user_or_sp else self._msal_app.get_accounts() + def get_user(self, user=None): + accounts = self._msal_app.get_accounts(user) if user else self._msal_app.get_accounts() return accounts - def logout_user(self, user_or_sp): - accounts = self._msal_app.get_accounts(user_or_sp) + def logout_user(self, user): + accounts = self._msal_app.get_accounts(user) logger.info('Before account removal:') logger.info(json.dumps(accounts)) @@ -271,11 +271,13 @@ def logout_user(self, user_or_sp): for account in accounts: self._msal_app.remove_account(account) - accounts = self._msal_app.get_accounts(user_or_sp) + accounts = self._msal_app.get_accounts(user) logger.info('After account removal:') logger.info(json.dumps(accounts)) + + def logout_sp(self, sp): # remove service principal secrets - self._msal_store.remove_cached_creds(user_or_sp) + self._msal_store.remove_cached_creds(sp) def logout_all(self): # TODO: Support multi-authority logout @@ -423,7 +425,7 @@ def add_credential(self, credential): "refreshToken": refresh_token[0]['secret'], "_clientId": _CLIENT_ID, "_authority": self._cli_ctx.cloud.endpoints.active_directory.rstrip('/') + - "/" + credential._auth_record.tenant_id, + "/" + credential._auth_record.tenant_id, # pylint: disable=bad-continuation "isMRRT": True } self.adal_token_cache.add([entry]) diff --git a/src/azure-cli-core/azure/cli/core/_msal.py b/src/azure-cli-core/azure/cli/core/_msal.py index cc46e9d2d57..5f0fbce3ea1 100644 --- a/src/azure-cli-core/azure/cli/core/_msal.py +++ b/src/azure-cli-core/azure/cli/core/_msal.py @@ -10,6 +10,7 @@ class AdalRefreshTokenBasedClientApplication(ClientApplication): """ This is added only for vmssh feature. It is a temporary solution and will deprecate after MSAL adopted completely. + todo: msal """ def _acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family( self, authority, scopes, account, **kwargs): @@ -17,6 +18,7 @@ def _acquire_token_silent_by_finding_rt_belongs_to_me_or_my_family( return self._acquire_token_silent_by_finding_specific_refresh_token( authority, scopes, None, **kwargs) + # pylint:disable=arguments-differ def _acquire_token_silent_by_finding_specific_refresh_token( self, authority, scopes, query, rt_remover=None, break_condition=lambda response: False, **kwargs): diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index bfdaba87821..96f4fabd22a 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -81,7 +81,6 @@ def _get_authority_url(cli_ctx, tenant): def get_credential_types(cli_ctx): - class CredentialType(Enum): # pylint: disable=too-few-public-methods cloud = get_active_cloud(cli_ctx) management = cli_ctx.cloud.endpoints.management @@ -110,6 +109,7 @@ def __init__(self, storage=None, auth_ctx_factory=None, use_global_creds_cache=T self._ad = self.cli_ctx.cloud.endpoints.active_directory self._adal_cache = ADALCredentialCache(cli_ctx=self.cli_ctx) + # pylint: disable=too-many-branches,too-many-statements def login(self, interactive, username, @@ -428,9 +428,6 @@ def logout(self, user_or_sp, clear_credential): # Always remove credential from the legacy cred cache, regardless of MSAL cache, to be deprecated adal_cache = ADALCredentialCache(cli_ctx=self.cli_ctx) adal_cache.remove_cached_creds(user_or_sp) - # remove service principle secret - msal_cache = MSALSecretStore() - msal_cache.remove_cached_creds(user_or_sp) logger.warning('Account %s was logged out from Azure CLI', user_or_sp) else: @@ -453,7 +450,8 @@ def logout(self, user_or_sp, clear_credential): 'To clear the credential, run `az logout --username %s --clear-credential`.', user_or_sp, user_or_sp) else: - logger.warning("The credential of %s was not found from MSAL encrypted cache.", user_or_sp) + # remove service principle secret + identity.logout_sp(user_or_sp) def logout_all(self, clear_credential): self._storage[_SUBSCRIPTIONS] = [] @@ -637,8 +635,9 @@ def refresh_accounts(self, subscription_finder=None): subscriptions = subscription_finder.find_using_specific_tenant(tenant, identity_credential) else: # pylint: disable=protected-access - subscriptions = subscription_finder.find_using_common_tenant(identity_credential._auth_record, - identity_credential) + subscriptions = subscription_finder. \ + find_using_common_tenant(identity_credential._auth_record, # pylint: disable=protected-access + identity_credential) except Exception as ex: # pylint: disable=broad-except logger.warning("Refreshing for '%s' failed with an error '%s'. The existing accounts were not " "modified. You can run 'az login' later to explicitly refresh them", user_name, ex) diff --git a/src/azure-cli-core/azure/cli/core/authentication.py b/src/azure-cli-core/azure/cli/core/authentication.py index 23e130903b0..a773a0032b7 100644 --- a/src/azure-cli-core/azure/cli/core/authentication.py +++ b/src/azure-cli-core/azure/cli/core/authentication.py @@ -53,24 +53,19 @@ def _get_token(self, *scopes): if in_cloud_console(): AuthenticationWrapper._log_hostname() - raise CLIError("Credentials have expired due to inactivity or " - "configuration of your account was changed. {}Error details: {}" - .format("Please run 'az login'. " if not in_cloud_console() else '', err)) - # todo: error type - # err = (getattr(err, 'error_response', None) or {}).get('error_description') or '' - # if 'AADSTS70008' in err: # all errors starting with 70008 should be creds expiration related - # raise CLIError("Credentials have expired due to inactivity. {}".format( - # "Please run 'az login'" if not in_cloud_console() else '')) - # if 'AADSTS50079' in err: - # raise CLIError("Configuration of your account was changed. {}".format( - # "Please run 'az login'" if not in_cloud_console() else '')) - # if 'AADSTS50173' in err: - # raise CLIError("The credential data used by CLI has been expired because you might have changed or " - # "reset the password. {}".format( - # "Please clear browser's cookies and run 'az login'" - # if not in_cloud_console() else '')) - # - # raise CLIError(err) + err = getattr(err, 'message', None) or '' + if 'AADSTS70008' in err: # all errors starting with 70008 should be creds expiration related + raise CLIError("Credentials have expired due to inactivity. {}".format( + "Please run 'az login'" if not in_cloud_console() else '')) + if 'AADSTS50079' in err: + raise CLIError("Configuration of your account was changed. {}".format( + "Please run 'az login'" if not in_cloud_console() else '')) + if 'AADSTS50173' in err: + raise CLIError("The credential data used by CLI has been expired because you might have changed or " + "reset the password. {}".format( + "Please clear browser's cookies and run 'az login'" + if not in_cloud_console() else '')) + raise CLIError(err) except requests.exceptions.SSLError as err: from .util import SSLERROR_TEMPLATE raise CLIError(SSLERROR_TEMPLATE.format(str(err))) From ecfa5ea657a0f844cbfdc7ef6f9ca682777fb959 Mon Sep 17 00:00:00 2001 From: qianwens Date: Tue, 9 Jun 2020 17:20:33 +0800 Subject: [PATCH 2/6] [MSAL]support MSI object/resource id --- src/azure-cli-core/azure/cli/core/_identity.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_identity.py b/src/azure-cli-core/azure/cli/core/_identity.py index a6d420143ce..2d57e21585c 100644 --- a/src/azure-cli-core/azure/cli/core/_identity.py +++ b/src/azure-cli-core/azure/cli/core/_identity.py @@ -174,8 +174,7 @@ def login_with_managed_identity(self, resource, identity_id=None): if identity_id: # Try resource ID if is_valid_resource_id(identity_id): - # TODO: Support resource ID in Azure Identity - credential = ManagedIdentityCredential(resource_id=identity_id) + credential = ManagedIdentityCredential(identity_config={"resource_id": identity_id}) id_type = self.MANAGED_IDENTITY_RESOURCE_ID else: authenticated = False @@ -193,8 +192,7 @@ def login_with_managed_identity(self, resource, identity_id=None): if not authenticated: try: # Try object ID - # TODO: Support resource ID in Azure Identity - credential = ManagedIdentityCredential(object_id=identity_id) + credential = ManagedIdentityCredential(identity_config={"object_id": identity_id}) id_type = self.MANAGED_IDENTITY_OBJECT_ID authenticated = True except HTTPError as ex: From d7d55b1cdf2e640881de4c2d45bc1909c34a2b3e Mon Sep 17 00:00:00 2001 From: qianwens Date: Tue, 9 Jun 2020 17:51:18 +0800 Subject: [PATCH 3/6] [MSAL]support MSI object/resource id --- src/azure-cli-core/azure/cli/core/_identity.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/azure-cli-core/azure/cli/core/_identity.py b/src/azure-cli-core/azure/cli/core/_identity.py index 2d57e21585c..25b77654e05 100644 --- a/src/azure-cli-core/azure/cli/core/_identity.py +++ b/src/azure-cli-core/azure/cli/core/_identity.py @@ -19,7 +19,8 @@ UsernamePasswordCredential, ClientSecretCredential, CertificateCredential, - ManagedIdentityCredential + ManagedIdentityCredential, + CredentialUnavailableError ) _CLIENT_ID = '04b07795-8ddb-461a-bbee-02f9e1bf7b46' @@ -183,6 +184,9 @@ def login_with_managed_identity(self, resource, identity_id=None): credential = ManagedIdentityCredential(client_id=identity_id) id_type = self.MANAGED_IDENTITY_CLIENT_ID authenticated = True + except CredentialUnavailableError as e: + logger.debug('MSI authentication error: %s', e.message) + logger.info('Sniff: not an MSI client id') except HTTPError as ex: if ex.response.reason == 'Bad Request' and ex.response.status == 400: logger.info('Sniff: not an MSI client id') From 0dec5b9d34260fcae3669796a5f3f5c974f0bc31 Mon Sep 17 00:00:00 2001 From: qianwens Date: Tue, 9 Jun 2020 18:12:19 +0800 Subject: [PATCH 4/6] [MSAL]support MSI object/resource id --- src/azure-cli-core/azure/cli/core/_identity.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_identity.py b/src/azure-cli-core/azure/cli/core/_identity.py index 25b77654e05..69fc9765e2c 100644 --- a/src/azure-cli-core/azure/cli/core/_identity.py +++ b/src/azure-cli-core/azure/cli/core/_identity.py @@ -19,10 +19,11 @@ UsernamePasswordCredential, ClientSecretCredential, CertificateCredential, - ManagedIdentityCredential, - CredentialUnavailableError + ManagedIdentityCredential ) +from azure.core.exceptions import ClientAuthenticationError + _CLIENT_ID = '04b07795-8ddb-461a-bbee-02f9e1bf7b46' logger = get_logger(__name__) @@ -172,6 +173,7 @@ def login_with_managed_identity(self, resource, identity_id=None): credential = None id_type = None + scope = resource.rstrip('/') + '/.default' if identity_id: # Try resource ID if is_valid_resource_id(identity_id): @@ -182,9 +184,10 @@ def login_with_managed_identity(self, resource, identity_id=None): try: # Try client ID credential = ManagedIdentityCredential(client_id=identity_id) + credential.get_token(scope) id_type = self.MANAGED_IDENTITY_CLIENT_ID authenticated = True - except CredentialUnavailableError as e: + except ClientAuthenticationError as e: logger.debug('MSI authentication error: %s', e.message) logger.info('Sniff: not an MSI client id') except HTTPError as ex: @@ -197,8 +200,12 @@ def login_with_managed_identity(self, resource, identity_id=None): try: # Try object ID credential = ManagedIdentityCredential(identity_config={"object_id": identity_id}) + credential.get_token(scope) id_type = self.MANAGED_IDENTITY_OBJECT_ID authenticated = True + except ClientAuthenticationError as e: + logger.debug('MSI authentication error: %s', e.message) + logger.info('Sniff: not an MSI object id') except HTTPError as ex: if ex.response.reason == 'Bad Request' and ex.response.status == 400: logger.info('Sniff: not an MSI object id') From 0f17cbc0a381e175010eda276d6de414b8a9bbdf Mon Sep 17 00:00:00 2001 From: qianwens Date: Tue, 9 Jun 2020 18:26:00 +0800 Subject: [PATCH 5/6] [MSAL]support MSI object/resource id --- src/azure-cli-core/azure/cli/core/_identity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/azure-cli-core/azure/cli/core/_identity.py b/src/azure-cli-core/azure/cli/core/_identity.py index 69fc9765e2c..5d69f0396bc 100644 --- a/src/azure-cli-core/azure/cli/core/_identity.py +++ b/src/azure-cli-core/azure/cli/core/_identity.py @@ -177,7 +177,7 @@ def login_with_managed_identity(self, resource, identity_id=None): if identity_id: # Try resource ID if is_valid_resource_id(identity_id): - credential = ManagedIdentityCredential(identity_config={"resource_id": identity_id}) + credential = ManagedIdentityCredential(identity_config={"msi_res_id": identity_id}) id_type = self.MANAGED_IDENTITY_RESOURCE_ID else: authenticated = False From d7aa95d78d464929a8a848774399d017d0aefbef Mon Sep 17 00:00:00 2001 From: qianwens Date: Wed, 10 Jun 2020 13:37:36 +0800 Subject: [PATCH 6/6] [MSAL]support MSI object/resource id --- src/azure-cli-core/azure/cli/core/_identity.py | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_identity.py b/src/azure-cli-core/azure/cli/core/_identity.py index 5d69f0396bc..bdede20abad 100644 --- a/src/azure-cli-core/azure/cli/core/_identity.py +++ b/src/azure-cli-core/azure/cli/core/_identity.py @@ -22,8 +22,6 @@ ManagedIdentityCredential ) -from azure.core.exceptions import ClientAuthenticationError - _CLIENT_ID = '04b07795-8ddb-461a-bbee-02f9e1bf7b46' logger = get_logger(__name__) @@ -170,6 +168,7 @@ def login_with_service_principal_certificate(self, client_id, certificate_path): def login_with_managed_identity(self, resource, identity_id=None): from msrestazure.tools import is_valid_resource_id from requests import HTTPError + from azure.core.exceptions import ClientAuthenticationError credential = None id_type = None @@ -188,11 +187,11 @@ def login_with_managed_identity(self, resource, identity_id=None): id_type = self.MANAGED_IDENTITY_CLIENT_ID authenticated = True except ClientAuthenticationError as e: - logger.debug('MSI authentication error: %s', e.message) - logger.info('Sniff: not an MSI client id') + logger.debug('Managed Identity authentication error: %s', e.message) + logger.info('Username is not an MSI client id') except HTTPError as ex: if ex.response.reason == 'Bad Request' and ex.response.status == 400: - logger.info('Sniff: not an MSI client id') + logger.info('Username is not an MSI client id') else: raise @@ -204,11 +203,11 @@ def login_with_managed_identity(self, resource, identity_id=None): id_type = self.MANAGED_IDENTITY_OBJECT_ID authenticated = True except ClientAuthenticationError as e: - logger.debug('MSI authentication error: %s', e.message) - logger.info('Sniff: not an MSI object id') + logger.debug('Managed Identity authentication error: %s', e.message) + logger.info('Username is not an MSI object id') except HTTPError as ex: if ex.response.reason == 'Bad Request' and ex.response.status == 400: - logger.info('Sniff: not an MSI object id') + logger.info('Username is not an MSI object id') else: raise