Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions tests/refresh_script.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import sys
import time

from tuf.ngclient import Updater

print(f"Fetching metadata {sys.argv[1]} times:")
print(f" metadata dir: {sys.argv[2]}")
print(f" metadata url: {sys.argv[3]}")

start = time.time()

for i in range(int(sys.argv[1])):
try:
refresh_start = time.time()
u = Updater(metadata_dir=sys.argv[2], metadata_base_url=sys.argv[3])
# file3.txt is delegated so we end up exercising all metadata load paths
u.get_targetinfo("file3.txt")
except OSError as e:
print(
f"Failed on iteration {i}, "
f"{time.time() - refresh_start} secs elapsed ({time.time() - start} total)"
)
raise e
4 changes: 3 additions & 1 deletion tests/test_updater_delegation_graphs.py
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,9 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None:
"""Assert that local metadata files match 'roles'"""
expected_files = [f"{role}.json" for role in roles]
found_files = [
e.name for e in os.scandir(self.metadata_dir) if e.is_file()
e.name
for e in os.scandir(self.metadata_dir)
if e.is_file() and e.name != ".lock"
]

self.assertListEqual(sorted(found_files), sorted(expected_files))
Expand Down
45 changes: 44 additions & 1 deletion tests/test_updater_ng.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import logging
import os
import shutil
import subprocess
import sys
import tempfile
import unittest
Expand Down Expand Up @@ -157,7 +158,9 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None:
"""Assert that local metadata files match 'roles'"""
expected_files = [f"{role}.json" for role in roles]
found_files = [
e.name for e in os.scandir(self.client_directory) if e.is_file()
e.name
for e in os.scandir(self.client_directory)
if e.is_file() and e.name != ".lock"
]

self.assertListEqual(sorted(found_files), sorted(expected_files))
Expand Down Expand Up @@ -353,6 +356,46 @@ def test_user_agent(self) -> None:

self.assertEqual(ua[:23], "MyApp/1.2.3 python-tuf/")

def test_parallel_updaters(self) -> None:
# Refresh many updaters in parallel many times, using the same local metadata cache.
# This should reveal race conditions.

iterations = 50
process_count = 10

project_root_dir = os.path.dirname(utils.TESTS_DIR)

command = [
sys.executable,
"-m",
"tests.refresh_script",
str(iterations),
self.client_directory,
self.metadata_url,
]

procs = [
subprocess.Popen(
command,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
cwd=project_root_dir,
)
for _ in range(process_count)
]

errout = ""
for proc in procs:
stdout, stderr = proc.communicate()
if proc.returncode != 0:
errout += "Parallel Refresh script failed:"
errout += f"\nprocess stdout: \n{stdout.decode()}"
errout += f"\nprocess stderr: \n{stderr.decode()}"
if errout:
self.fail(
f"One or more scripts failed parallel refresh test:\n{errout}"
)


if __name__ == "__main__":
utils.configure_test_logging(sys.argv)
Expand Down
14 changes: 9 additions & 5 deletions tests/test_updater_top_level_update.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ def _assert_files_exist(self, roles: Iterable[str]) -> None:
"""Assert that local metadata files match 'roles'"""
expected_files = [f"{role}.json" for role in roles]
found_files = [
e.name for e in os.scandir(self.metadata_dir) if e.is_file()
e.name
for e in os.scandir(self.metadata_dir)
if e.is_file() and e.name != ".lock"
]

self.assertListEqual(sorted(found_files), sorted(expected_files))
Expand Down Expand Up @@ -133,8 +135,7 @@ def test_cached_root_missing_without_bootstrap(self) -> None:
self._run_refresh(skip_bootstrap=True)

# Metadata dir is empty
with self.assertRaises(FileNotFoundError):
os.listdir(self.metadata_dir)
self._assert_files_exist([])

def test_trusted_root_expired(self) -> None:
# Create an expired root version
Expand Down Expand Up @@ -644,14 +645,16 @@ def test_not_loading_targets_twice(self, wrapped_open: MagicMock) -> None:
wrapped_open.reset_mock()

# First time looking for "somepath", only 'role1' must be loaded
# (and ".lock" for metadata locking)
updater.get_targetinfo("somepath")
wrapped_open.assert_called_once_with(
self.assertEqual(wrapped_open.call_count, 2)
wrapped_open.assert_called_with(
os.path.join(self.metadata_dir, "role1.json"), "rb"
)
wrapped_open.reset_mock()
# Second call to get_targetinfo, all metadata is already loaded
updater.get_targetinfo("somepath")
wrapped_open.assert_not_called()
self.assertEqual(wrapped_open.call_count, 1)

def test_snapshot_rollback_with_local_snapshot_hash_mismatch(self) -> None:
# Test triggering snapshot rollback check on a newly downloaded snapshot
Expand Down Expand Up @@ -709,6 +712,7 @@ def test_load_metadata_from_cache(self, wrapped_open: MagicMock) -> None:
root_dir = os.path.join(self.metadata_dir, "root_history")
wrapped_open.assert_has_calls(
[
call(os.path.join(self.metadata_dir, ".lock"), "wb"),
call(os.path.join(root_dir, "2.root.json"), "rb"),
call(os.path.join(self.metadata_dir, "timestamp.json"), "rb"),
call(os.path.join(self.metadata_dir, "snapshot.json"), "rb"),
Expand Down
4 changes: 2 additions & 2 deletions tests/test_updater_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ def test_local_target_storage_fail(self) -> None:

def test_non_existing_metadata_dir(self) -> None:
with self.assertRaises(FileNotFoundError):
# Initialize Updater with non-existing metadata_dir
# Initialize Updater with non-existing metadata_dir and no bootstrap root
Updater(
"non_existing_metadata_dir",
f"{self.temp_dir.name}/non_existing_metadata_dir",
"https://example.com/metadata/",
fetcher=self.sim,
)
Expand Down
2 changes: 1 addition & 1 deletion tests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def cleanup_metadata_dir(path: str) -> None:
for entry in it:
if entry.name == "root_history":
cleanup_metadata_dir(entry.path)
elif entry.name.endswith(".json"):
elif entry.name.endswith(".json") or entry.name == ".lock":
os.remove(entry.path)
else:
raise ValueError(f"Unexpected local metadata file {entry.path}")
Expand Down
4 changes: 2 additions & 2 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ skipsdist = true
[testenv]
commands =
python3 --version
python3 -m coverage run -m unittest
python3 -m coverage report -m --fail-under 97
python3 -m coverage run -m unittest -v
python3 -m coverage report -m --fail-under 96

deps =
-r{toxinidir}/requirements/test.txt
Expand Down
56 changes: 56 additions & 0 deletions tuf/ngclient/_internal/file_lock.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import logging
import time
from collections.abc import Iterator
from contextlib import contextmanager
from typing import IO

logger = logging.getLogger(__name__)

try:
# advisory file locking for posix
import fcntl

@contextmanager
def lock_file(path: str) -> Iterator[IO]:
with open(path, "wb") as f:
fcntl.lockf(f, fcntl.LOCK_EX)
yield f

except ModuleNotFoundError:
# Windows file locking, in belt-and-suspenders-from-Temu style:
# Use a loop that tries to open the lockfile for 30 secs, but also
# use msvcrt.locking().
# * since open() usually just fails when another process has the file open
# msvcrt.locking() almost never gets called when there is a lock. open()
# sometimes succeeds for multiple processes though
# * msvcrt.locking() does not even block until file is available: it just
# tries once per second in a non-blocking manner for 10 seconds. So if
# another process keeps opening the file it's unlikely that we actually
# get the lock
import msvcrt

@contextmanager
def lock_file(path: str) -> Iterator[IO]:
err = None
locked = False
for _ in range(100):
try:
with open(path, "wb") as f:
msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1)
locked = True
yield f
return
except FileNotFoundError:
# could be from yield or from open() -- either way we bail
raise
except OSError as e:
if locked:
# yield has raised, let's not continue loop
raise e
err = e
logger.warning("Unsuccessful lock attempt for %s: %s", path, e)
time.sleep(0.3)

# raise the last failure if we never got a lock
if err is not None:
raise err
62 changes: 40 additions & 22 deletions tuf/ngclient/updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@
* ``Updater.download_target()`` downloads a target file and ensures it is
verified correct by the metadata.

Note that applications using ``Updater`` should be 'single instance'
applications: running multiple instances that use the same cache directories at
the same time is not supported.

A simple example of using the Updater to implement a Python TUF client that
downloads target files is available in `examples/client
<https://github.com/theupdateframework/python-tuf/tree/develop/examples/client>`_.
Expand Down Expand Up @@ -64,11 +60,14 @@

from tuf.api import exceptions
from tuf.api.metadata import Root, Snapshot, TargetFile, Targets, Timestamp
from tuf.ngclient._internal.file_lock import lock_file
from tuf.ngclient._internal.trusted_metadata_set import TrustedMetadataSet
from tuf.ngclient.config import EnvelopeType, UpdaterConfig
from tuf.ngclient.urllib3_fetcher import Urllib3Fetcher

if TYPE_CHECKING:
from collections.abc import Iterator

from tuf.ngclient.fetcher import FetcherInterface

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -131,16 +130,30 @@ def __init__(
f"got '{self.config.envelope_type}'"
)

if not bootstrap:
# if no root was provided, use the cached non-versioned root.json
bootstrap = self._load_local_metadata(Root.type)
# Ensure the whole metadata directory structure exists
rootdir = Path(self._dir, "root_history")
rootdir.mkdir(exist_ok=True, parents=True)

# Load the initial root, make sure it's cached
self._trusted_set = TrustedMetadataSet(
bootstrap, self.config.envelope_type
)
self._persist_root(self._trusted_set.root.version, bootstrap)
self._update_root_symlink()
with self._lock_metadata():
if not bootstrap:
# if no root was provided, use the cached non-versioned root
bootstrap = self._load_local_metadata(Root.type)

# Load the initial root, make sure it's cached
self._trusted_set = TrustedMetadataSet(
bootstrap, self.config.envelope_type
)
self._persist_root(self._trusted_set.root.version, bootstrap)
self._update_root_symlink()

@contextlib.contextmanager
def _lock_metadata(self) -> Iterator[None]:
"""Context manager for locking the metadata directory."""

logger.debug("Getting metadata lock...")
with lock_file(os.path.join(self._dir, ".lock")):
yield
logger.debug("Released metadata lock")

def refresh(self) -> None:
"""Refresh top-level metadata.
Expand All @@ -166,10 +179,11 @@ def refresh(self) -> None:
DownloadError: Download of a metadata file failed in some way
"""

self._load_root()
self._load_timestamp()
self._load_snapshot()
self._load_targets(Targets.type, Root.type)
with self._lock_metadata():
self._load_root()
self._load_timestamp()
self._load_snapshot()
self._load_targets(Targets.type, Root.type)

def _generate_target_file_path(self, targetinfo: TargetFile) -> str:
if self.target_dir is None:
Expand Down Expand Up @@ -205,9 +219,14 @@ def get_targetinfo(self, target_path: str) -> TargetFile | None:
``TargetFile`` instance or ``None``.
"""

if Targets.type not in self._trusted_set:
self.refresh()
return self._preorder_depth_first_walk(target_path)
with self._lock_metadata():
if Targets.type not in self._trusted_set:
# implicit refresh
self._load_root()
self._load_timestamp()
self._load_snapshot()
self._load_targets(Targets.type, Root.type)
return self._preorder_depth_first_walk(target_path)

def find_cached_target(
self,
Expand Down Expand Up @@ -295,7 +314,7 @@ def download_target(
targetinfo.verify_length_and_hashes(target_file)

target_file.seek(0)
with open(filepath, "wb") as destination_file:
with lock_file(filepath) as destination_file:
shutil.copyfileobj(target_file, destination_file)

logger.debug("Downloaded target %s", targetinfo.path)
Expand Down Expand Up @@ -335,7 +354,6 @@ def _persist_root(self, version: int, data: bytes) -> None:
"root_history/1.root.json").
"""
rootdir = Path(self._dir, "root_history")
rootdir.mkdir(exist_ok=True, parents=True)
self._persist_file(str(rootdir / f"{version}.root.json"), data)

def _persist_file(self, filename: str, data: bytes) -> None:
Expand Down
Loading