diff --git a/src/azure-cli-core/azure/cli/core/_profile.py b/src/azure-cli-core/azure/cli/core/_profile.py index afd66504e2b..055f527a202 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..a3ce08fc221 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,12 @@ 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) + # MSAL only removes the accounts it knows about, and on Linux and macOS the credential + # lives in the OS keychain, so the payload has to be emptied, not just deleted. + # Every auth type has a token cache, but only service principals have a secret store, so + # this is the one location that warns about what the keychain may still hold. + erase_persistence(self._token_cache_file, self._encrypt, type="Token cache", + empty_payload='{}', warn_if_credentials_may_remain=True) def logout_service_principal(self, client_id): # If client_id is a username, it is ignored @@ -226,11 +229,10 @@ def logout_service_principal(self, client_id): self._service_principal_store.remove_entry(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..724c2f000c1 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,162 @@ 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. " + "Learn more: https://aka.ms/azure-cli-credential-encryption to enable encryption.") + +# Credentials left in the OS credential store by an earlier encrypted run, which a clear cannot +# reach. Which one applies depends on why this run is not using the store. +CREDENTIAL_STORE_UNAVAILABLE_WARNING = ( + "Credentials may remain in the OS credential store, which is currently unavailable. " + "Clear again once it works.") +CREDENTIAL_STORE_NOT_CLEARED_WARNING = ( + "Credentials may remain in the OS credential store. It is not cleared because encryption is " + "off, and clearing it would prompt to unlock the keyring. Set 'core.encrypt_token_cache' to " + "true and clear again to remove them.") + +# 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"} - ) + # 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 {} + label = f"{LIBSECRET_SCHEMA_NAME} - {type}" if type else LIBSECRET_SCHEMA_NAME + return LibsecretPersistence( + path, + schema_name=LIBSECRET_SCHEMA_NAME, + attributes=attributes, + label=label + ) + 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, keep_signal_file=False): + # Every extension, not just the one in use, to clean up after a changed encrypt_token_cache. + for extension in file_extensions: + if keep_signal_file and extension == file_extension_signal: + continue + _try_remove(location + extension) + + +def _warn_about_the_os_credential_store(location): + # A signal file means this location was written with encryption on, so the OS credential store + # may still hold a payload. Emptying it here may prompt to unlock the keyring, which is the + # interruption encrypt_token_cache=false opted out of, so leave the payload alone and say so. + if not os.path.exists(location + file_extension_signal): + return + if _encryption_fallback: + # Encryption is already on, so there is nothing to turn on. The keyring is what's broken. + logger.warning(CREDENTIAL_STORE_UNAVAILABLE_WARNING) else: - return FilePersistence(location) + logger.warning(CREDENTIAL_STORE_NOT_CLEARED_WARNING) + + +def erase_persistence(location, encrypt, type=None, empty_payload='{}', # pylint: disable=redefined-builtin + warn_if_credentials_may_remain=False): + """Empty a persisted payload and remove its files. Returns whether it succeeded. + + With encryption on, the payload is held by libsecret or Keychain and the file is only a + modification signal, so it is overwritten rather than deleted. With encryption off the + credential store is not touched at all, so clearing never prompts to unlock a keyring the user + opted out of, and the signal file is kept as the evidence that it may still hold a payload. + + :param warn_if_credentials_may_remain: Warn about what the credential store may still hold. + Only one location should, because the warning is about the store, not the location. + """ + try: + persistence = build_persistence(location, encrypt, type=type) + # Serialize against other az processes, like SecretStore.save and PersistedTokenCache do. + with CrossPlatLock(persistence.get_location() + '.lockfile'): + if persistence.is_encrypted: + persistence.save(empty_payload) + elif warn_if_credentials_may_remain: + _warn_about_the_os_credential_store(location) + _remove_persistence_files(location, keep_signal_file=not persistence.is_encrypted) + return True + except Exception as e: # pylint: disable=broad-except + # Clearing must not fail, but nothing was removed and the credential is still readable, so + # this can't be silent. In practice another az process is holding the lock. + 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 not _encryption_fallback: + return + + # Nothing can be installed on Cloud Shell's machine, so the advice would only be noise. + from azure.cli.core.util import in_cloud_console + if in_cloud_console(): + logger.debug("Encryption is unavailable, but the warning is suppressed in Cloud Shell.") + return + + 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..06307f8714f --- /dev/null +++ b/src/azure-cli-core/azure/cli/core/auth/tests/test_persistence.py @@ -0,0 +1,297 @@ +# -------------------------------------------------------------------------------------------- +# 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 TestLibsecretLabel(unittest.TestCase): + + @staticmethod + def _build(type): + with mock.patch.object(persistence.sys, 'platform', 'linux'), \ + mock.patch.object(persistence, 'LibsecretPersistence') as libsecret_mock: + persistence.build_persistence('/tmp/test_persistence', True, type=type) + return libsecret_mock.call_args.kwargs + + def test_label_names_the_store(self): + # Without it libsecret stores an empty label, which shows as a blank row in keyring viewers. + self.assertEqual(self._build('Token cache')['label'], 'Microsoft Azure CLI - Token cache') + self.assertEqual(self._build('Secret store')['label'], 'Microsoft Azure CLI - Secret store') + + def test_label_falls_back_to_the_schema_name(self): + self.assertEqual(self._build(None)['label'], 'Microsoft Azure CLI') + + +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_fallback_reason_goes_to_the_debug_log(self): + # The warning says the credentials are in plaintext; only this says what went wrong, and + # it is all a user has to go on when the keyring is meant to be working. + with mock.patch.object(persistence.logger, 'debug') as debug_mock: + self._build_with_libsecret_unavailable() + + logged = ' '.join(str(call) for call in debug_mock.call_args_list) + self.assertIn('Failed to initialize LibsecretPersistence', logged) + self.assertIn('no libsecret', logged) + + 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() + + # A CI agent gets the warning too, so clear the environment to keep az's own CI honest. + with mock.patch.dict(os.environ, {}, clear=True), \ + 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_warning_shown_on_a_ci_agent(self): + # A CI agent is someone's own machine to configure, unlike Cloud Shell. + for name, value in [('GITHUB_ACTIONS', 'true'), ('TF_BUILD', 'True')]: + with self.subTest(variable=name): + self._build_with_libsecret_unavailable() + + with mock.patch.dict(os.environ, {name: value}, clear=True), \ + 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_in_cloud_shell(self): + # Cloud Shell can't have an OS credential store installed, so the advice to enable + # encryption is unactionable there. + self._build_with_libsecret_unavailable() + + with mock.patch.dict(os.environ, {'ACC_CLOUD': 'PROD'}, clear=True), \ + mock.patch.object(persistence.logger, 'warning') as warning_mock: + persistence.warn_if_encryption_unavailable() + + warning_mock.assert_not_called() + + def test_no_warning_without_fallback(self): + with mock.patch.dict(os.environ, {}, clear=True), \ + 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 TestPlaintextClear(unittest.TestCase): + """With encryption off, clearing must not touch the OS credential store. + + Opting out of core.encrypt_token_cache opts out of the keyring prompt, and saving an empty + payload to libsecret or Keychain is what would raise it. The signal file stays behind, so a + later clear with encryption on still finds the payload. + """ + + LOCATION = '/tmp/test_persistence' + + def setUp(self): + persistence._encryption_fallback = False + self.addCleanup(setattr, persistence, '_encryption_fallback', False) + + def _erase(self, signal_file_exists=False, warn_if_credentials_may_remain=False): + built = mock.MagicMock() + built.is_encrypted = False + built.get_location.return_value = self.LOCATION + persistence.file_extension_plaintext + with mock.patch.object(persistence, 'build_persistence', return_value=built), \ + mock.patch.object(persistence, 'CrossPlatLock'), \ + mock.patch.object(persistence.os.path, 'exists', return_value=signal_file_exists), \ + mock.patch.object(persistence, '_try_remove') as remove_mock, \ + mock.patch.object(persistence.logger, 'warning') as warning_mock: + result = persistence.erase_persistence( + self.LOCATION, False, type='Token cache', + warn_if_credentials_may_remain=warn_if_credentials_may_remain) + return result, built, remove_mock, warning_mock + + def test_credential_store_is_left_alone(self): + result, built, _, _ = self._erase() + + self.assertTrue(result) + built.save.assert_not_called() + + def test_signal_file_is_kept(self): + # Removing it would orphan the payload the clear could not reach. + _, _, remove_mock, _ = self._erase() + + self.assertNotIn(mock.call(self.LOCATION + persistence.file_extension_signal), + remove_mock.call_args_list) + + def test_the_other_files_are_removed(self): + # Including a .bin written while encryption was on. + _, _, remove_mock, _ = self._erase() + + for extension in (persistence.file_extension_plaintext, persistence.file_extension_encrypted): + self.assertIn(mock.call(self.LOCATION + extension), remove_mock.call_args_list) + + def test_a_leftover_signal_file_warns_about_the_credential_store(self): + _, _, _, warning_mock = self._erase(signal_file_exists=True, + warn_if_credentials_may_remain=True) + + warning_mock.assert_called_once() + message = warning_mock.call_args[0][0] + self.assertIn('OS credential store', message) + self.assertIn('core.encrypt_token_cache', message) + + def test_only_the_asked_location_warns(self): + # az account clear erases two locations, but the warning is about the store, not the file. + _, _, _, warning_mock = self._erase(signal_file_exists=True) + + warning_mock.assert_not_called() + + def test_no_warning_when_encryption_was_never_used(self): + _, _, _, warning_mock = self._erase(signal_file_exists=False, + warn_if_credentials_may_remain=True) + + warning_mock.assert_not_called() + + def test_a_fallback_is_not_told_to_enable_encryption(self): + # Encryption is already on here; the keyring is what is broken. + persistence._encryption_fallback = True + _, _, _, warning_mock = self._erase(signal_file_exists=True, + warn_if_credentials_may_remain=True) + + message = warning_mock.call_args[0][0] + self.assertIn('OS credential store', message) + self.assertNotIn('core.encrypt_token_cache', message) + + def test_encryption_on_erases_and_removes_everything(self): + built = mock.MagicMock() + built.is_encrypted = True + built.get_location.return_value = self.LOCATION + persistence.file_extension_signal + with mock.patch.object(persistence, 'build_persistence', return_value=built), \ + mock.patch.object(persistence, 'CrossPlatLock'), \ + mock.patch.object(persistence.os.path, 'exists', return_value=True), \ + mock.patch.object(persistence, '_try_remove') as remove_mock, \ + mock.patch.object(persistence.logger, 'warning') as warning_mock: + persistence.erase_persistence(self.LOCATION, True, type='Token cache', + warn_if_credentials_may_remain=True) + + built.save.assert_called_once() + warning_mock.assert_not_called() + self.assertEqual( + [mock.call(self.LOCATION + e) for e in persistence.file_extensions], + remove_mock.call_args_list) + + def test_only_the_signal_file_is_left_behind(self): + # End to end with a real _try_remove, whichever setting wrote the files. + with tempfile.TemporaryDirectory() as directory: + location = os.path.join(directory, 'msal_token_cache') + for extension in persistence.file_extensions: + open(location + extension, 'w').close() # pylint: disable=consider-using-with + + with mock.patch.object(persistence.sys, 'platform', 'linux'): + result = persistence.erase_persistence(location, False, type='Token cache') + + self.assertTrue(result) + left = sorted(f for f in os.listdir(directory) if not f.endswith('.lockfile')) + self.assertEqual(['msal_token_cache' + persistence.file_extension_signal], left) + + +class TestErasePersistence(unittest.TestCase): + """With encryption on, clearing 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.is_encrypted = True + 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.is_encrypted = True + 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.is_encrypted = True + 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/tests/test_util.py b/src/azure-cli-core/azure/cli/core/tests/test_util.py index 776d2c66739..fd319bda619 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_util.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_util.py @@ -585,6 +585,28 @@ def test_getprop(self): self.assertEqual(getprop(self, 'new_props', "new_props"), "new_props") +class TestShouldEncryptTokenCache(unittest.TestCase): + """The default is what a user who never touched the config runs with, so it is pinned here. + + The live encryption tests all set core.encrypt_token_cache or its environment override, so + flipping the fallback back to plaintext would not fail any of them. + """ + + def test_encryption_is_on_by_default(self): + from azure.cli.core.util import should_encrypt_token_cache + cli = DummyCli() + # Nothing configured: the value the caller passes as fallback is what takes effect. + with mock.patch.object(cli.config, 'getboolean', + side_effect=lambda section, option, fallback=None: fallback): + self.assertTrue(should_encrypt_token_cache(cli)) + + def test_config_can_turn_encryption_off(self): + from azure.cli.core.util import should_encrypt_token_cache + cli = DummyCli() + with mock.patch.object(cli.config, 'getboolean', return_value=False): + self.assertFalse(should_encrypt_token_cache(cli)) + + class TestHandleException(unittest.TestCase): @mock.patch('azure.cli.core.azclierror.logger.error', autospec=True) diff --git a/src/azure-cli-core/azure/cli/core/util.py b/src/azure-cli-core/azure/cli/core/util.py index 0efad6ab8ad..419cb452b83 100644 --- a/src/azure-cli-core/azure/cli/core/util.py +++ b/src/azure-cli-core/azure/cli/core/util.py @@ -1624,9 +1624,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)