From d6c3d50baa371b075e9a9d06dc70e748ab734282 Mon Sep 17 00:00:00 2001 From: isra-fel <11371776+isra-fel@users.noreply.github.com> Date: Tue, 4 Aug 2026 09:43:53 +1000 Subject: [PATCH 01/10] enabled encryption of token cache by default for macos and linux --- .../azure/cli/core/auth/persistence.py | 19 ++++++++++++++++--- src/azure-cli-core/azure/cli/core/util.py | 5 +++-- 2 files changed, 19 insertions(+), 5 deletions(-) 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..bda4c3e3d58 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -22,6 +22,8 @@ # Files extensions for encrypted and plaintext persistence file_extensions = {True: '.bin', False: '.json'} +KEYCHAIN_SERVICE_NAME = 'azure-cli' +LIBSECRET_SCHEMA_NAME = 'azure-cli' def load_persisted_token_cache(location, encrypt): persistence = build_persistence(location, encrypt) @@ -39,14 +41,25 @@ def build_persistence(location, encrypt): logger.debug("build_persistence: location=%r, encrypt=%r", location, encrypt) if encrypt: if sys.platform.startswith('win'): + # For FilePersistenceWithDataProtection, location is where the credential is stored. + logger.debug("Initializing FilePersistenceWithDataProtection.") return FilePersistenceWithDataProtection(location) 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 + logger.debug("Initializing KeychainPersistence") + return KeychainPersistence(location, service_name=KEYCHAIN_SERVICE_NAME) if sys.platform.startswith('linux'): + # 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. + logger.debug("Initializing LibsecretPersistence.") return LibsecretPersistence( location, - schema_name="my_schema_name", - attributes={"my_attr1": "foo", "my_attr2": "bar"} + schema_name=LIBSECRET_SCHEMA_NAME, + attributes={} ) else: return FilePersistence(location) 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) From 2dbe201ca9863b886ace9ab738dfd6bcafe263a4 Mon Sep 17 00:00:00 2001 From: isra-fel <11371776+isra-fel@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:46:47 +1000 Subject: [PATCH 02/10] refactor: token cache file extension --- .../azure/cli/core/auth/persistence.py | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) 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 bda4c3e3d58..9bbd8ee6a8e 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -20,10 +20,13 @@ 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 = 'azure-cli' -LIBSECRET_SCHEMA_NAME = 'azure-cli' +KEYCHAIN_SERVICE_NAME = 'Microsoft Azure CLI MSAL Token Cache' +LIBSECRET_SCHEMA_NAME = 'Microsoft Azure CLI MSAL Token Cache' def load_persisted_token_cache(location, encrypt): persistence = build_persistence(location, encrypt) @@ -37,32 +40,43 @@ def load_secret_store(location, encrypt): def build_persistence(location, encrypt): """Build a suitable persistence instance based your current OS""" - location += file_extensions[encrypt] logger.debug("build_persistence: location=%r, encrypt=%r", location, encrypt) if encrypt: if sys.platform.startswith('win'): # For FilePersistenceWithDataProtection, location is where the credential is stored. - logger.debug("Initializing FilePersistenceWithDataProtection.") - return FilePersistenceWithDataProtection(location) + path = location + file_extension_encrypted + logger.debug("Initializing FilePersistenceWithDataProtection: location=%r", path) + return FilePersistenceWithDataProtection(path) if sys.platform.startswith('darwin'): # 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 - logger.debug("Initializing KeychainPersistence") - return KeychainPersistence(location, service_name=KEYCHAIN_SERVICE_NAME) + path = location + file_extension_signal + logger.debug("Initializing KeychainPersistence: location=%r", path) + return KeychainPersistence(path, service_name=KEYCHAIN_SERVICE_NAME) if sys.platform.startswith('linux'): # 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. - logger.debug("Initializing LibsecretPersistence.") - return LibsecretPersistence( - location, - schema_name=LIBSECRET_SCHEMA_NAME, - attributes={} - ) - else: - return FilePersistence(location) + path = location + file_extension_signal + logger.debug("Initializing LibsecretPersistence: location=%r", path) + try: + return LibsecretPersistence( + path, + schema_name=LIBSECRET_SCHEMA_NAME, + attributes={} + ) + except Exception as e: + # Warn the user and continue with FilePersistence. + # LibsecretPersistence are known to be unavailable in some Linux environments. + logger.debug("Failed to initialize LibsecretPersistence: %s", e) + logger.warning("TBD: Encryption is unavailable. Falling back to plaintext persistence." + "Please follow https://aka.ms/azure-cli-credential-encryption to enable encryption.") + # 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) class SecretStore: From 1c6f437bcb07d32bda4aae004af9b729fdd1e975 Mon Sep 17 00:00:00 2001 From: isra-fel <11371776+isra-fel@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:01:25 +1000 Subject: [PATCH 03/10] use different types for token cache and secret store --- .../azure/cli/core/auth/persistence.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) 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 9bbd8ee6a8e..5f9a7388708 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -25,22 +25,22 @@ file_extension_signal = '.sig' file_extensions = [file_extension_encrypted, file_extension_plaintext, file_extension_signal] -KEYCHAIN_SERVICE_NAME = 'Microsoft Azure CLI MSAL Token Cache' -LIBSECRET_SCHEMA_NAME = 'Microsoft Azure CLI MSAL Token Cache' +KEYCHAIN_SERVICE_NAME = 'Microsoft Azure CLI' +LIBSECRET_SCHEMA_NAME = 'Microsoft Azure CLI' 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): """Build a suitable persistence instance based your current OS""" - 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'): # For FilePersistenceWithDataProtection, location is where the credential is stored. @@ -54,7 +54,7 @@ def build_persistence(location, encrypt): # 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) + return KeychainPersistence(path, service_name=KEYCHAIN_SERVICE_NAME, account_name=type) if sys.platform.startswith('linux'): # 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. @@ -62,10 +62,11 @@ def build_persistence(location, encrypt): 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=attributes ) except Exception as e: # Warn the user and continue with FilePersistence. From 233ceb959d4d67279bbcdb4234fd721a1fd4cc9d Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Thu, 6 Aug 2026 09:21:56 +1000 Subject: [PATCH 04/10] fix az logout failure by using list values instead of dictionary values --- src/azure-cli-core/azure/cli/core/auth/identity.py | 4 ++-- 1 file changed, 2 insertions(+), 2 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 91629e89441..cc4ebd0e542 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -211,7 +211,7 @@ def logout_all_users(self): self._msal_app.remove_account(account) # Also remove token cache file - for e in file_extensions.values(): + for e in file_extensions: _try_remove(self._token_cache_file + e) def logout_service_principal(self, client_id): @@ -229,7 +229,7 @@ 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(): + for e in file_extensions: _try_remove(self._secret_file + e) def get_user(self, user=None): From 5b689038eea4fcd0464465277134cf24d9ad0a4b Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Wed, 19 Aug 2026 12:24:28 +1000 Subject: [PATCH 05/10] Warning only login and debug when build persistence --- src/azure-cli-core/azure/cli/core/_profile.py | 6 +- .../azure/cli/core/auth/persistence.py | 31 ++++++++-- .../cli/core/auth/tests/test_persistence.py | 58 +++++++++++++++++++ 3 files changed, 90 insertions(+), 5 deletions(-) create mode 100644 src/azure-cli-core/azure/cli/core/auth/tests/test_persistence.py 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/persistence.py b/src/azure-cli-core/azure/cli/core/auth/persistence.py index 5f9a7388708..1dc1d1e84db 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -28,6 +28,15 @@ 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, type="Token cache") return PersistedTokenCache(persistence) @@ -69,17 +78,31 @@ def build_persistence(location, encrypt, type=None): attributes=attributes ) except Exception as e: - # Warn the user and continue with FilePersistence. - # LibsecretPersistence are known to be unavailable in some Linux environments. + # 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) - logger.warning("TBD: Encryption is unavailable. Falling back to plaintext persistence." - "Please follow https://aka.ms/azure-cli-credential-encryption to enable encryption.") + _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 warn_if_encryption_unavailable(): + """Warn that credentials are persisted in plaintext. + + Called at sign-in only. Every command runs in a new process, so warning once per session would + require storing state on the machine, and warning on every command is too noisy. + """ + if _encryption_fallback: + logger.warning(ENCRYPTION_FALLBACK_WARNING) + + class SecretStore: def __init__(self, persistence): self._lock_file = persistence.get_location() + ".lockfile" 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..88afd1de1fe --- /dev/null +++ b/src/azure-cli-core/azure/cli/core/auth/tests/test_persistence.py @@ -0,0 +1,58 @@ +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- + +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) + + +if __name__ == '__main__': + unittest.main() From 06b0c0d873cdb3260facb6d351318bb3b685915f Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Thu, 20 Aug 2026 09:24:55 +1000 Subject: [PATCH 06/10] erase os secret persistence when logout or account clear --- .../azure/cli/core/auth/identity.py | 16 ++++++++++--- .../azure/cli/core/auth/persistence.py | 14 +++++++++++ .../cli/core/auth/tests/test_persistence.py | 24 +++++++++++++++++++ 3 files changed, 51 insertions(+), 3 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 cc4ebd0e542..69a62951338 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,8 @@ 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, file_extensions, 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,6 +211,12 @@ def logout_all_users(self): for account in accounts: self._msal_app.remove_account(account) + # Empty the payload before removing 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='{}') + # Also remove token cache file for e in file_extensions: _try_remove(self._token_cache_file + e) @@ -227,8 +234,11 @@ 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. + # 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 token cache. + erase_persistence(self._secret_file, self._encrypt, type="Secret store", + empty_payload='[]') + for e in file_extensions: _try_remove(self._secret_file + e) 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 1dc1d1e84db..67e595e602a 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -93,6 +93,20 @@ def _record_encryption_fallback(): _encryption_fallback = True +def erase_persistence(location, encrypt, type=None, empty_payload='{}'): # pylint: disable=redefined-builtin + """Overwrite a persisted payload with empty content. + + 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. + """ + try: + build_persistence(location, encrypt, type=type).save(empty_payload) + except Exception as e: # pylint: disable=broad-except + # Best effort. The files are removed by the caller regardless. + logger.debug("Failed to erase persisted payload at %r: %s", location, e) + + def warn_if_encryption_unavailable(): """Warn that credentials are persisted in plaintext. 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 index 88afd1de1fe..45a9384309b 100644 --- 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 @@ -54,5 +54,29 @@ def test_no_warning_when_encryption_opted_out(self): self.assertFalse(persistence._encryption_fallback) +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() + with mock.patch.object(persistence, 'build_persistence', return_value=built) as build_mock: + persistence.erase_persistence('/tmp/test_persistence', True, type='Secret store', + empty_payload='[]') + + build_mock.assert_called_once_with('/tmp/test_persistence', True, type='Secret store') + built.save.assert_called_once_with('[]') + + def test_failure_to_erase_is_swallowed(self): + # The caller removes the files regardless, so a keyring error must not break logout. + built = mock.MagicMock() + built.save.side_effect = Exception('keyring is gone') + with mock.patch.object(persistence, 'build_persistence', return_value=built): + persistence.erase_persistence('/tmp/test_persistence', True, type='Token cache') + + if __name__ == '__main__': unittest.main() From e3d86e9dd4fc3423fed1ba416411d246a65e2ec8 Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Thu, 20 Aug 2026 20:00:32 +1000 Subject: [PATCH 07/10] Lock to prevent concurrent write to corrupt the file --- .../azure/cli/core/auth/identity.py | 23 ++----- .../azure/cli/core/auth/persistence.py | 38 +++++++++++- .../cli/core/auth/tests/test_persistence.py | 60 ++++++++++++++++--- 3 files changed, 91 insertions(+), 30 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 69a62951338..def69e753f1 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -15,8 +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, - erase_persistence) +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: @@ -211,16 +210,12 @@ def logout_all_users(self): for account in accounts: self._msal_app.remove_account(account) - # Empty the payload before removing 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. + # 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='{}') - # Also remove token cache file - for e in file_extensions: - _try_remove(self._token_cache_file + e) - def logout_service_principal(self, client_id): # If client_id is a username, it is ignored @@ -239,9 +234,6 @@ def logout_all_service_principal(self): erase_persistence(self._secret_file, self._encrypt, type="Secret store", empty_payload='[]') - for e in file_extensions: - _try_remove(self._secret_file + e) - def get_user(self, user=None): accounts = self._msal_app.get_accounts(user) if user else self._msal_app.get_accounts() return accounts @@ -441,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 67e595e602a..0fd06593ffc 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, @@ -93,18 +94,49 @@ def _record_encryption_fallback(): _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 erase_persistence(location, encrypt, type=None, empty_payload='{}'): # pylint: disable=redefined-builtin - """Overwrite a persisted payload with empty content. + """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. + + 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: - build_persistence(location, encrypt, type=type).save(empty_payload) + persistence = build_persistence(location, encrypt, type=type) + # 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 - # Best effort. The files are removed by the caller regardless. + # 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(): 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 index 45a9384309b..e0489ef6920 100644 --- 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 @@ -63,20 +63,64 @@ class TestErasePersistence(unittest.TestCase): def test_payload_is_overwritten_through_the_persistence(self): built = mock.MagicMock() - with mock.patch.object(persistence, 'build_persistence', return_value=built) as build_mock: - persistence.erase_persistence('/tmp/test_persistence', True, type='Secret store', - empty_payload='[]') - + 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_failure_to_erase_is_swallowed(self): - # The caller removes the files regardless, so a keyring error must not break logout. + 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.save.side_effect = Exception('keyring is gone') - with mock.patch.object(persistence, 'build_persistence', return_value=built): + 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() From 9439e457673499cd467eda892514d6812e6459da Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Sat, 22 Aug 2026 08:06:35 +1000 Subject: [PATCH 08/10] Modify comment --- src/azure-cli-core/azure/cli/core/auth/identity.py | 2 +- src/azure-cli-core/azure/cli/core/auth/persistence.py | 5 ----- 2 files changed, 1 insertion(+), 6 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 def69e753f1..a293e13bfc4 100644 --- a/src/azure-cli-core/azure/cli/core/auth/identity.py +++ b/src/azure-cli-core/azure/cli/core/auth/identity.py @@ -230,7 +230,7 @@ def logout_service_principal(self, client_id): def logout_all_service_principal(self): # remove service principal secrets # 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 token cache. + # 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='[]') 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 0fd06593ffc..8f6225276d1 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -140,11 +140,6 @@ def erase_persistence(location, encrypt, type=None, empty_payload='{}'): # pyli def warn_if_encryption_unavailable(): - """Warn that credentials are persisted in plaintext. - - Called at sign-in only. Every command runs in a new process, so warning once per session would - require storing state on the machine, and warning on every command is too noisy. - """ if _encryption_fallback: logger.warning(ENCRYPTION_FALLBACK_WARNING) From c8543c53db6fc1acd1df43a56ae1109ee3449dc4 Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Sun, 23 Aug 2026 11:14:23 +1000 Subject: [PATCH 09/10] Delete previous os encryption store when current encrypt config is false --- .../azure/cli/core/auth/persistence.py | 31 ++++ .../cli/core/auth/tests/test_persistence.py | 136 ++++++++++++++++++ 2 files changed, 167 insertions(+) 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 8f6225276d1..c6d1557a6e6 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -113,6 +113,32 @@ def _remove_persistence_files(location): _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. @@ -120,12 +146,17 @@ def erase_persistence(location, encrypt, type=None, empty_payload='{}'): # pyli 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) 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 index e0489ef6920..6e5316c0b5e 100644 --- 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 @@ -3,6 +3,8 @@ # 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 @@ -54,6 +56,140 @@ def test_no_warning_when_encryption_opted_out(self): 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. From 6c314ed2dd5d771b5ede98801e08fa158f5f0d05 Mon Sep 17 00:00:00 2001 From: Ming Xu Date: Wed, 26 Aug 2026 19:05:25 +1000 Subject: [PATCH 10/10] Fix pylint warnings in persistence.py Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/azure-cli-core/azure/cli/core/auth/persistence.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 c6d1557a6e6..a80d5ef744a 100644 --- a/src/azure-cli-core/azure/cli/core/auth/persistence.py +++ b/src/azure-cli-core/azure/cli/core/auth/persistence.py @@ -48,7 +48,7 @@ def load_secret_store(location, encrypt): return SecretStore(persistence) -def build_persistence(location, encrypt, type=None): +def build_persistence(location, encrypt, type=None): # pylint: disable=redefined-builtin """Build a suitable persistence instance based your current OS""" logger.debug("build_persistence: location=%r, encrypt=%r, type=%r", location, encrypt, type) if encrypt: @@ -78,7 +78,7 @@ def build_persistence(location, encrypt, type=None): schema_name=LIBSECRET_SCHEMA_NAME, attributes=attributes ) - except Exception as e: + 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)