SNOW-698526: Python connector 2.8.2 infinite loop in OCSP cache validation (#1348)

Author: sfc-gh-aling

DESCRIPTION.md

@@ -11,6 +11,7 @@ Source code is also available at: https://github.com/snowflakedb/snowflake-conne
 - v2.8.3(Unreleased)
   - Bumped cryptography dependency from <39.0.0 to <41.0.0
   - Fixed a bug where the permission of the file downloaded via GET command is changed
+  - Fixed a bug where expired OCSP response cache caused infinite recursion during cache loading
 
 - v2.8.2(November 18,2022)
 

src/snowflake/connector/cache.py

@@ -58,6 +58,8 @@ def __init__(
         self._cache: dict[K, CacheEntry[V]] = {}
         self._lock = Lock()
         self._reset_telemetry()
+        # aliasing _getitem to unify the api with SFDictFileCache
+        self._getitem_non_locking = self._getitem
 
     def __len__(self) -> int:
         with self._lock:
@@ -403,16 +405,20 @@ def __init__(
         if os.path.exists(self.file_path):
             self._load()
 
-    def _getitem(
+    def _getitem_non_locking(
         self,
         k: K,
         *,
         should_record_hits: bool = True,
     ) -> V:
-        """Non-locking version of __getitem__.
+        """Non-locking version of __getitem__ of SFDictFileCache.
 
         This should only be used by internal functions when already
         holding self._lock.
+
+        Note that we do not overwrite _getitem because _getitem is used by
+        self._load to clear in-memory expired caches. Overwriting would cause
+        infinite recursive call.
         """
         if k not in self._cache:
             loaded = self._load_if_should()

src/snowflake/connector/ocsp_snowflake.py

@@ -708,7 +708,7 @@ def find_cache(
             ocsp_response_validation_result = (
                 OCSP_RESPONSE_VALIDATION_CACHE[cache_key]
                 if lock_cache
-                else OCSP_RESPONSE_VALIDATION_CACHE._getitem(cache_key)
+                else OCSP_RESPONSE_VALIDATION_CACHE._getitem_non_locking(cache_key)
             )
             try:
                 # is_valid_time can raise exception if the cache

src/snowflake/connector/version.py

@@ -1,3 +1,3 @@
 # Update this for the versions
 # Don't change the forth version number from None
-VERSION = (2, 8, 2, None)
+VERSION = (2, 8, 3, None)

test/unit/test_cache.py

@@ -104,6 +104,7 @@ def test_get(self):
         c._entry_lifetime = NO_LIFETIME
         c["d"] = 4
         assert c.get("d") is None
+        assert c._getitem_non_locking(1) == "a"
 
     def test_items(self):
         c = cache.SFDictCache()
@@ -405,3 +406,25 @@ def test_clear(self, tmpdir):
         os.unlink(cache_path)
         c["a"] = 1
         assert os.path.exists(cache_path)
+
+    def test_load_with_expired_entries(self, tmpdir):
+        # Test: https://snowflakecomputing.atlassian.net/browse/SNOW-698526
+
+        # create cache first
+        cache_path = os.path.join(tmpdir, "cache.txt")
+        c1 = cache.SFDictFileCache(file_path=cache_path)
+        c1["a"] = 1
+        c1._save()
+
+        # load cache
+        c2 = cache.SFDictFileCache(
+            file_path=cache_path, entry_lifetime=int(NO_LIFETIME.total_seconds())
+        )
+        c2["b"] = 2
+        c2["c"] = 3
+        # cache will expire immediately due to the NO_LIFETIME setting
+        # when loading again, clearing cache logic will be triggered
+        # load will trigger clear expired entries, and then further call _getitem
+        c2._load()
+
+        assert len(c2) == 1 and c2["a"] == 1 and c2._getitem_non_locking("a") == 1