Skip to content

[Core] Replace unsafe pickle deserialization with JSON in MSAL HTTP cache #33405

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
notyashhh wants to merge 1 commit into
base: dev
Choose a base branch
from
+10 −12
Open

[Core] Replace unsafe pickle deserialization with JSON in MSAL HTTP cache #33405

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 10 additions & 12 deletions src/azure-cli-core/azure/cli/core/auth/binary_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
# --------------------------------------------------------------------------------------------

from collections.abc import MutableMapping
import pickle
import json
import os

from azure.cli.core.decorators import retry
from knack.log import get_logger
Expand All @@ -15,7 +16,7 @@
class BinaryCache(MutableMapping):
"""
Derived from azure.cli.core._session.Session.
A simple dict-like class that is backed by a binary file.
A simple dict-like class that is backed by a JSON file.

All direct modifications with `__setitem__` and `__delitem__` will save the file.
Indirect modifications should be followed by a call to `save`.
Expand All @@ -31,8 +32,8 @@ def __init__(self, file_name):
def _load(self):
"""Load cache with retry. If it still fails at last, raise the original exception as-is."""
try:
with open(self.filename, 'rb') as f:
return pickle.load(f)
with open(self.filename, 'r', encoding='utf-8') as f:
return json.load(f)
except FileNotFoundError:
# The cache file has not been created. This is expected. No need to retry.
logger.debug("%s not found. Using a fresh one.", self.filename)
Expand All @@ -44,19 +45,16 @@ def load(self):
self.data = self._load()
except Exception as ex: # pylint: disable=broad-exception-caught
# If we still get exception after retry, ignore all types of exceptions and use a new cache.
# - pickle.UnpicklingError is caused by corrupted cache file, perhaps due to concurrent writes.
# - EOFError is caused by empty cache file created by other az instance, but hasn't been filled yet.
# - AttributeError is caused by reading cache generated by different MSAL versions.
# - json.JSONDecodeError is caused by corrupted or legacy pickle cache file.
# - ValueError/KeyError from malformed JSON content.
Comment on lines +48 to +49
logger.debug("Failed to load cache: %s. Using a fresh one.", ex)
self.data = {} # Ignore a non-existing or corrupted http_cache

@retry()
def _save(self):
with open(self.filename, 'wb') as f:
# At this point, an empty cache file will be created. Loading this cache file will
# raise EOFError. This can be simulated by adding time.sleep(30) here.
# So during loading, EOFError is ignored.
pickle.dump(self.data, f)
fd = os.open(self.filename, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
with os.fdopen(fd, 'w', encoding='utf-8') as f:
json.dump(self.data, f)

def save(self):
logger.debug("save: %s", self.filename)
Expand Down
Loading