From 42d482da5f2c5e93316abbe4534651639278431c Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Tue, 30 Nov 2021 23:42:21 -0800 Subject: [PATCH 1/5] Making all platform-dependent parameters optional --- msal_extensions/persistence.py | 18 +++++++++++------- sample/persistence_sample.py | 5 ++--- sample/token_cache_sample.py | 5 ++--- tests/test_persistence.py | 9 ++------- 4 files changed, 17 insertions(+), 20 deletions(-) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index e4b6c66..dd8b890 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -9,6 +9,7 @@ import abc import os import errno +import hashlib import logging import sys try: @@ -50,6 +51,9 @@ def _mkdir_p(path): else: raise +def _auto_hash(input_string): + return hashlib.sha256(input_string.encode('utf-8')).hexdigest() + # We do not aim to wrap every os-specific exception. # Here we define only the most common one, @@ -197,19 +201,18 @@ class KeychainPersistence(BasePersistence): and protected by native Keychain libraries on OSX""" is_encrypted = True - def __init__(self, signal_location, service_name, account_name): + def __init__(self, signal_location, service_name=None, account_name=None): """Initialization could fail due to unsatisfied dependency. :param signal_location: See :func:`persistence.LibsecretPersistence.__init__` """ - if not (service_name and account_name): # It would hang on OSX - raise ValueError("service_name and account_name are required") from .osx import Keychain, KeychainError # pylint: disable=import-outside-toplevel self._file_persistence = FilePersistence(signal_location) # Favor composition self._Keychain = Keychain # pylint: disable=invalid-name self._KeychainError = KeychainError # pylint: disable=invalid-name - self._service_name = service_name - self._account_name = account_name + default_service_name = "msal-extensions" # This is also our package name + self._service_name = service_name or default_service_name + self._account_name = account_name or _auto_hash(signal_location) def save(self, content): with self._Keychain() as locker: @@ -247,7 +250,7 @@ class LibsecretPersistence(BasePersistence): and protected by native libsecret libraries on Linux""" is_encrypted = True - def __init__(self, signal_location, schema_name, attributes, **kwargs): + def __init__(self, signal_location, schema_name=None, attributes=None, **kwargs): """Initialization could fail due to unsatisfied dependency. :param string signal_location: @@ -262,7 +265,8 @@ def __init__(self, signal_location, schema_name, attributes, **kwargs): from .libsecret import ( # This uncertain import is deferred till runtime LibSecretAgent, trial_run) trial_run() - self._agent = LibSecretAgent(schema_name, attributes, **kwargs) + self._agent = LibSecretAgent( + schema_name or _auto_hash(signal_location), attributes or {}, **kwargs) self._file_persistence = FilePersistence(signal_location) # Favor composition def save(self, content): diff --git a/sample/persistence_sample.py b/sample/persistence_sample.py index 74074d3..f5c8c06 100644 --- a/sample/persistence_sample.py +++ b/sample/persistence_sample.py @@ -10,7 +10,7 @@ def build_persistence(location, fallback_to_plaintext=False): if sys.platform.startswith('win'): return FilePersistenceWithDataProtection(location) if sys.platform.startswith('darwin'): - return KeychainPersistence(location, "my_service_name", "my_account_name") + return KeychainPersistence(location) if sys.platform.startswith('linux'): try: return LibsecretPersistence( @@ -21,8 +21,6 @@ def build_persistence(location, fallback_to_plaintext=False): # unless there would frequently be a desktop session and # a remote ssh session being active simultaneously. location, - schema_name="my_schema_name", - attributes={"my_attr1": "foo", "my_attr2": "bar"}, ) except: # pylint: disable=bare-except if not fallback_to_plaintext: @@ -31,6 +29,7 @@ def build_persistence(location, fallback_to_plaintext=False): return FilePersistence(location) persistence = build_persistence("storage.bin", fallback_to_plaintext=False) +print("Type of persistence: {}".format(persistence.__class__.__name__)) print("Is this persistence encrypted?", persistence.is_encrypted) data = { # It can be anything, here we demonstrate an arbitrary json object diff --git a/sample/token_cache_sample.py b/sample/token_cache_sample.py index b48e19d..7210efa 100644 --- a/sample/token_cache_sample.py +++ b/sample/token_cache_sample.py @@ -10,7 +10,7 @@ def build_persistence(location, fallback_to_plaintext=False): if sys.platform.startswith('win'): return FilePersistenceWithDataProtection(location) if sys.platform.startswith('darwin'): - return KeychainPersistence(location, "my_service_name", "my_account_name") + return KeychainPersistence(location) if sys.platform.startswith('linux'): try: return LibsecretPersistence( @@ -21,8 +21,6 @@ def build_persistence(location, fallback_to_plaintext=False): # unless there would frequently be a desktop session and # a remote ssh session being active simultaneously. location, - schema_name="my_schema_name", - attributes={"my_attr1": "foo", "my_attr2": "bar"}, ) except: # pylint: disable=bare-except if not fallback_to_plaintext: @@ -31,6 +29,7 @@ def build_persistence(location, fallback_to_plaintext=False): return FilePersistence(location) persistence = build_persistence("token_cache.bin") +print("Type of persistence: {}".format(persistence.__class__.__name__)) print("Is this persistence encrypted?", persistence.is_encrypted) cache = PersistedTokenCache(persistence) diff --git a/tests/test_persistence.py b/tests/test_persistence.py index bbbe155..dfc0963 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -54,8 +54,7 @@ def test_nonexistent_file_persistence_with_data_protection(temp_location): not sys.platform.startswith('darwin'), reason="Requires OSX. Whether running on TRAVIS CI does not seem to matter.") def test_keychain_persistence(temp_location): - _test_persistence_roundtrip(KeychainPersistence( - temp_location, "my_service_name", "my_account_name")) + _test_persistence_roundtrip(KeychainPersistence(temp_location)) @pytest.mark.skipif( not sys.platform.startswith('darwin'), @@ -69,11 +68,7 @@ def test_nonexistent_keychain_persistence(temp_location): is_running_on_travis_ci or not sys.platform.startswith('linux'), reason="Requires Linux Desktop. Headless or SSH session won't work.") def test_libsecret_persistence(temp_location): - _test_persistence_roundtrip(LibsecretPersistence( - temp_location, - "my_schema_name", - {"my_attr_1": "foo", "my_attr_2": "bar"}, - )) + _test_persistence_roundtrip(LibsecretPersistence(temp_location)) @pytest.mark.skipif( is_running_on_travis_ci or not sys.platform.startswith('linux'), From 48bc3d71ecd0fd7e16bb0d9a915b44999cda2cd4 Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Sun, 30 Jan 2022 11:24:44 -0800 Subject: [PATCH 2/5] Use 600 mode on Unix --- msal_extensions/persistence.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index dd8b890..4c56cd5 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -105,6 +105,11 @@ def get_location(self): raise NotImplementedError +def _open(location): + return os.open(location, os.O_RDWR | os.O_CREAT | os.O_TRUNC, 0o600) + # The 600 seems no-op on NTFS/Windows, and that is fine + + class FilePersistence(BasePersistence): """A generic persistence, storing data in a plain-text file""" @@ -117,7 +122,7 @@ def __init__(self, location): def save(self, content): # type: (str) -> None """Save the content into this persistence""" - with open(self._location, 'w+') as handle: # pylint: disable=unspecified-encoding + with os.fdopen(_open(self._location), 'w+') as handle: handle.write(content) def load(self): @@ -173,7 +178,7 @@ def __init__(self, location, entropy=''): def save(self, content): # type: (str) -> None data = self._dp_agent.protect(content) - with open(self._location, 'wb+') as handle: + with os.fdopen(_open(self._location), 'wb+') as handle: handle.write(data) def load(self): From cc6fd0036a151acfec456e8ed1f90ff75d2c52ba Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Thu, 3 Feb 2022 23:08:51 -0800 Subject: [PATCH 3/5] Provide actionable messages for 2 dpapi errors --- msal_extensions/persistence.py | 40 +++++++++++++++++++++++++++------- msal_extensions/windows.py | 12 ++++++++-- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index 4c56cd5..8ce1af6 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -56,19 +56,31 @@ def _auto_hash(input_string): # We do not aim to wrap every os-specific exception. -# Here we define only the most common one, -# otherwise caller would need to catch os-specific persistence exceptions. -class PersistenceNotFound(IOError): # Use IOError rather than OSError as base, +# Here we standardize only the most common ones, +# otherwise caller would need to catch os-specific underlying exceptions. +class PersistenceError(IOError): # Use IOError rather than OSError as base, + """The base exception for persistence.""" # because historically an IOError was bubbled up and expected. # https://github.com/AzureAD/microsoft-authentication-extensions-for-python/blob/0.2.2/msal_extensions/token_cache.py#L38 # Now we want to maintain backward compatibility even when using Python 2.x # It makes no difference in Python 3.3+ where IOError is an alias of OSError. + def __init__(self, err_no=None, message=None, location=None): # pylint: disable=useless-super-delegation + super(PersistenceError, self).__init__(err_no, message, location) + + +class PersistenceNotFound(PersistenceError): """This happens when attempting BasePersistence.load() on a non-existent persistence instance""" def __init__(self, err_no=None, message=None, location=None): super(PersistenceNotFound, self).__init__( - err_no or errno.ENOENT, - message or "Persistence not found", - location) + err_no=errno.ENOENT, + message=message or "Persistence not found", + location=location) + +class PersistenceEncryptionError(PersistenceError): + """This could be raised by persistence.save()""" + +class PersistenceDecryptionError(PersistenceError): + """This could be raised by persistence.load()""" class BasePersistence(ABC): @@ -177,7 +189,12 @@ def __init__(self, location, entropy=''): def save(self, content): # type: (str) -> None - data = self._dp_agent.protect(content) + try: + data = self._dp_agent.protect(content) + except OSError as exception: + raise PersistenceEncryptionError( + err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows + message="Unable to encrypt data. You may consider disable encryption.") with os.fdopen(_open(self._location), 'wb+') as handle: handle.write(data) @@ -186,7 +203,6 @@ def load(self): try: with open(self._location, 'rb') as handle: data = handle.read() - return self._dp_agent.unprotect(data) except EnvironmentError as exp: # EnvironmentError in Py 2.7 works across platform if exp.errno == errno.ENOENT: raise PersistenceNotFound( @@ -199,6 +215,14 @@ def load(self): "DPAPI error likely caused by file content not previously encrypted. " "App developer should migrate by calling save(plaintext) first.") raise + try: + return self._dp_agent.unprotect(data) + except OSError as exception: + raise PersistenceDecryptionError( + err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows + message="Unable to decrypt data. You may have to delete the file.", + location=self._location, + ) class KeychainPersistence(BasePersistence): diff --git a/msal_extensions/windows.py b/msal_extensions/windows.py index f1e8b59..9da9275 100644 --- a/msal_extensions/windows.py +++ b/msal_extensions/windows.py @@ -39,6 +39,14 @@ def raw(self): _MEMCPY(blob_buffer, pb_data, cb_data) return blob_buffer.raw +_err_description = { + # Keys came from real world observation, values came from winerror.h (https://errors) + -2146893813: "Key not valid for use in specified state.", + -2146892987: "The requested operation cannot be completed. " + "The computer must be trusted for delegation and " + "the current user account must be configured to allow delegation. " + "See also https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/enable-computer-and-user-accounts-to-be-trusted-for-delegation", + } # This code is modeled from a StackOverflow question, which can be found here: # https://stackoverflow.com/questions/463832/using-dpapi-with-python @@ -82,7 +90,7 @@ def protect(self, message): _LOCAL_FREE(result.pbData) err_code = _GET_LAST_ERROR() - raise OSError(256, '', '', err_code) + raise OSError(None, _err_description.get(err_code), None, err_code) def unprotect(self, cipher_text): # type: (bytes) -> str @@ -111,4 +119,4 @@ def unprotect(self, cipher_text): finally: _LOCAL_FREE(result.pbData) err_code = _GET_LAST_ERROR() - raise OSError(256, '', '', err_code) + raise OSError(None, _err_description.get(err_code), None, err_code) From c98bcba9149f601a286bfe0a953c26c46b9fb22f Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Sat, 5 Feb 2022 16:43:05 -0800 Subject: [PATCH 4/5] Add underlying OSError info into Persistence exceptions --- msal_extensions/persistence.py | 5 +++-- msal_extensions/windows.py | 1 + tests/test_persistence.py | 19 ++++++++++++++++--- tox.ini | 2 ++ 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index 8ce1af6..257adad 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -194,7 +194,8 @@ def save(self, content): except OSError as exception: raise PersistenceEncryptionError( err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows - message="Unable to encrypt data. You may consider disable encryption.") + message="Encryption failed: {}. Consider disable encryption.".format(exception), # pylint: disable=consider-using-f-string + ) with os.fdopen(_open(self._location), 'wb+') as handle: handle.write(data) @@ -220,7 +221,7 @@ def load(self): except OSError as exception: raise PersistenceDecryptionError( err_no=getattr(exception, "winerror", None), # Exists in Python 3 on Windows - message="Unable to decrypt data. You may have to delete the file.", + message="Decryption failed: {}. You may have to delete the file.".format(exception), # pylint: disable=consider-using-f-string location=self._location, ) diff --git a/msal_extensions/windows.py b/msal_extensions/windows.py index 9da9275..d534619 100644 --- a/msal_extensions/windows.py +++ b/msal_extensions/windows.py @@ -46,6 +46,7 @@ def raw(self): "The computer must be trusted for delegation and " "the current user account must be configured to allow delegation. " "See also https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/enable-computer-and-user-accounts-to-be-trusted-for-delegation", + 13: "The data is invalid", } # This code is modeled from a StackOverflow question, which can be found here: diff --git a/tests/test_persistence.py b/tests/test_persistence.py index dfc0963..eab4048 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -9,11 +9,18 @@ from msal_extensions.persistence import * -is_running_on_travis_ci = bool( # (WTF) What-The-Finding: +def _is_env_var_defined(env_var): + return bool( # (WTF) What-The-Finding: # The bool(...) is necessary, otherwise skipif(...) would treat "true" as # string conditions and then raise an undefined "true" exception. # https://docs.pytest.org/en/latest/historical-notes.html#string-conditions - os.getenv("TRAVIS")) + os.getenv(env_var)) + + +# Note: If you use tox, remember to pass them through via tox.ini +# https://tox.wiki/en/latest/example/basic.html#passing-down-environment-variables +is_running_on_travis_ci = _is_env_var_defined("TRAVIS") +is_running_on_github_ci = _is_env_var_defined("GITHUB_ACTIONS") @pytest.fixture def temp_location(): @@ -42,7 +49,13 @@ def test_nonexistent_file_persistence(temp_location): is_running_on_travis_ci or not sys.platform.startswith('win'), reason="Requires Windows Desktop") def test_file_persistence_with_data_protection(temp_location): - _test_persistence_roundtrip(FilePersistenceWithDataProtection(temp_location)) + try: + _test_persistence_roundtrip(FilePersistenceWithDataProtection(temp_location)) + except PersistenceDecryptionError: + if is_running_on_github_ci or is_running_on_travis_ci: + logging.warning("DPAPI tends to fail on Windows VM. Run this on your desktop to double check.") + else: + raise @pytest.mark.skipif( is_running_on_travis_ci or not sys.platform.startswith('win'), diff --git a/tox.ini b/tox.ini index ecbab74..8a538bc 100644 --- a/tox.ini +++ b/tox.ini @@ -5,5 +5,7 @@ envlist = py27,py35,py36,py37,py38 deps = pytest passenv = TRAVIS + GITHUB_ACTIONS + commands = pytest From cda2b7c62683569e1c51084c45f7d30d1530c3bb Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Wed, 8 Dec 2021 20:44:33 -0800 Subject: [PATCH 5/5] MSAL Extensions Python 0.4.0 Add description that will be displayed on PyPI --- msal_extensions/__init__.py | 2 +- setup.cfg | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/msal_extensions/__init__.py b/msal_extensions/__init__.py index 14a8c6d..141cfb9 100644 --- a/msal_extensions/__init__.py +++ b/msal_extensions/__init__.py @@ -1,5 +1,5 @@ """Provides auxiliary functionality to the `msal` package.""" -__version__ = "0.3.1" +__version__ = "0.4.0" import sys diff --git a/setup.cfg b/setup.cfg index 80050d9..6b5d8b8 100644 --- a/setup.cfg +++ b/setup.cfg @@ -9,3 +9,4 @@ project_urls = Changelog = https://github.com/AzureAD/microsoft-authenticatio classifiers = License :: OSI Approved :: MIT License Development Status :: 4 - Beta +description = Microsoft Authentication Library extensions (MSAL EX) provides a persistence API that can save your data on disk, encrypted on Windows, macOS and Linux. Concurrent data access will be coordinated by a file lock mechanism.