diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index 13f47ed417c..6f55f4c3983 100644 --- a/src/azure-cli-core/azure/cli/core/_profile.py +++ b/src/azure-cli-core/azure/cli/core/_profile.py @@ -183,7 +183,11 @@ def login(self, else: identity.login_with_service_principal(username, password, scopes=scopes) - # We have finished login. Let's find all subscriptions. + # We have finished login. Warn once here if credentials fell back to plaintext. + from .auth.persistence import warn_if_encryption_unavailable + warn_if_encryption_unavailable() + + # Let's find all subscriptions. if show_progress: message = ('Retrieving subscriptions for the selection...' if tenant else 'Retrieving tenants and subscriptions for the selection...') 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 91629e89441..a293e13bfc4 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -15,7 +15,7 @@ from .constants import AZURE_CLI_CLIENT_ID from .msal_credentials import UserCredential, ServicePrincipalCredential -from .persistence import load_persisted_token_cache, file_extensions, load_secret_store +from .persistence import load_persisted_token_cache, load_secret_store, erase_persistence from .util import check_result # Service principal entry properties. Names are taken from OAuth 2.0 client credentials flow parameters: @@ -210,9 +210,11 @@ def logout_all_users(self): for account in accounts: self._msal_app.remove_account(account) - # Also remove token cache file - for e in file_extensions.values(): - _try_remove(self._token_cache_file + e) + # Empty the payload, then remove the files. MSAL only removes the accounts it knows about, + # and on Linux and macOS the credential lives in the OS keychain, where removing the + # signal file would leave it behind. + erase_persistence(self._token_cache_file, self._encrypt, type="Token cache", + empty_payload='{}') def logout_service_principal(self, client_id): # If client_id is a username, it is ignored @@ -227,10 +229,10 @@ def logout_service_principal(self, client_id): def logout_all_service_principal(self): # remove service principal secrets - # TODO: As MSAL provides no interface to get all service principals in its token cache, this method can't - # clear all service principals' access tokens from MSAL token cache. - for e in file_extensions.values(): - _try_remove(self._secret_file + e) + # MSAL provides no interface to enumerate the service principals in its token cache, so + # their access tokens are cleared by logout_all_users emptying the whole sp secret store + erase_persistence(self._secret_file, self._encrypt, type="Secret store", + empty_payload='[]') def get_user(self, user=None): accounts = self._msal_app.get_accounts(user) if user else self._msal_app.get_accounts() @@ -431,13 +433,6 @@ def _get_authority_url(authority_endpoint, tenant): return authority_url, is_adfs -def _try_remove(path): - try: - os.remove(path) - except FileNotFoundError: - pass - - def get_environment_credential(): # A temporary workaround used by rdbms module to use environment credential. # TODO: Integrate with Identity and utilize MSAL HTTP and token cache to officially implement 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 eb51a82660c..a80d5ef744a 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -7,6 +7,7 @@ # https://github.com/AzureAD/microsoft-authentication-extensions-for-python/blob/dev/sample/token_cache_sample.py import json +import os import sys from msal_extensions import (FilePersistenceWithDataProtection, KeychainPersistence, LibsecretPersistence, @@ -20,36 +21,158 @@ logger = get_logger(__name__) # Files extensions for encrypted and plaintext persistence -file_extensions = {True: '.bin', False: '.json'} +file_extension_encrypted = '.bin' +file_extension_plaintext = '.json' +file_extension_signal = '.sig' +file_extensions = [file_extension_encrypted, file_extension_plaintext, file_extension_signal] + +KEYCHAIN_SERVICE_NAME = 'Microsoft Azure CLI' +LIBSECRET_SCHEMA_NAME = 'Microsoft Azure CLI' + +ENCRYPTION_FALLBACK_WARNING = ( + "Encryption is unavailable on this machine, so the token cache and service principal secrets " + "are stored in plaintext. " + "Please follow https://aka.ms/azure-cli-credential-encryption to enable encryption.") + +# Set when a persistence falls back to plaintext, so sign-in can warn about it. +_encryption_fallback = False def load_persisted_token_cache(location, encrypt): - persistence = build_persistence(location, encrypt) + persistence = build_persistence(location, encrypt, type="Token cache") return PersistedTokenCache(persistence) def load_secret_store(location, encrypt): - persistence = build_persistence(location, encrypt) + persistence = build_persistence(location, encrypt, type="Secret store") return SecretStore(persistence) -def build_persistence(location, encrypt): +def build_persistence(location, encrypt, type=None): # pylint: disable=redefined-builtin """Build a suitable persistence instance based your current OS""" - location += file_extensions[encrypt] - logger.debug("build_persistence: location=%r, encrypt=%r", location, encrypt) + logger.debug("build_persistence: location=%r, encrypt=%r, type=%r", location, encrypt, type) if encrypt: if sys.platform.startswith('win'): - return FilePersistenceWithDataProtection(location) + # For FilePersistenceWithDataProtection, location is where the credential is stored. + path = location + file_extension_encrypted + logger.debug("Initializing FilePersistenceWithDataProtection: location=%r", path) + return FilePersistenceWithDataProtection(path) if sys.platform.startswith('darwin'): - return KeychainPersistence(location, "my_service_name", "my_account_name") + # For KeychainPersistence, location is only used as a signal for the credential's last modified time. + # The credential is stored in Keychain identified by (service_name, account_name) combination. + # msal-extensions automatically computes account_name from signal_location. + # https://github.com/AzureAD/microsoft-authentication-extensions-for-python/pull/103 + path = location + file_extension_signal + logger.debug("Initializing KeychainPersistence: location=%r", path) + return KeychainPersistence(path, service_name=KEYCHAIN_SERVICE_NAME, account_name=type) if sys.platform.startswith('linux'): - return LibsecretPersistence( - location, - schema_name="my_schema_name", - attributes={"my_attr1": "foo", "my_attr2": "bar"} - ) - else: - return FilePersistence(location) + # For LibsecretPersistence, location is only used as a signal for the credential's last modified time. + # The credential is stored in libsecret identified by (schema_name, attributes) combination. + # Doesn't seem to be a reason to use attributes to further filter the credential. + path = location + file_extension_signal + logger.debug("Initializing LibsecretPersistence: location=%r", path) + try: + attributes = {"type": type} if type else {} + return LibsecretPersistence( + path, + schema_name=LIBSECRET_SCHEMA_NAME, + attributes=attributes + ) + except Exception as e: # pylint: disable=broad-except + # LibsecretPersistence is known to be unavailable in some Linux environments. + # Fall back to FilePersistence. The user is warned at sign-in. + logger.debug("Failed to initialize LibsecretPersistence: %s", e) + _record_encryption_fallback() + # Either encryption is opted out or the OS is not supported for encryption. Use FilePersistence. + path = location + file_extension_plaintext + logger.debug("Initializing FilePersistence: location=%r", path) + return FilePersistence(path) + + +def _record_encryption_fallback(): + global _encryption_fallback # pylint: disable=global-statement + _encryption_fallback = True + + +def _try_remove(path): + try: + os.remove(path) + except FileNotFoundError: + pass + except OSError as e: + logger.debug("Failed to remove %r: %s", path, e) + + +def _remove_persistence_files(location): + """Remove every persistence file for a location. + + All extensions are tried, not just the one in use, so that a plaintext or DPAPI file left + over from a previous core.encrypt_token_cache setting is cleaned up too. + """ + for extension in file_extensions: + _try_remove(location + extension) + + +def _try_erase_os_credential_store(location, type=None, empty_payload='{}'): # pylint: disable=redefined-builtin + """Best effort erase of a payload the OS credential store may still hold. + + On macOS and Linux the credential lives in Keychain or libsecret and the file is only a + modification signal, so removing the file hides the payload instead of removing it: a clear run + with core.encrypt_token_cache off would leave the credential readable as soon as the setting is + turned back on. Windows needs nothing here, because the DPAPI file is the ciphertext itself. + + The persistence API has no delete, and macOS Keychain offers none at all, so an empty payload is + written over it instead. Like _try_remove, a failure must not stop the clear: an unreachable + credential store cannot be cleared by any means, so there is nothing to do but record it. + """ + if not (sys.platform.startswith('darwin') or sys.platform.startswith('linux')): + return + try: + persistence = build_persistence(location, True, type=type) + if not persistence.is_encrypted: + # Fell back to plaintext, so the OS credential store is unreachable from here. The + # plaintext file is the caller's business and is dealt with there. + return + with CrossPlatLock(persistence.get_location() + '.lockfile'): + persistence.save(empty_payload) + except Exception as e: # pylint: disable=broad-except + logger.debug("Failed to erase the OS credential store at %r: %s", location, e) + + +def erase_persistence(location, encrypt, type=None, empty_payload='{}'): # pylint: disable=redefined-builtin + """Empty a persisted payload and remove its files. Returns whether it succeeded. + + Removing the files is not enough on Linux and macOS: the credential is held by libsecret or + Keychain and the file is only a modification signal. Write through the same persistence that + reads it, the way logging out of a single account does. + + The OS credential store is erased too when it is not the configured one, so that a payload + written under a previous core.encrypt_token_cache setting does not outlive the clear. + + Emptying and removing happen under one lock, and nothing is touched unless the lock is held. + In practice this only fails when another az process is using the credential store, and + deleting its freshly written signal file would hide a credential rather than remove it. + """ + try: + persistence = build_persistence(location, encrypt, type=type) + if not persistence.is_encrypted: + _try_erase_os_credential_store(location, type=type, empty_payload=empty_payload) + # Serialize against other az processes, like SecretStore.save and PersistedTokenCache do. + with CrossPlatLock(persistence.get_location() + '.lockfile'): + persistence.save(empty_payload) + _remove_persistence_files(location) + return True + except Exception as e: # pylint: disable=broad-except + # Logging out must not fail, but nothing was cleared and the credential is still readable, + # so this can't be silent. Details go to the debug log. + logger.debug("Failed to erase persisted payload at %r: %s", location, e) + logger.warning("Could not clear credentials. Run 'az account clear' again.") + return False + + +def warn_if_encryption_unavailable(): + if _encryption_fallback: + logger.warning(ENCRYPTION_FALLBACK_WARNING) class SecretStore: diff --git a/src/azure-cli-core/azure/cli/core/auth/tests/test_persistence.py b/src/azure-cli-core/azure/cli/core/auth/tests/test_persistence.py new file mode 100644 index 00000000000..6e5316c0b5e --- /dev/null +++ b/src/azure-cli-core/azure/cli/core/auth/tests/test_persistence.py @@ -0,0 +1,262 @@ +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- + +import os +import tempfile +import unittest +from unittest import mock + +from azure.cli.core.auth import persistence + + +class TestEncryptionFallbackWarning(unittest.TestCase): + """The plaintext fallback warning is shown at sign-in, not by each persistence build.""" + + def setUp(self): + persistence._encryption_fallback = False + self.addCleanup(setattr, persistence, '_encryption_fallback', False) + + @staticmethod + def _build_with_libsecret_unavailable(): + with mock.patch.object(persistence.sys, 'platform', 'linux'), \ + mock.patch.object(persistence, 'LibsecretPersistence', side_effect=ImportError('no libsecret')): + return persistence.build_persistence('/tmp/test_persistence', True, type='Token cache') + + def test_fallback_is_silent_but_recorded(self): + with mock.patch.object(persistence.logger, 'warning') as warning_mock: + store = self._build_with_libsecret_unavailable() + + self.assertIsInstance(store, persistence.FilePersistence) + warning_mock.assert_not_called() + self.assertTrue(persistence._encryption_fallback) + + def test_warning_shown_at_sign_in(self): + # Token cache and secret store both fall back, but sign-in warns once. + self._build_with_libsecret_unavailable() + self._build_with_libsecret_unavailable() + + with mock.patch.object(persistence.logger, 'warning') as warning_mock: + persistence.warn_if_encryption_unavailable() + + warning_mock.assert_called_once_with(persistence.ENCRYPTION_FALLBACK_WARNING) + + def test_no_warning_without_fallback(self): + with mock.patch.object(persistence.logger, 'warning') as warning_mock: + persistence.warn_if_encryption_unavailable() + + warning_mock.assert_not_called() + + def test_no_warning_when_encryption_opted_out(self): + with mock.patch.object(persistence.sys, 'platform', 'linux'): + store = persistence.build_persistence('/tmp/test_persistence', False, type='Token cache') + + self.assertIsInstance(store, persistence.FilePersistence) + self.assertFalse(persistence._encryption_fallback) + + +class TestEraseOsCredentialStore(unittest.TestCase): + """A clear must not leave a credential in the OS credential store. + + On Linux and macOS the payload lives in libsecret or Keychain and the file is only a signal, so + a clear run with core.encrypt_token_cache off would remove the signal and leave the credential + readable as soon as the setting is turned back on. + """ + + LOCATION = '/tmp/test_persistence' + + @staticmethod + def _persistence_mock(is_encrypted, extension): + built = mock.MagicMock() + built.is_encrypted = is_encrypted + built.get_location.return_value = TestEraseOsCredentialStore.LOCATION + extension + return built + + def _erase(self, encrypt=False, platform='linux', os_store=None): + """Run a clear with the configured persistence and the OS credential store separated.""" + plaintext = self._persistence_mock(False, persistence.file_extension_plaintext) + encrypted = os_store if os_store is not None else \ + self._persistence_mock(True, persistence.file_extension_signal) + + def build(location, wants_encryption, type=None): # pylint: disable=redefined-builtin + return encrypted if wants_encryption else plaintext + + with mock.patch.object(persistence.sys, 'platform', platform), \ + mock.patch.object(persistence, 'build_persistence', side_effect=build), \ + mock.patch.object(persistence, 'CrossPlatLock'), \ + mock.patch.object(persistence, '_try_remove') as remove_mock, \ + mock.patch.object(persistence.logger, 'warning') as warning_mock: + result = persistence.erase_persistence(self.LOCATION, encrypt, type='Secret store', + empty_payload='[]') + return result, plaintext, encrypted, remove_mock, warning_mock + + def test_encrypted_payload_is_erased_when_encryption_is_off(self): + # The case a plain 'az account clear' used to miss entirely. + result, plaintext, encrypted, _, warning_mock = self._erase(encrypt=False) + + self.assertTrue(result) + plaintext.save.assert_called_once_with('[]') + encrypted.save.assert_called_once_with('[]') + warning_mock.assert_not_called() + + def test_configured_encrypted_store_is_erased_once(self): + # With encryption on, the configured persistence already is the OS credential store. + result, _, encrypted, _, warning_mock = self._erase(encrypt=True) + + self.assertTrue(result) + encrypted.save.assert_called_once_with('[]') + warning_mock.assert_not_called() + + def test_windows_needs_no_second_pass(self): + # The DPAPI file is the ciphertext, so removing the files is the whole of the erase. + result, plaintext, encrypted, _, warning_mock = self._erase(encrypt=False, platform='win32') + + self.assertTrue(result) + plaintext.save.assert_called_once_with('[]') + encrypted.save.assert_not_called() + warning_mock.assert_not_called() + + def test_unreachable_credential_store_is_skipped(self): + # Falling back means the keyring cannot be reached, so there is nothing that can be done + # about whatever it holds. The plaintext clear must still go ahead. + fallback = self._persistence_mock(False, persistence.file_extension_plaintext) + result, plaintext, _, remove_mock, warning_mock = self._erase(encrypt=False, os_store=fallback) + + self.assertTrue(result) + plaintext.save.assert_called_once_with('[]') + remove_mock.assert_called() + warning_mock.assert_not_called() + + def test_a_failing_credential_store_does_not_fail_the_clear(self): + # Best effort, like _try_remove: the files still have to go, and the caller still succeeds. + encrypted = self._persistence_mock(True, persistence.file_extension_signal) + encrypted.save.side_effect = Exception('keyring is locked') + with mock.patch.object(persistence.logger, 'debug') as debug_mock: + result, plaintext, _, remove_mock, warning_mock = self._erase( + encrypt=False, os_store=encrypted) + + self.assertTrue(result) + plaintext.save.assert_called_once_with('[]') + remove_mock.assert_called() + warning_mock.assert_not_called() + self.assertTrue(any('OS credential store' in str(call) for call in debug_mock.call_args_list)) + + def test_credential_store_is_erased_before_the_files_are_removed(self): + # Keychain and libsecret touch the signal file on save, so erasing after the removal would + # put the file back. The order is what keeps the clear from leaving one behind. + plaintext = self._persistence_mock(False, persistence.file_extension_plaintext) + encrypted = self._persistence_mock(True, persistence.file_extension_signal) + calls = [] + plaintext.save.side_effect = lambda _: calls.append('save plaintext') + encrypted.save.side_effect = lambda _: calls.append('save credential store') + + def build(location, wants_encryption, type=None): # pylint: disable=redefined-builtin + return encrypted if wants_encryption else plaintext + + with mock.patch.object(persistence.sys, 'platform', 'linux'), \ + mock.patch.object(persistence, 'build_persistence', side_effect=build), \ + mock.patch.object(persistence, 'CrossPlatLock'), \ + mock.patch.object(persistence, '_try_remove', + side_effect=lambda path: calls.append('remove')): + persistence.erase_persistence(self.LOCATION, False, type='Secret store') + + self.assertLess(calls.index('save credential store'), calls.index('remove')) + + def test_no_signal_file_is_left_behind(self): + # The failure the order above prevents, reproduced end to end: a real save touches the + # signal file, and a real _try_remove has to be the last thing that runs. + with tempfile.TemporaryDirectory() as directory: + location = os.path.join(directory, 'service_principal_entries') + plaintext = self._persistence_mock(False, persistence.file_extension_plaintext) + encrypted = self._persistence_mock(True, persistence.file_extension_signal) + plaintext.get_location.return_value = location + persistence.file_extension_plaintext + encrypted.get_location.return_value = location + persistence.file_extension_signal + + def touch(path): + return lambda _: open(path, 'w').close() # pylint: disable=consider-using-with + + plaintext.save.side_effect = touch(location + persistence.file_extension_plaintext) + encrypted.save.side_effect = touch(location + persistence.file_extension_signal) + + def build(location_, wants_encryption, type=None): # pylint: disable=redefined-builtin + return encrypted if wants_encryption else plaintext + + with mock.patch.object(persistence.sys, 'platform', 'linux'), \ + mock.patch.object(persistence, 'build_persistence', side_effect=build), \ + mock.patch.object(persistence, 'CrossPlatLock'): + persistence.erase_persistence(location, False, type='Secret store') + + self.assertEqual([], os.listdir(directory), 'the clear left a persistence file behind') + + +class TestErasePersistence(unittest.TestCase): + """Clearing all accounts must empty the payload, not just remove the files. + + On Linux and macOS the credential is held by libsecret or Keychain and the file is only a + modification signal, so removing files alone would leave the credential behind. + """ + + def test_payload_is_overwritten_through_the_persistence(self): + built = mock.MagicMock() + built.get_location.return_value = '/tmp/test_persistence.sig' + with mock.patch.object(persistence, 'build_persistence', return_value=built) as build_mock, \ + mock.patch.object(persistence, 'CrossPlatLock'), \ + mock.patch.object(persistence, '_try_remove'): + result = persistence.erase_persistence('/tmp/test_persistence', True, + type='Secret store', empty_payload='[]') + + self.assertTrue(result) + build_mock.assert_called_once_with('/tmp/test_persistence', True, type='Secret store') + built.save.assert_called_once_with('[]') + + def test_files_are_removed_under_the_same_lock_as_the_erase(self): + # A login landing between the erase and the removal would leave a credential in the OS + # credential store that the removed signal file no longer points at. + built = mock.MagicMock() + built.get_location.return_value = '/tmp/test_persistence.sig' + calls = [] + with mock.patch.object(persistence, 'build_persistence', return_value=built), \ + mock.patch.object(persistence, 'CrossPlatLock') as lock_mock, \ + mock.patch.object(persistence, '_try_remove') as remove_mock: + lock_mock.return_value.__enter__.side_effect = lambda: calls.append('lock') + lock_mock.return_value.__exit__.side_effect = lambda *a: calls.append('unlock') + built.save.side_effect = lambda _: calls.append('save') + remove_mock.side_effect = lambda path: calls.append('remove') + persistence.erase_persistence('/tmp/test_persistence', True, type='Token cache') + + lock_mock.assert_called_once_with('/tmp/test_persistence.sig.lockfile') + self.assertEqual(calls[0], 'lock') + self.assertEqual(calls[-1], 'unlock') + # The erase must precede the removal, or the save would recreate the removed files. + self.assertLess(calls.index('save'), calls.index('remove')) + self.assertEqual( + [mock.call('/tmp/test_persistence' + e) for e in persistence.file_extensions], + remove_mock.call_args_list) + + def test_failure_to_erase_warns_and_leaves_everything_alone(self): + # The realistic failure is another az process holding the lock. Removing the files it just + # wrote would hide its credential instead of removing it, so the clear is all-or-nothing + # and the user is told to retry. + built = mock.MagicMock() + built.get_location.return_value = '/tmp/test_persistence.sig' + built.save.side_effect = Exception('AlreadyLocked') + with mock.patch.object(persistence, 'build_persistence', return_value=built), \ + mock.patch.object(persistence, 'CrossPlatLock'), \ + mock.patch.object(persistence, '_try_remove') as remove_mock, \ + mock.patch.object(persistence.logger, 'warning') as warning_mock: + result = persistence.erase_persistence('/tmp/test_persistence', True, + type='Token cache') + + self.assertFalse(result) + warning_mock.assert_called_once() + remove_mock.assert_not_called() + + def test_removal_survives_a_locked_file(self): + # On Windows, removing a file another az process holds open raises a sharing violation. + with mock.patch.object(persistence.os, 'remove', side_effect=PermissionError('in use')): + persistence._try_remove('/tmp/test_persistence.bin') + + +if __name__ == '__main__': + unittest.main() diff --git a/src/azure-cli-core/azure/cli/core/util.py b/src/azure-cli-core/azure/cli/core/util.py index 67c7650f5fa..bd5749ba72b 100644 --- a/src/azure-cli-core/azure/cli/core/util.py +++ b/src/azure-cli-core/azure/cli/core/util.py @@ -1514,9 +1514,10 @@ def get_secret_store(cli_ctx, name): def should_encrypt_token_cache(cli_ctx): - # Only enable encryption for Windows (for now). - fallback = sys.platform.startswith('win32') + # Encryption enabled by default + fallback = True + # TODO: Remove the config and always enable encryption # EXPERIMENTAL: Use core.encrypt_token_cache=False to turn off token cache encryption. # encrypt_token_cache affects both MSAL token cache and service principal entries. encrypt = cli_ctx.config.getboolean('core', 'encrypt_token_cache', fallback=fallback)