From 2620dca8bbdb52991e710da9d140aacfdddf1687 Mon Sep 17 00:00:00 2001 From: jiasli <4003950+jiasli@users.noreply.github.com> Date: Thu, 14 Oct 2021 17:21:08 +0800 Subject: [PATCH 1/3] Support Azure Stack and ADFS with MSAL --- src/azure-cli-core/azure/cli/core/_profile.py | 9 ++-- .../azure/cli/core/auth/identity.py | 42 ++++++++++++++----- .../cli/core/auth/tests/test_identity.py | 33 ++++++++++++++- .../azure/cli/core/auth/util.py | 6 +-- .../azure/cli/core/tests/test_profile.py | 26 +----------- 5 files changed, 70 insertions(+), 46 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index fd948429a67..834925fb4c3 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -147,10 +147,7 @@ def login(self, if not scopes: scopes = self._arm_scope - # For ADFS, auth_tenant is 'adfs' - # https://github.com/Azure/azure-sdk-for-python/blob/661cd524e88f480c14220ed1f86de06aaff9a977/sdk/identity/azure-identity/CHANGELOG.md#L19 - authority, auth_tenant = _detect_adfs_authority(self.cli_ctx.cloud.endpoints.active_directory, tenant) - identity = Identity(authority=authority, tenant_id=auth_tenant, client_id=client_id) + identity = Identity(self._authority, tenant_id=tenant, client_id=client_id) user_identity = None if interactive: @@ -589,7 +586,7 @@ def _create_credential(self, account, tenant_id=None, client_id=None): user_type = account[_USER_ENTITY][_USER_TYPE] username_or_sp_id = account[_USER_ENTITY][_USER_NAME] tenant_id = tenant_id if tenant_id else account[_TENANT_ID] - identity = Identity(client_id=client_id, authority=self._authority, tenant_id=tenant_id) + identity = Identity(self._authority, tenant_id=tenant_id, client_id=client_id) # User if user_type == _USER: @@ -720,7 +717,7 @@ def find_using_common_tenant(self, username, credential=None): logger.info("Finding subscriptions under tenant %s", t.tenant_id_name) - identity = Identity(self.authority, tenant_id) + identity = Identity(self.authority, tenant_id=tenant_id) specific_tenant_credential = identity.get_user_credential(username) diff --git a/src/azure-cli-core/azure/cli/core/auth/identity.py b/src/azure-cli-core/azure/cli/core/auth/identity.py index 37e065b0334..2d6afa8c34a 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -38,19 +38,22 @@ class Identity: # pylint: disable=too-many-instance-attributes # https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/407 http_cache = None - def __init__(self, authority=None, tenant_id=None, client_id=None): + def __init__(self, authority, tenant_id=None, client_id=None): """ - - :param authority: AAD endpoint, like https://login.microsoftonline.com/ - :param tenant_id: Tenant GUID, like 00000000-0000-0000-0000-000000000000 + :param authority: Authentication authority endpoint. For example, + - AAD: https://login.microsoftonline.com + - ADFS: https://adfs.redmond.azurestack.corp.microsoft.com/adfs + :param tenant_id: Tenant GUID, like 00000000-0000-0000-0000-000000000000. If unspecified, default to + 'organizations'. :param client_id: Client ID of the CLI application. """ self.authority = authority - self.tenant_id = tenant_id or "organizations" - # Build the authority in MSAL style, like https://login.microsoftonline.com/your_tenant - self.msal_authority = "{}/{}".format(self.authority, self.tenant_id) + self.tenant_id = tenant_id self.client_id = client_id or AZURE_CLI_CLIENT_ID + # Build the authority in MSAL style + self.msal_authority = _get_authority_url(authority, tenant_id) + config_dir = get_config_dir() self._token_cache_file = os.path.join(config_dir, "msal_token_cache") self._secret_file = os.path.join(config_dir, "service_principal_entries") @@ -108,15 +111,15 @@ def msal_app(self): def login_with_auth_code(self, scopes=None, **kwargs): # Emit a warning to inform that a browser is opened. # Only show the path part of the URL and hide the query string. - logger.warning("The default web browser has been opened at %s/oauth2/v2.0/authorize. " - "Please continue the login in the web browser. " + logger.warning("The default web browser has been opened at %s. Please continue the login in the web browser. " "If no web browser is available or if the web browser fails to open, use device code flow " "with `az login --use-device-code`.", self.msal_authority) success_template, error_template = _read_response_templates() result = self.msal_app.acquire_token_interactive( - scopes, prompt='select_account', success_template=success_template, error_template=error_template, **kwargs) + scopes, prompt='select_account', port=8400, + success_template=success_template, error_template=error_template, **kwargs) return check_result(result) def login_with_device_code(self, scopes=None, **kwargs): @@ -323,3 +326,22 @@ def _read_response_templates(): error_template = f.read() return success_template, error_template + + +def _get_authority_url(authority_endpoint, tenant): + """Convert authority endpoint (active_directory) to MSAL authority: + - AAD: https://login.microsoftonline.com/your_tenant + - ADFS: https://adfs.redmond.azurestack.corp.microsoft.com/adfs + For ADFS, tenant is discarded. + """ + + # Some Azure Stack (bellevue)'s metadata returns + # "loginEndpoint": "https://login.microsoftonline.com/" + # Normalize it by removing the trailing /, so that authority_url won't become + # "https://login.microsoftonline.com//tenant_id". + authority_endpoint = authority_endpoint.rstrip('/').lower() + if authority_endpoint.endswith('adfs'): + authority_url = authority_endpoint + else: + authority_url = '{}/{}'.format(authority_endpoint, tenant or "organizations") + return authority_url diff --git a/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py b/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py index e38fd8d9eb5..07fbf26a4aa 100644 --- a/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py @@ -7,7 +7,8 @@ import unittest from unittest import mock -from azure.cli.core.auth.identity import Identity, ServicePrincipalAuth, ServicePrincipalStore +from azure.cli.core.auth.identity import (Identity, ServicePrincipalAuth, ServicePrincipalStore, + _get_authority_url) from knack.util import CLIError @@ -15,7 +16,7 @@ class TestIdentity(unittest.TestCase): def test_login_with_service_principal_certificate_cert_err(self): import os - identity = Identity() + identity = Identity('https://login.microsoftonline.com') current_dir = os.path.dirname(os.path.realpath(__file__)) test_cert_file = os.path.join(current_dir, 'err_sp_cert.pem') @@ -139,6 +140,34 @@ def test_remove_entry(self, load_secret_store_mock): assert store._content == [] +class TestUtils(unittest.TestCase): + + def test_get_authority_url(self): + # AAD + # Default tenant + self.assertEqual(_get_authority_url('https://login.microsoftonline.com', None), + 'https://login.microsoftonline.com/organizations') + # Trailing slash is stripped + self.assertEqual(_get_authority_url('https://login.microsoftonline.com/', None), + 'https://login.microsoftonline.com/organizations') + # Custom tenant + self.assertEqual(_get_authority_url('https://login.microsoftonline.com', + '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a'), + 'https://login.microsoftonline.com/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a') + + # ADFS + # Default tenant + self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', None), + 'https://adfs.redmond.azurestack.corp.microsoft.com/adfs') + # Trailing slash is stripped + self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs/', None), + 'https://adfs.redmond.azurestack.corp.microsoft.com/adfs') + # Tenant ID is discarded + self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', + '601d729d-0000-0000-0000-000000000000'), + 'https://adfs.redmond.azurestack.corp.microsoft.com/adfs') + + class MemoryStore: def __init__(self): diff --git a/src/azure-cli-core/azure/cli/core/auth/util.py b/src/azure-cli-core/azure/cli/core/auth/util.py index 5d79080b907..6bf15477ad2 100644 --- a/src/azure-cli-core/azure/cli/core/auth/util.py +++ b/src/azure-cli-core/azure/cli/core/auth/util.py @@ -123,11 +123,11 @@ def check_result(result, **kwargs): # For user authentication if 'id_token_claims' in result: - idt = result['id_token_claims'] + id_token = result['id_token_claims'] return { # AAD returns "preferred_username", ADFS returns "upn" - 'username': idt.get("preferred_username") or idt["upn"], - 'tenant_id': idt['tid'] + 'username': id_token.get("preferred_username") or id_token["upn"], + 'tenant_id': id_token['tid'] } return None diff --git a/src/azure-cli-core/azure/cli/core/tests/test_profile.py b/src/azure-cli-core/azure/cli/core/tests/test_profile.py index 3b5b5abdd01..a396f02296c 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_profile.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_profile.py @@ -16,8 +16,7 @@ from azure.core.credentials import AccessToken -from azure.cli.core._profile import (Profile, SubscriptionFinder, - _detect_adfs_authority, _attach_token_tenant, +from azure.cli.core._profile import (Profile, SubscriptionFinder, _attach_token_tenant, _transform_subscription_for_multiapi) from azure.mgmt.resource.subscriptions.models import \ @@ -1417,29 +1416,6 @@ def __init__(self, tenant_id, display_name="DISPLAY_NAME"): class TestUtils(unittest.TestCase): - def test_detect_adfs_authority(self): - # Public cloud - # Default tenant - self.assertEqual(_detect_adfs_authority('https://login.microsoftonline.com', None), - ('https://login.microsoftonline.com', None)) - # Trailing slash is stripped - self.assertEqual(_detect_adfs_authority('https://login.microsoftonline.com/', None), - ('https://login.microsoftonline.com', None)) - # Custom tenant - self.assertEqual(_detect_adfs_authority('https://login.microsoftonline.com', '601d729d-0000-0000-0000-000000000000'), - ('https://login.microsoftonline.com', '601d729d-0000-0000-0000-000000000000')) - - # ADFS - # Default tenant - self.assertEqual(_detect_adfs_authority('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', None), - ('https://adfs.redmond.azurestack.corp.microsoft.com', 'adfs')) - # Trailing slash is stripped - self.assertEqual(_detect_adfs_authority('https://adfs.redmond.azurestack.corp.microsoft.com/adfs/', None), - ('https://adfs.redmond.azurestack.corp.microsoft.com', 'adfs')) - # Tenant ID is discarded - self.assertEqual(_detect_adfs_authority('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', '601d729d-0000-0000-0000-000000000000'), - ('https://adfs.redmond.azurestack.corp.microsoft.com', 'adfs')) - def test_attach_token_tenant(self): from azure.mgmt.resource.subscriptions.v2016_06_01.models import Subscription \ as Subscription_v2016_06_01 From 1db68f23a7cdd89e44540ff9198906cf701745f0 Mon Sep 17 00:00:00 2001 From: jiasli <4003950+jiasli@users.noreply.github.com> Date: Fri, 15 Oct 2021 10:13:02 +0800 Subject: [PATCH 2/3] logout --- .../azure/cli/core/auth/identity.py | 21 +++++++++++-------- .../azure/cli/core/auth/persistence.py | 6 ++++-- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/auth/identity.py b/src/azure-cli-core/azure/cli/core/auth/identity.py index 2d6afa8c34a..72c4cc51719 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -13,6 +13,7 @@ from .msal_authentication import UserCredential, ServicePrincipalCredential from .util import check_result +from .persistence import load_persisted_token_cache, file_extensions # Service principal entry properties from .msal_authentication import _CLIENT_ID, _TENANT, _CLIENT_SECRET, _CERTIFICATE, _CLIENT_ASSERTION,\ @@ -74,7 +75,6 @@ def __init__(self, authority, tenant_id=None, client_id=None): } def _load_msal_cache(self): - from .persistence import load_persisted_token_cache # Store for user token persistence cache = load_persisted_token_cache(self._token_cache_file, self.token_encryption) return cache @@ -162,10 +162,8 @@ def logout_user(self, user): self.msal_app.remove_account(account) def logout_all_users(self): - try: - os.remove(self._token_cache_file) - except FileNotFoundError: - pass + for e in file_extensions.values(): + _try_remove(self._token_cache_file + e) def logout_service_principal(self, sp): # remove service principal secrets @@ -303,10 +301,8 @@ def remove_entry(self, sp_id): self._save_persistence() def remove_all_entries(self): - try: - os.remove(self._secret_file) - except FileNotFoundError: - pass + for e in file_extensions.values(): + _try_remove(self._secret_file + e) def _save_persistence(self): self._secret_store.save(self._entries) @@ -345,3 +341,10 @@ def _get_authority_url(authority_endpoint, tenant): else: authority_url = '{}/{}'.format(authority_endpoint, tenant or "organizations") return authority_url + + +def _try_remove(path): + try: + os.remove(path) + except FileNotFoundError: + pass diff --git a/src/azure-cli-core/azure/cli/core/auth/persistence.py b/src/azure-cli-core/azure/cli/core/auth/persistence.py index 1620abf892f..560f7b4e78f 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -18,6 +18,9 @@ logger = get_logger(__name__) +# Files extensions for encrypted and plaintext persistence +file_extensions = {True: '.bin', False: '.json'} + def load_persisted_token_cache(location, encrypt): persistence = build_persistence(location, encrypt) @@ -31,8 +34,8 @@ def load_secret_store(location, encrypt): def build_persistence(location, encrypt): """Build a suitable persistence instance based your current OS""" + location += file_extensions[encrypt] if encrypt: - location += '.bin' if sys.platform.startswith('win'): return FilePersistenceWithDataProtection(location) if sys.platform.startswith('darwin'): @@ -44,7 +47,6 @@ def build_persistence(location, encrypt): attributes={"my_attr1": "foo", "my_attr2": "bar"} ) else: - location += '.json' return FilePersistence(location) From a8116de3f69cf8aa76a5e6fd19a55698a87e1b9d Mon Sep 17 00:00:00 2001 From: jiasli <4003950+jiasli@users.noreply.github.com> Date: Fri, 15 Oct 2021 10:55:36 +0800 Subject: [PATCH 3/3] port 8400 --- .../azure/cli/core/auth/identity.py | 15 +++++++++------ .../azure/cli/core/auth/tests/test_identity.py | 13 +++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/auth/identity.py b/src/azure-cli-core/azure/cli/core/auth/identity.py index 72c4cc51719..907a031b231 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -53,7 +53,7 @@ def __init__(self, authority, tenant_id=None, client_id=None): self.client_id = client_id or AZURE_CLI_CLIENT_ID # Build the authority in MSAL style - self.msal_authority = _get_authority_url(authority, tenant_id) + self._msal_authority, self._is_adfs = _get_authority_url(authority, tenant_id) config_dir = get_config_dir() self._token_cache_file = os.path.join(config_dir, "msal_token_cache") @@ -69,7 +69,7 @@ def __init__(self, authority, tenant_id=None, client_id=None): # Store for Service principal credential persistence self._msal_secret_store = ServicePrincipalStore(self._secret_file, self.token_encryption) self._msal_app_kwargs = { - "authority": self.msal_authority, + "authority": self._msal_authority, "token_cache": self._load_msal_cache() # "http_cache": Identity.http_cache } @@ -113,12 +113,14 @@ def login_with_auth_code(self, scopes=None, **kwargs): # Only show the path part of the URL and hide the query string. logger.warning("The default web browser has been opened at %s. Please continue the login in the web browser. " "If no web browser is available or if the web browser fails to open, use device code flow " - "with `az login --use-device-code`.", self.msal_authority) + "with `az login --use-device-code`.", self._msal_authority) success_template, error_template = _read_response_templates() + # For AAD, use port 0 to let the system choose arbitrary unused ephemeral port to avoid port collision + # on port 8400 from the old design. However, ADFS only allows port 8400. result = self.msal_app.acquire_token_interactive( - scopes, prompt='select_account', port=8400, + scopes, prompt='select_account', port=8400 if self._is_adfs else None, success_template=success_template, error_template=error_template, **kwargs) return check_result(result) @@ -336,11 +338,12 @@ def _get_authority_url(authority_endpoint, tenant): # Normalize it by removing the trailing /, so that authority_url won't become # "https://login.microsoftonline.com//tenant_id". authority_endpoint = authority_endpoint.rstrip('/').lower() - if authority_endpoint.endswith('adfs'): + is_adfs = authority_endpoint.endswith('adfs') + if is_adfs: authority_url = authority_endpoint else: authority_url = '{}/{}'.format(authority_endpoint, tenant or "organizations") - return authority_url + return authority_url, is_adfs def _try_remove(path): diff --git a/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py b/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py index 07fbf26a4aa..81995f10bbb 100644 --- a/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/tests/test_identity.py @@ -146,26 +146,27 @@ def test_get_authority_url(self): # AAD # Default tenant self.assertEqual(_get_authority_url('https://login.microsoftonline.com', None), - 'https://login.microsoftonline.com/organizations') + ('https://login.microsoftonline.com/organizations', False)) # Trailing slash is stripped self.assertEqual(_get_authority_url('https://login.microsoftonline.com/', None), - 'https://login.microsoftonline.com/organizations') + ('https://login.microsoftonline.com/organizations', False)) # Custom tenant self.assertEqual(_get_authority_url('https://login.microsoftonline.com', '54826b22-38d6-4fb2-bad9-b7b93a3e9c5a'), - 'https://login.microsoftonline.com/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a') + ('https://login.microsoftonline.com/54826b22-38d6-4fb2-bad9-b7b93a3e9c5a', False)) # ADFS # Default tenant + adfs_expected = ('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', True) self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', None), - 'https://adfs.redmond.azurestack.corp.microsoft.com/adfs') + adfs_expected) # Trailing slash is stripped self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs/', None), - 'https://adfs.redmond.azurestack.corp.microsoft.com/adfs') + adfs_expected) # Tenant ID is discarded self.assertEqual(_get_authority_url('https://adfs.redmond.azurestack.corp.microsoft.com/adfs', '601d729d-0000-0000-0000-000000000000'), - 'https://adfs.redmond.azurestack.corp.microsoft.com/adfs') + adfs_expected) class MemoryStore: