From 4dc6427457cf9d7b4437c6a13c5a0b417b5e0398 Mon Sep 17 00:00:00 2001 From: Justin Hutchings Date: Tue, 28 Apr 2020 12:25:36 -0700 Subject: [PATCH 1/8] Add CodeQL Analysis workflow --- .github/workflows/workflows/codeql.yml | 46 ++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 .github/workflows/workflows/codeql.yml diff --git a/.github/workflows/workflows/codeql.yml b/.github/workflows/workflows/codeql.yml new file mode 100644 index 0000000..75a7e5b --- /dev/null +++ b/.github/workflows/workflows/codeql.yml @@ -0,0 +1,46 @@ +name: "Code Scanning - Action" + +on: + push: + schedule: + - cron: '0 0 * * 0' + +jobs: + CodeQL-Build: + + strategy: + fail-fast: false + + + # CodeQL runs on ubuntu-latest, windows-latest, and macos-latest + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v2 + + # Initializes the CodeQL tools for scanning. + - name: Initialize CodeQL + uses: github/codeql-action/init@v1 + # Override language selection by uncommenting this and choosing your languages + # with: + # languages: go, javascript, csharp, python, cpp, java + + # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). + # If this step fails, then you should remove it and run the build manually (see below). + - name: Autobuild + uses: github/codeql-action/autobuild@v1 + + # ℹī¸ Command-line programs to run using the OS shell. + # 📚 https://git.io/JvXDl + + # ✏ī¸ If the Autobuild fails above, remove it and uncomment the following three lines + # and modify them (or add more) to build your code if your project + # uses a compiled language + + #- run: | + # make bootstrap + # make release + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v1 \ No newline at end of file From f2f3f6d4f11f7532cd40a453575b81e89d28c250 Mon Sep 17 00:00:00 2001 From: Grey Baker Date: Wed, 29 Apr 2020 00:16:54 +0100 Subject: [PATCH 2/8] Move .github/workflows/workflows dir to .github/workflows --- .github/workflows/{workflows => }/codeql.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename .github/workflows/{workflows => }/codeql.yml (96%) diff --git a/.github/workflows/workflows/codeql.yml b/.github/workflows/codeql.yml similarity index 96% rename from .github/workflows/workflows/codeql.yml rename to .github/workflows/codeql.yml index 75a7e5b..aabab06 100644 --- a/.github/workflows/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -43,4 +43,4 @@ jobs: # make release - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v1 \ No newline at end of file + uses: github/codeql-action/analyze@v1 From 35641c37de1f0bbb214627769e5847e74808ad55 Mon Sep 17 00:00:00 2001 From: Abhidnya Date: Fri, 5 Jun 2020 12:23:02 -0700 Subject: [PATCH 3/8] Performance Testing (#58) --- tests/cache_file_generator.py | 43 +++++++++++++++++ tests/test_cache_lock_file_perf.py | 75 ++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 tests/cache_file_generator.py create mode 100644 tests/test_cache_lock_file_perf.py diff --git a/tests/cache_file_generator.py b/tests/cache_file_generator.py new file mode 100644 index 0000000..4164314 --- /dev/null +++ b/tests/cache_file_generator.py @@ -0,0 +1,43 @@ +""" +Usage: cache_file_generator.py cache_file_path sleep_interval + +This is a console application which is to be used for cross-platform lock performance testing. +The app will acquire lock for the cache file, log the process id and then release the lock. + +It takes in two arguments - cache file path and the sleep interval. +The cache file path is the path of cache file. +The sleep interval is the time in seconds for which the lock is held by a process. +""" + +import logging +import os +import sys +import time + +from portalocker import exceptions + +from msal_extensions import FilePersistence, CrossPlatLock + + +def _acquire_lock_and_write_to_cache(cache_location, sleep_interval): + cache_accessor = FilePersistence(cache_location) + lock_file_path = cache_accessor.get_location() + ".lockfile" + try: + with CrossPlatLock(lock_file_path): + data = cache_accessor.load() + if data is None: + data = "" + data += "< " + str(os.getpid()) + "\n" + time.sleep(sleep_interval) + data += "> " + str(os.getpid()) + "\n" + cache_accessor.save(data) + except exceptions.LockException as e: + logging.warning("Unable to acquire lock %s", e) + + +if __name__ == "__main__": + if len(sys.argv) < 3: + print(__doc__) + sys.exit(0) + _acquire_lock_and_write_to_cache(sys.argv[1], float(sys.argv[2])) + diff --git a/tests/test_cache_lock_file_perf.py b/tests/test_cache_lock_file_perf.py new file mode 100644 index 0000000..757fb80 --- /dev/null +++ b/tests/test_cache_lock_file_perf.py @@ -0,0 +1,75 @@ +import multiprocessing +import os +import shutil +import tempfile + +import pytest + +from cache_file_generator import _acquire_lock_and_write_to_cache + + +@pytest.fixture +def temp_location(): + test_folder = tempfile.mkdtemp(prefix="test_persistence_roundtrip") + yield os.path.join(test_folder, 'persistence.bin') + shutil.rmtree(test_folder, ignore_errors=True) + + +def _validate_result_in_cache(cache_location): + with open(cache_location) as handle: + data = handle.read() + prev_process_id = None + count = 0 + for line in data.split("\n"): + if line: + count += 1 + tag, process_id = line.split(" ") + if prev_process_id is not None: + assert process_id == prev_process_id, "Process overlap found" + assert tag == '>', "Process overlap_found" + prev_process_id = None + else: + assert tag == '<', "Opening bracket not found" + prev_process_id = process_id + return count + + +def _run_multiple_processes(no_of_processes, cache_location, sleep_interval): + open(cache_location, "w+") + processes = [] + for i in range(no_of_processes): + process = multiprocessing.Process( + target=_acquire_lock_and_write_to_cache, + args=(cache_location, sleep_interval)) + processes.append(process) + + for process in processes: + process.start() + + for process in processes: + process.join() + + +def test_lock_for_normal_workload(temp_location): + num_of_processes = 4 + sleep_interval = 0.1 + _run_multiple_processes(num_of_processes, temp_location, sleep_interval) + count = _validate_result_in_cache(temp_location) + assert count == num_of_processes * 2, "Should not observe starvation" + + +def test_lock_for_high_workload(temp_location): + num_of_processes = 80 + sleep_interval = 0 + _run_multiple_processes(num_of_processes, temp_location, sleep_interval) + count = _validate_result_in_cache(temp_location) + assert count <= num_of_processes * 2, "Starvation or not, we should not observe garbled payload" + + +def test_lock_for_timeout(temp_location): + num_of_processes = 10 + sleep_interval = 1 + _run_multiple_processes(num_of_processes, temp_location, sleep_interval) + count = _validate_result_in_cache(temp_location) + assert count < num_of_processes * 2, "Should observe starvation" + From 5c0f3ca30f1b1a84f88f1aa185909b7f42be6c8c Mon Sep 17 00:00:00 2001 From: Charles Lowell Date: Thu, 13 Aug 2020 17:52:55 -0700 Subject: [PATCH 4/8] Handle file not found exceptions on Python 2.7 --- msal_extensions/token_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msal_extensions/token_cache.py b/msal_extensions/token_cache.py index bf55448..d425c4d 100644 --- a/msal_extensions/token_cache.py +++ b/msal_extensions/token_cache.py @@ -35,7 +35,7 @@ def _reload_if_necessary(self): if self._last_sync < self._persistence.time_last_modified(): self.deserialize(self._persistence.load()) self._last_sync = time.time() - except IOError as exp: + except EnvironmentError as exp: if exp.errno != errno.ENOENT: raise # Otherwise, from cache's perspective, a nonexistent file is a NO-OP From 2ec6924129495f265d91d50162646c3fe25f7764 Mon Sep 17 00:00:00 2001 From: Abhidnya Date: Tue, 25 Aug 2020 15:57:55 -0700 Subject: [PATCH 5/8] Adding test case for this scenario --- tests/test_agnostic_backend.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_agnostic_backend.py b/tests/test_agnostic_backend.py index 4619391..29ca8a0 100644 --- a/tests/test_agnostic_backend.py +++ b/tests/test_agnostic_backend.py @@ -44,3 +44,8 @@ def test_current_platform_cache_roundtrip_with_alias_class(temp_location): def test_persisted_token_cache(temp_location): _test_token_cache_roundtrip(PersistedTokenCache(FilePersistence(temp_location))) +def test_file_not_found_error_is_not_raised(): + persistence = FilePersistence('non_existing_file') + cache = PersistedTokenCache(persistence=persistence) + # An exception raised here will fail the test case as it is supposed to be a NO-OP + cache.find('') From 8605a9503aab930397efbbd5caea5d19b2713ced Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Fri, 28 Aug 2020 17:10:04 -0700 Subject: [PATCH 6/8] Unified exception when load() is called before save() (#67) * Unified exception when load() is called before save() * Go with an explicit PersistenceNotFound exception * Caller has a much simpler pattern to handle read errors * Change "where" to "location" * Test cases for PersistentNotFound --- msal_extensions/persistence.py | 101 +++++++++++++++++++++++++++++---- msal_extensions/token_cache.py | 11 ++-- tests/test_persistence.py | 34 +++++++++++ 3 files changed, 128 insertions(+), 18 deletions(-) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index ef27d55..61a8cba 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -9,6 +9,7 @@ import abc import os import errno +import logging try: from pathlib import Path # Built-in in Python 3 except: @@ -21,6 +22,9 @@ ABC = abc.ABCMeta("ABC", (object,), {"__slots__": ()}) # type: ignore +logger = logging.getLogger(__name__) + + def _mkdir_p(path): """Creates a directory, and any necessary parents. @@ -41,6 +45,16 @@ def _mkdir_p(path): raise +# 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(OSError): + def __init__( + self, + err_no=errno.ENOENT, message="Persistence not found", location=None): + super(PersistenceNotFound, self).__init__(err_no, message, location) + + class BasePersistence(ABC): """An abstract persistence defining the common interface of this family""" @@ -55,12 +69,18 @@ def save(self, content): @abc.abstractmethod def load(self): # type: () -> str - """Load content from this persistence""" + """Load content from this persistence. + + Could raise PersistenceNotFound if no save() was called before. + """ raise NotImplementedError @abc.abstractmethod def time_last_modified(self): - """Get the last time when this persistence has been modified""" + """Get the last time when this persistence has been modified. + + Could raise PersistenceNotFound if no save() was called before. + """ raise NotImplementedError @abc.abstractmethod @@ -87,11 +107,32 @@ def save(self, content): def load(self): # type: () -> str """Load content from this persistence""" - with open(self._location, 'r') as handle: - return handle.read() + try: + with open(self._location, 'r') as handle: + return handle.read() + except EnvironmentError as exp: # EnvironmentError in Py 2.7 works across platform + if exp.errno == errno.ENOENT: + raise PersistenceNotFound( + message=( + "Persistence not initialized. " + "You can recover by calling a save() first."), + location=self._location, + ) + raise + def time_last_modified(self): - return os.path.getmtime(self._location) + try: + return os.path.getmtime(self._location) + except EnvironmentError as exp: # EnvironmentError in Py 2.7 works across platform + if exp.errno == errno.ENOENT: + raise PersistenceNotFound( + message=( + "Persistence not initialized. " + "You can recover by calling a save() first."), + location=self._location, + ) + raise def touch(self): """To touch this file-based persistence without writing content into it""" @@ -115,13 +156,28 @@ 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: - handle.write(self._dp_agent.protect(content)) + handle.write(data) def load(self): # type: () -> str - with open(self._location, 'rb') as handle: - return self._dp_agent.unprotect(handle.read()) + 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( + message=( + "Persistence not initialized. " + "You can recover by calling a save() first."), + location=self._location, + ) + logger.exception( + "DPAPI error likely caused by file content not previously encrypted. " + "App developer should migrate by calling save(plaintext) first.") + raise class KeychainPersistence(BasePersistence): @@ -136,9 +192,10 @@ def __init__(self, signal_location, service_name, account_name): """ 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 # pylint: disable=import-outside-toplevel + 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 @@ -150,8 +207,21 @@ def save(self, content): def load(self): with self._Keychain() as locker: - return locker.get_generic_password( - self._service_name, self._account_name) + try: + return locker.get_generic_password( + self._service_name, self._account_name) + except self._KeychainError as ex: + if ex.exit_status == self._KeychainError.ITEM_NOT_FOUND: + # This happens when a load() is called before a save(). + # We map it into cross-platform error for unified catching. + raise PersistenceNotFound( + location="Service:{} Account:{}".format( + self._service_name, self._account_name), + message=( + "Keychain persistence not initialized. " + "You can recover by call a save() first."), + ) + raise # We do not intend to hide any other underlying exceptions def time_last_modified(self): return self._file_persistence.time_last_modified() @@ -188,7 +258,14 @@ def save(self, content): self._file_persistence.touch() # For time_last_modified() def load(self): - return self._agent.load() + data = self._agent.load() + if data is None: + # Lower level libsecret would return None when found nothing. Here + # in persistence layer, we convert it to a unified error for consistence. + raise PersistenceNotFound(message=( + "Keyring persistence not initialized. " + "You can recover by call a save() first.")) + return data def time_last_modified(self): return self._file_persistence.time_last_modified() diff --git a/msal_extensions/token_cache.py b/msal_extensions/token_cache.py index d425c4d..e3bbea3 100644 --- a/msal_extensions/token_cache.py +++ b/msal_extensions/token_cache.py @@ -2,14 +2,13 @@ import os import warnings import time -import errno import logging import msal from .cache_lock import CrossPlatLock from .persistence import ( - _mkdir_p, FilePersistence, + _mkdir_p, PersistenceNotFound, FilePersistence, FilePersistenceWithDataProtection, KeychainPersistence) @@ -35,10 +34,10 @@ def _reload_if_necessary(self): if self._last_sync < self._persistence.time_last_modified(): self.deserialize(self._persistence.load()) self._last_sync = time.time() - except EnvironmentError as exp: - if exp.errno != errno.ENOENT: - raise - # Otherwise, from cache's perspective, a nonexistent file is a NO-OP + except PersistenceNotFound: + # From cache's perspective, a nonexistent persistence is a NO-OP. + pass + # However, existing data unable to be decrypted will still be bubbled up. def modify(self, credential_type, old_entry, new_key_value_pairs=None): with CrossPlatLock(self._lock_location): diff --git a/tests/test_persistence.py b/tests/test_persistence.py index 0ec369b..bbbe155 100644 --- a/tests/test_persistence.py +++ b/tests/test_persistence.py @@ -26,15 +26,30 @@ def _test_persistence_roundtrip(persistence): persistence.save(payload) assert persistence.load() == payload +def _test_nonexistent_persistence(persistence): + with pytest.raises(PersistenceNotFound): + persistence.load() + with pytest.raises(PersistenceNotFound): + persistence.time_last_modified() + def test_file_persistence(temp_location): _test_persistence_roundtrip(FilePersistence(temp_location)) +def test_nonexistent_file_persistence(temp_location): + _test_nonexistent_persistence(FilePersistence(temp_location)) + @pytest.mark.skipif( 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)) +@pytest.mark.skipif( + is_running_on_travis_ci or not sys.platform.startswith('win'), + reason="Requires Windows Desktop") +def test_nonexistent_file_persistence_with_data_protection(temp_location): + _test_nonexistent_persistence(FilePersistenceWithDataProtection(temp_location)) + @pytest.mark.skipif( not sys.platform.startswith('darwin'), reason="Requires OSX. Whether running on TRAVIS CI does not seem to matter.") @@ -42,6 +57,14 @@ def test_keychain_persistence(temp_location): _test_persistence_roundtrip(KeychainPersistence( temp_location, "my_service_name", "my_account_name")) +@pytest.mark.skipif( + not sys.platform.startswith('darwin'), + reason="Requires OSX. Whether running on TRAVIS CI does not seem to matter.") +def test_nonexistent_keychain_persistence(temp_location): + random_service_name = random_account_name = str(id(temp_location)) + _test_nonexistent_persistence( + KeychainPersistence(temp_location, random_service_name, random_account_name)) + @pytest.mark.skipif( is_running_on_travis_ci or not sys.platform.startswith('linux'), reason="Requires Linux Desktop. Headless or SSH session won't work.") @@ -52,3 +75,14 @@ def test_libsecret_persistence(temp_location): {"my_attr_1": "foo", "my_attr_2": "bar"}, )) +@pytest.mark.skipif( + is_running_on_travis_ci or not sys.platform.startswith('linux'), + reason="Requires Linux Desktop. Headless or SSH session won't work.") +def test_nonexistent_libsecret_persistence(temp_location): + random_schema_name = random_value = str(id(temp_location)) + _test_nonexistent_persistence(LibsecretPersistence( + temp_location, + random_schema_name, + {"my_attr_1": random_value, "my_attr_2": random_value}, + )) + From 5bad518f22095ae14836df95f632eed526f58ece Mon Sep 17 00:00:00 2001 From: Ray Luo Date: Fri, 28 Aug 2020 17:42:06 -0700 Subject: [PATCH 7/8] Change PersistenceNotFound's base from OSError to IOError --- msal_extensions/persistence.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/msal_extensions/persistence.py b/msal_extensions/persistence.py index 61a8cba..1ad5c8e 100644 --- a/msal_extensions/persistence.py +++ b/msal_extensions/persistence.py @@ -48,7 +48,11 @@ def _mkdir_p(path): # 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(OSError): +class PersistenceNotFound(IOError): # Use IOError rather than OSError as base, + # 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=errno.ENOENT, message="Persistence not found", location=None): From 6d2efabf7881348101abbf903c6723a26aff1a53 Mon Sep 17 00:00:00 2001 From: Abhidnya Date: Tue, 1 Sep 2020 13:21:00 -0700 Subject: [PATCH 8/8] MSAL Extensions for Python 0.3.0 Bumping version number --- msal_extensions/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msal_extensions/__init__.py b/msal_extensions/__init__.py index 53b457b..bfa02bd 100644 --- a/msal_extensions/__init__.py +++ b/msal_extensions/__init__.py @@ -1,5 +1,5 @@ """Provides auxiliary functionality to the `msal` package.""" -__version__ = "0.2.2" +__version__ = "0.3.0" import sys