From 80ff532be1e4134e05a11287d31fc88ddf78edf3 Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Thu, 29 Apr 2021 09:28:06 +0300 Subject: [PATCH 01/10] Download metadata from a single mirror Keep the current API and mirrors configuration but use only the first mirror from the list for metadata download. Target files download remains unchanged. Signed-off-by: Teodora Sechkova --- tuf/client_rework/updater_rework.py | 208 ++++++++++------------------ 1 file changed, 77 insertions(+), 131 deletions(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index c6872c6478..8a643204a3 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -223,37 +223,42 @@ def _load_root(self) -> None: for next_version in range(lower_bound, upper_bound): try: + # Get the list of mirrors but we'll use only the first one root_mirrors = mirrors.get_list_of_mirrors( "meta", f"{next_version}.root.json", self._mirrors, ) + temp_obj = None # For each version of root iterate over the list of mirrors # until an intermediate root is successfully downloaded and # verified. - intermediate_root = self._root_mirrors_download(root_mirrors) - - # Exit the loop when all mirrors have raised only 403 / 404 errors, - # which indicates that a bigger root version does not exist. - except exceptions.NoWorkingMirrorError as exception: - for mirror_error in exception.mirror_errors.values(): - # Otherwise, reraise the error, because it is not a simple - # HTTP error. - if neither_403_nor_404(mirror_error): - logger.info( - "Misc error for root version " + str(next_version) - ) - raise + temp_obj = download.download_file( + root_mirrors[0], + settings.DEFAULT_ROOT_REQUIRED_LENGTH, + self._fetcher, + strict_required_length=False, + ) - logger.debug("HTTP error for root version " + str(next_version)) - # If we are here, then we ran into only 403 / 404 errors, which - # are good reasons to suspect that the next root metadata file - # does not exist. + temp_obj.seek(0) + intermediate_root = self._verify_root(temp_obj.read()) + # TODO: persist should happen here for each intermediate + # root according to the spec + + except exceptions.FetcherHTTPError as exception: + if exception.status_code not in {403, 404}: + raise + # Stop looking for a bigger version if "File not found" + # error is received break - # Continue only if a newer root version is found - if intermediate_root is not None: + finally: + if temp_obj: + temp_obj.close() + temp_obj = None + + if intermediate_root: # Check for a freeze attack. The latest known time MUST be lower # than the expiration timestamp in the trusted root metadata file # TODO define which exceptions are part of the public API @@ -288,52 +293,6 @@ def _load_root(self) -> None: "root" ].signed.consistent_snapshot - def _root_mirrors_download(self, root_mirrors: Dict) -> "RootWrapper": - """Iterate over the list of "root_mirrors" until an intermediate - root is successfully downloaded and verified. - Raise "NoWorkingMirrorError" if a root file cannot be downloaded or - verified from any mirror""" - - file_mirror_errors = {} - temp_obj = None - intermediate_root = None - - for root_mirror in root_mirrors: - try: - temp_obj = download.download_file( - root_mirror, - settings.DEFAULT_ROOT_REQUIRED_LENGTH, - self._fetcher, - strict_required_length=False, - ) - - temp_obj.seek(0) - intermediate_root = self._verify_root(temp_obj.read()) - # When we reach this point, a root file has been successfully - # downloaded and verified so we can exit the loop. - break - - # pylint cannot figure out that we store the exceptions - # in a dictionary to raise them later so we disable - # the warning. This should be reviewed in the future still. - except Exception as exception: # pylint: disable=broad-except - # Store the exceptions until all mirrors are iterated. - # If an exception is raised from one mirror but a valid - # file is found in the next one, the first exception is ignored. - file_mirror_errors[root_mirror] = exception - - finally: - if temp_obj: - temp_obj.close() - temp_obj = None - - if not intermediate_root: - # If all mirrors are tried but a valid root file is not found, - # then raise an exception with the stored errors - raise exceptions.NoWorkingMirrorError(file_mirror_errors) - - return intermediate_root - def _load_timestamp(self) -> None: """ TODO @@ -344,31 +303,27 @@ def _load_timestamp(self) -> None: "meta", "timestamp.json", self._mirrors ) - file_mirror_errors = {} verified_timestamp = None - for file_mirror in file_mirrors: - try: - temp_obj = download.download_file( - file_mirror, - settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, - self._fetcher, - strict_required_length=False, - ) - - temp_obj.seek(0) - verified_timestamp = self._verify_timestamp(temp_obj.read()) - break + temp_obj = None + try: + temp_obj = download.download_file( + file_mirrors[0], + settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, + self._fetcher, + strict_required_length=False, + ) - except Exception as exception: # pylint: disable=broad-except - file_mirror_errors[file_mirror] = exception + temp_obj.seek(0) + verified_timestamp = self._verify_timestamp(temp_obj.read()) - finally: - if temp_obj: - temp_obj.close() - temp_obj = None + except Exception as e: + # TODO: do we reraise a NoWorkingMirrorError or just + # let exceptions propagate? + raise exceptions.NoWorkingMirrorError({file_mirrors[0]: e}) from e - if not verified_timestamp: - raise exceptions.NoWorkingMirrorError(file_mirror_errors) + finally: + if temp_obj: + temp_obj.close() self._metadata["timestamp"] = verified_timestamp # Persist root metadata. The client MUST write the file to @@ -393,36 +348,31 @@ def _load_snapshot(self) -> None: # version = None # TODO: Check if exists locally - file_mirrors = mirrors.get_list_of_mirrors( "meta", "snapshot.json", self._mirrors ) - file_mirror_errors = {} verified_snapshot = False - for file_mirror in file_mirrors: - try: - temp_obj = download.download_file( - file_mirror, - length, - self._fetcher, - strict_required_length=False, - ) + temp_obj = None + try: + temp_obj = download.download_file( + file_mirrors[0], + length, + self._fetcher, + strict_required_length=False, + ) - temp_obj.seek(0) - verified_snapshot = self._verify_snapshot(temp_obj.read()) - break + temp_obj.seek(0) + verified_snapshot = self._verify_snapshot(temp_obj.read()) - except Exception as exception: # pylint: disable=broad-except - file_mirror_errors[file_mirror] = exception + except Exception as e: + # TODO: do we reraise a NoWorkingMirrorError or just + # let exceptions propagate? + raise exceptions.NoWorkingMirrorError({file_mirrors[0]: e}) from e - finally: - if temp_obj: - temp_obj.close() - temp_obj = None - - if not verified_snapshot: - raise exceptions.NoWorkingMirrorError(file_mirror_errors) + finally: + if temp_obj: + temp_obj.close() self._metadata["snapshot"] = verified_snapshot # Persist root metadata. The client MUST write the file to @@ -452,33 +402,29 @@ def _load_targets(self, targets_role: str, parent_role: str) -> None: "meta", f"{targets_role}.json", self._mirrors ) - file_mirror_errors = {} verified_targets = False - for file_mirror in file_mirrors: - try: - temp_obj = download.download_file( - file_mirror, - length, - self._fetcher, - strict_required_length=False, - ) - - temp_obj.seek(0) - verified_targets = self._verify_targets( - temp_obj.read(), targets_role, parent_role - ) - break + temp_obj = None + try: + temp_obj = download.download_file( + file_mirrors[0], + length, + self._fetcher, + strict_required_length=False, + ) - except Exception as exception: # pylint: disable=broad-except - file_mirror_errors[file_mirror] = exception + temp_obj.seek(0) + verified_targets = self._verify_targets( + temp_obj.read(), targets_role, parent_role + ) - finally: - if temp_obj: - temp_obj.close() - temp_obj = None + except Exception as e: + # TODO: do we reraise a NoWorkingMirrorError or just + # let exceptions propagate? + raise exceptions.NoWorkingMirrorError({file_mirrors[0]: e}) from e - if not verified_targets: - raise exceptions.NoWorkingMirrorError(file_mirror_errors) + finally: + if temp_obj: + temp_obj.close() self._metadata[targets_role] = verified_targets # Persist root metadata. The client MUST write the file to From 576d055cd402a5297424563a77b0a5a96304275b Mon Sep 17 00:00:00 2001 From: Teodora Sechkova Date: Thu, 29 Apr 2021 12:07:25 +0300 Subject: [PATCH 02/10] Drop mirrors support Updater now uses only a single url for metadata download. Target files download use either a default url or an optional one for each file passed by the caller. Signed-off-by: Teodora Sechkova --- tests/test_updater_rework.py | 9 +- tuf/client_rework/mirrors.py | 130 -------------------------- tuf/client_rework/updater_rework.py | 136 +++++++++++++--------------- 3 files changed, 66 insertions(+), 209 deletions(-) delete mode 100644 tuf/client_rework/mirrors.py diff --git a/tests/test_updater_rework.py b/tests/test_updater_rework.py index bc6ce3a3f1..162fa5b1f9 100644 --- a/tests/test_updater_rework.py +++ b/tests/test_updater_rework.py @@ -123,14 +123,13 @@ def setUp(self): # directory copied from the original repository files. tuf.settings.repositories_directory = self.client_directory - self.repository_mirrors = {'mirror1': {'url_prefix': url_prefix, - 'metadata_path': 'metadata', - 'targets_path': 'targets'}} - + metadata_url = os.path.join(url_prefix, 'metadata/') + targets_url = os.path.join(url_prefix, 'targets/') # Creating a repository instance. The test cases will use this client # updater to refresh metadata, fetch target files, etc. self.repository_updater = updater.Updater(self.repository_name, - self.repository_mirrors) + metadata_url, + targets_url) # Metadata role keys are needed by the test cases to make changes to the # repository (e.g., adding a new target file to 'targets.json' and then diff --git a/tuf/client_rework/mirrors.py b/tuf/client_rework/mirrors.py deleted file mode 100644 index 83991e64e6..0000000000 --- a/tuf/client_rework/mirrors.py +++ /dev/null @@ -1,130 +0,0 @@ -#!/usr/bin/env python - -# Copyright 2012 - 2017, New York University and the TUF contributors -# SPDX-License-Identifier: MIT OR Apache-2.0 - -""" - - mirrors.py - - - Konstantin Andrianov. - Derived from original mirrors.py written by Geremy Condra. - - - March 12, 2012. - - - See LICENSE-MIT OR LICENSE for licensing information. - - - Extract a list of mirror urls corresponding to the file type and the location - of the file with respect to the base url. -""" - -import os -from urllib import parse - -from securesystemslib import exceptions as sslib_exceptions -from securesystemslib import formats as sslib_formats -from securesystemslib import util as sslib_util - -from tuf import formats - -# The type of file to be downloaded from a repository. The -# 'get_list_of_mirrors' function supports these file types. -_SUPPORTED_FILE_TYPES = ["meta", "target"] - - -def get_list_of_mirrors(file_type, file_path, mirrors_dict): - """ - - Get a list of mirror urls from a mirrors dictionary, provided the type - and the path of the file with respect to the base url. - - - file_type: - Type of data needed for download, must correspond to one of the strings - in the list ['meta', 'target']. 'meta' for metadata file type or - 'target' for target file type. It should correspond to - NAME_SCHEMA format. - - file_path: - A relative path to the file that corresponds to RELPATH_SCHEMA format. - Ex: 'http://url_prefix/targets_path/file_path' - - mirrors_dict: - A mirrors_dict object that corresponds to MIRRORDICT_SCHEMA, where - keys are strings and values are MIRROR_SCHEMA. An example format - of MIRROR_SCHEMA: - - {'url_prefix': 'http://localhost:8001', - 'metadata_path': 'metadata/', - 'targets_path': 'targets/', - 'confined_target_dirs': ['targets/snapshot1/', ...], - 'custom': {...}} - - The 'custom' field is optional. - - - securesystemslib.exceptions.Error, on unsupported 'file_type'. - - securesystemslib.exceptions.FormatError, on bad argument. - - - List of mirror urls corresponding to the file_type and file_path. If no - match is found, empty list is returned. - """ - - # Checking if all the arguments have appropriate format. - formats.RELPATH_SCHEMA.check_match(file_path) - formats.MIRRORDICT_SCHEMA.check_match(mirrors_dict) - sslib_formats.NAME_SCHEMA.check_match(file_type) - - # Verify 'file_type' is supported. - if file_type not in _SUPPORTED_FILE_TYPES: - raise sslib_exceptions.Error( - "Invalid file_type argument." - " Supported file types: " + repr(_SUPPORTED_FILE_TYPES) - ) - path_key = "metadata_path" if file_type == "meta" else "targets_path" - - # Reference to 'securesystemslib.util.file_in_confined_directories()' - # (improve readability). This function checks whether a mirror should - # serve a file to the client. A client may be confined to certain paths - # on a repository mirror when fetching target files. This field may be set - # by the client when the repository mirror is added to the - # 'tuf.client.updater.Updater' object. - in_confined_directory = sslib_util.file_in_confined_directories - - list_of_mirrors = [] - for mirror_info in mirrors_dict.values(): - # Does mirror serve this file type at all? - path = mirror_info.get(path_key) - if path is None: - continue - - # for targets, ensure directory confinement - if path_key == "targets_path": - full_filepath = os.path.join(path, file_path) - confined_target_dirs = mirror_info.get("confined_target_dirs") - # confined_target_dirs is an optional field - if confined_target_dirs and not in_confined_directory( - full_filepath, confined_target_dirs - ): - continue - - # urllib.quote(string) replaces special characters in string using - # the %xx escape. This is done to avoid parsing issues of the URL - # on the server side. Do *NOT* pass URLs with Unicode characters without - # first encoding the URL as UTF-8. Needed a long-term solution with #61. - # http://bugs.python.org/issue1712522 - file_path = parse.quote(file_path) - url = os.path.join(mirror_info["url_prefix"], path, file_path) - - # The above os.path.join() result as well as input file_path may be - # invalid on windows (might contain both separator types), see #1077. - # Make sure the URL doesn't contain backward slashes on Windows. - list_of_mirrors.append(url.replace("\\", "/")) - - return list_of_mirrors diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index 8a643204a3..9fbae6edc1 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -11,6 +11,7 @@ import logging import os from typing import Dict, Optional +from urllib import parse from securesystemslib import exceptions as sslib_exceptions from securesystemslib import hash as sslib_hash @@ -18,7 +19,7 @@ from tuf import exceptions, settings from tuf.client.fetcher import FetcherInterface -from tuf.client_rework import download, mirrors, requests_fetcher +from tuf.client_rework import download, requests_fetcher from .metadata_wrapper import ( RootWrapper, @@ -35,27 +36,26 @@ class Updater: """ Provides a class that can download target files securely. - Attributes: - metadata: - - repository_name: - - mirrors: - - fetcher: - - consistent_snapshot: + TODO """ def __init__( self, repository_name: str, - repository_mirrors: Dict, + metadata_url: str, + default_target_url: Optional[str] = None, fetcher: Optional[FetcherInterface] = None, ): self._repository_name = repository_name - self._mirrors = repository_mirrors + self._metadata_url = metadata_url + # Should we accept metadata url as a default for targets or + # targets_url should be provided either in this constructor + # or as a download_target parameter? + if default_target_url is None: + self._default_target_url = metadata_url + else: + self._default_target_url = default_target_url self._consistent_snapshot = False self._metadata = {} @@ -142,44 +142,38 @@ def updated_targets(targets: Dict, destination_directory: str) -> Dict: return updated_targets - def download_target(self, target: Dict, destination_directory: str): + def download_target( + self, + targetinfo: Dict, + destination_directory: str, + target_url: Optional[str] = None, + ): """ This method performs the actual download of the specified target. The file is saved to the 'destination_directory' argument. """ + if target_url is None: + target_url = self._default_target_url - temp_obj = None - file_mirror_errors = {} - file_mirrors = mirrors.get_list_of_mirrors( - "target", target["filepath"], self._mirrors - ) + full_url = _build_full_url(target_url, targetinfo["filepath"]) - for file_mirror in file_mirrors: - try: - temp_obj = download.download_file( - file_mirror, target["fileinfo"]["length"], self._fetcher - ) - _check_file_length(temp_obj, target["fileinfo"]["length"]) - temp_obj.seek(0) - _check_hashes_obj(temp_obj, target["fileinfo"]["hashes"]) - break - - except Exception as exception: # pylint: disable=broad-except - # Store the exceptions until all mirrors are iterated. - # If an exception is raised from one mirror but a valid - # file is found in the next one, the first exception is ignored. - file_mirror_errors[file_mirror] = exception - - if temp_obj: - temp_obj.close() - temp_obj = None + temp_obj = None + try: + temp_obj = download.download_file( + full_url, targetinfo["fileinfo"]["length"], self._fetcher + ) + _check_file_length(temp_obj, targetinfo["fileinfo"]["length"]) + temp_obj.seek(0) + _check_hashes_obj(temp_obj, targetinfo["fileinfo"]["hashes"]) - # If all mirrors are iterated but a file object is not successfully - # downloaded and verifies, raise the collected errors - if not temp_obj: - raise exceptions.NoWorkingMirrorError(file_mirror_errors) + except Exception as e: + if temp_obj: + temp_obj.close() + # TODO: do we reraise a NoWorkingMirrorError or just + # let exceptions propagate? + raise exceptions.NoWorkingMirrorError({full_url: e}) from e - filepath = os.path.join(destination_directory, target["filepath"]) + filepath = os.path.join(destination_directory, targetinfo["filepath"]) sslib_util.persist_temp_file(temp_obj, filepath) temp_obj.close() @@ -223,19 +217,15 @@ def _load_root(self) -> None: for next_version in range(lower_bound, upper_bound): try: - # Get the list of mirrors but we'll use only the first one - root_mirrors = mirrors.get_list_of_mirrors( - "meta", - f"{next_version}.root.json", - self._mirrors, + root_url = _build_full_url( + self._metadata_url, f"{next_version}.root.json" ) - temp_obj = None # For each version of root iterate over the list of mirrors # until an intermediate root is successfully downloaded and # verified. temp_obj = download.download_file( - root_mirrors[0], + root_url, settings.DEFAULT_ROOT_REQUIRED_LENGTH, self._fetcher, strict_required_length=False, @@ -298,16 +288,12 @@ def _load_timestamp(self) -> None: TODO """ # TODO Check if timestamp exists locally - - file_mirrors = mirrors.get_list_of_mirrors( - "meta", "timestamp.json", self._mirrors - ) - + timestamp_url = _build_full_url(self._metadata_url, "timestamp.json") verified_timestamp = None temp_obj = None try: temp_obj = download.download_file( - file_mirrors[0], + timestamp_url, settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, self._fetcher, strict_required_length=False, @@ -319,7 +305,7 @@ def _load_timestamp(self) -> None: except Exception as e: # TODO: do we reraise a NoWorkingMirrorError or just # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({file_mirrors[0]: e}) from e + raise exceptions.NoWorkingMirrorError({timestamp_url: e}) from e finally: if temp_obj: @@ -348,15 +334,12 @@ def _load_snapshot(self) -> None: # version = None # TODO: Check if exists locally - file_mirrors = mirrors.get_list_of_mirrors( - "meta", "snapshot.json", self._mirrors - ) - + snapshot_url = _build_full_url(self._metadata_url, "snapshot.json") verified_snapshot = False temp_obj = None try: temp_obj = download.download_file( - file_mirrors[0], + snapshot_url, length, self._fetcher, strict_required_length=False, @@ -368,7 +351,7 @@ def _load_snapshot(self) -> None: except Exception as e: # TODO: do we reraise a NoWorkingMirrorError or just # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({file_mirrors[0]: e}) from e + raise exceptions.NoWorkingMirrorError({snapshot_url: e}) from e finally: if temp_obj: @@ -398,15 +381,14 @@ def _load_targets(self, targets_role: str, parent_role: str) -> None: # TODO: Check if exists locally - file_mirrors = mirrors.get_list_of_mirrors( - "meta", f"{targets_role}.json", self._mirrors + targets_url = _build_full_url( + self._metadata_url, f"{targets_role}.json" ) - verified_targets = False temp_obj = None try: temp_obj = download.download_file( - file_mirrors[0], + targets_url, length, self._fetcher, strict_required_length=False, @@ -420,7 +402,7 @@ def _load_targets(self, targets_role: str, parent_role: str) -> None: except Exception as e: # TODO: do we reraise a NoWorkingMirrorError or just # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({file_mirrors[0]: e}) from e + raise exceptions.NoWorkingMirrorError({targets_url: e}) from e finally: if temp_obj: @@ -849,11 +831,17 @@ def _get_target_hash(target_filepath, hash_function="sha256"): return target_filepath_hash -def neither_403_nor_404(mirror_error): +def _build_full_url(base_url, filepath): """ - TODO + Build a full “absolute" URL by combining a base URL with + a relative file path. """ - if isinstance(mirror_error, exceptions.FetcherHTTPError): - if mirror_error.status_code in {403, 404}: - return False - return True + # Are these steps enough? Or too much? Is this the right place? + filepath = parse.quote(filepath) + # Assuming that base_url ends with a '/' character, otherwise parse.urljoin + # omits (correcly) the last part of the base URL path + full_url = parse.urljoin(base_url, filepath) + # Avoid windows path separators. Should we keep this check? Or correct + # paths should be required from the user? + # full_url.replace("\\", "/") + return full_url From 45259cfdb22dd55ed85fb1a63e815a9d8ac4ad4c Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 4 May 2021 10:33:03 +0300 Subject: [PATCH 03/10] new updater: Seek to beginning of file after length check Signed-off-by: Jussi Kukkonen --- tuf/client_rework/updater_rework.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index 9fbae6edc1..93aa22fca5 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -163,7 +163,6 @@ def download_target( full_url, targetinfo["fileinfo"]["length"], self._fetcher ) _check_file_length(temp_obj, targetinfo["fileinfo"]["length"]) - temp_obj.seek(0) _check_hashes_obj(temp_obj, targetinfo["fileinfo"]["hashes"]) except Exception as e: @@ -766,6 +765,7 @@ def _check_file_length(file_object, trusted_file_length): """ file_object.seek(0, 2) observed_length = file_object.tell() + file_object.seek(0) # Return and log a message if the length 'file_object' is equal to # 'trusted_file_length', otherwise raise an exception. A hard check From 5732163229ffa42dee6239f339496749ae84f04f Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 4 May 2021 11:08:08 +0300 Subject: [PATCH 04/10] new updater: remove unnecessary file objects Removing mirrors means we no longer need to do file object handling manually. Note that this means we're now exposing the Updater caller to all kinds of new exceptions (as NoWorkingMirrorError is no longer an excuse we can use). Signed-off-by: Jussi Kukkonen --- tuf/client_rework/download.py | 12 +++ tuf/client_rework/updater_rework.py | 135 +++++++--------------------- 2 files changed, 46 insertions(+), 101 deletions(-) diff --git a/tuf/client_rework/download.py b/tuf/client_rework/download.py index ec7b3e1ec0..fc53b70e12 100644 --- a/tuf/client_rework/download.py +++ b/tuf/client_rework/download.py @@ -138,9 +138,21 @@ def download_file(url, required_length, fetcher, strict_required_length=True): raise else: + temp_file.seek(0) return temp_file +def download_bytes(url, required_length, fetcher, strict_required_length=True): + """Download bytes from given url + + Returns the downloaded bytes, otherwise like download_file() + """ + with download_file( + url, required_length, fetcher, strict_required_length + ) as dl_file: + return dl_file.read() + + def _check_downloaded_length( total_downloaded, required_length, diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index 93aa22fca5..d964525806 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -157,24 +157,16 @@ def download_target( full_url = _build_full_url(target_url, targetinfo["filepath"]) - temp_obj = None - try: - temp_obj = download.download_file( - full_url, targetinfo["fileinfo"]["length"], self._fetcher + with download.download_file( + full_url, targetinfo["fileinfo"]["length"], self._fetcher + ) as target_file: + _check_file_length(target_file, targetinfo["fileinfo"]["length"]) + _check_hashes_obj(target_file, targetinfo["fileinfo"]["hashes"]) + + filepath = os.path.join( + destination_directory, targetinfo["filepath"] ) - _check_file_length(temp_obj, targetinfo["fileinfo"]["length"]) - _check_hashes_obj(temp_obj, targetinfo["fileinfo"]["hashes"]) - - except Exception as e: - if temp_obj: - temp_obj.close() - # TODO: do we reraise a NoWorkingMirrorError or just - # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({full_url: e}) from e - - filepath = os.path.join(destination_directory, targetinfo["filepath"]) - sslib_util.persist_temp_file(temp_obj, filepath) - temp_obj.close() + sslib_util.persist_temp_file(target_file, filepath) def _get_full_meta_name( self, role: str, extension: str = ".json", version: int = None @@ -219,19 +211,17 @@ def _load_root(self) -> None: root_url = _build_full_url( self._metadata_url, f"{next_version}.root.json" ) - temp_obj = None # For each version of root iterate over the list of mirrors # until an intermediate root is successfully downloaded and # verified. - temp_obj = download.download_file( + data = download.download_bytes( root_url, settings.DEFAULT_ROOT_REQUIRED_LENGTH, self._fetcher, strict_required_length=False, ) - temp_obj.seek(0) - intermediate_root = self._verify_root(temp_obj.read()) + intermediate_root = self._verify_root(data) # TODO: persist should happen here for each intermediate # root according to the spec @@ -242,11 +232,6 @@ def _load_root(self) -> None: # error is received break - finally: - if temp_obj: - temp_obj.close() - temp_obj = None - if intermediate_root: # Check for a freeze attack. The latest known time MUST be lower # than the expiration timestamp in the trusted root metadata file @@ -288,31 +273,13 @@ def _load_timestamp(self) -> None: """ # TODO Check if timestamp exists locally timestamp_url = _build_full_url(self._metadata_url, "timestamp.json") - verified_timestamp = None - temp_obj = None - try: - temp_obj = download.download_file( - timestamp_url, - settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, - self._fetcher, - strict_required_length=False, - ) - - temp_obj.seek(0) - verified_timestamp = self._verify_timestamp(temp_obj.read()) - - except Exception as e: - # TODO: do we reraise a NoWorkingMirrorError or just - # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({timestamp_url: e}) from e - - finally: - if temp_obj: - temp_obj.close() - - self._metadata["timestamp"] = verified_timestamp - # Persist root metadata. The client MUST write the file to - # non-volatile storage as FILENAME.EXT (e.g. root.json). + data = download.download_bytes( + timestamp_url, + settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, + self._fetcher, + strict_required_length=False, + ) + self._metadata["timestamp"] = self._verify_timestamp(data) self._metadata["timestamp"].persist( self._get_full_meta_name("timestamp.json") ) @@ -334,31 +301,14 @@ def _load_snapshot(self) -> None: # TODO: Check if exists locally snapshot_url = _build_full_url(self._metadata_url, "snapshot.json") - verified_snapshot = False - temp_obj = None - try: - temp_obj = download.download_file( - snapshot_url, - length, - self._fetcher, - strict_required_length=False, - ) - - temp_obj.seek(0) - verified_snapshot = self._verify_snapshot(temp_obj.read()) - - except Exception as e: - # TODO: do we reraise a NoWorkingMirrorError or just - # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({snapshot_url: e}) from e - - finally: - if temp_obj: - temp_obj.close() + data = download.download_bytes( + snapshot_url, + length, + self._fetcher, + strict_required_length=False, + ) - self._metadata["snapshot"] = verified_snapshot - # Persist root metadata. The client MUST write the file to - # non-volatile storage as FILENAME.EXT (e.g. root.json). + self._metadata["snapshot"] = self._verify_snapshot(data) self._metadata["snapshot"].persist( self._get_full_meta_name("snapshot.json") ) @@ -383,33 +333,16 @@ def _load_targets(self, targets_role: str, parent_role: str) -> None: targets_url = _build_full_url( self._metadata_url, f"{targets_role}.json" ) - verified_targets = False - temp_obj = None - try: - temp_obj = download.download_file( - targets_url, - length, - self._fetcher, - strict_required_length=False, - ) - - temp_obj.seek(0) - verified_targets = self._verify_targets( - temp_obj.read(), targets_role, parent_role - ) - - except Exception as e: - # TODO: do we reraise a NoWorkingMirrorError or just - # let exceptions propagate? - raise exceptions.NoWorkingMirrorError({targets_url: e}) from e - - finally: - if temp_obj: - temp_obj.close() + data = download.download_bytes( + targets_url, + length, + self._fetcher, + strict_required_length=False, + ) - self._metadata[targets_role] = verified_targets - # Persist root metadata. The client MUST write the file to - # non-volatile storage as FILENAME.EXT (e.g. root.json). + self._metadata[targets_role] = self._verify_targets( + data, targets_role, parent_role + ) self._metadata[targets_role].persist( self._get_full_meta_name(targets_role, extension=".json") ) From 9fae500f8716f5fba4d15796db86784243604902 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 4 May 2021 11:18:08 +0300 Subject: [PATCH 05/10] new updater: Rename _get_target_hash() The function actually hashes the target filepath. Signed-off-by: Jussi Kukkonen --- tuf/client_rework/updater_rework.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index d964525806..c74b4afee1 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -640,7 +640,7 @@ def _visit_child_role(child_role: Dict, target_filepath: str) -> str: child_role_path_hash_prefixes = child_role.get("path_hash_prefixes") if child_role_path_hash_prefixes is not None: - target_filepath_hash = _get_target_hash(target_filepath) + target_filepath_hash = _get_filepath_hash(target_filepath) for child_role_path_hash_prefix in child_role_path_hash_prefixes: if not target_filepath_hash.startswith(child_role_path_hash_prefix): continue @@ -749,7 +749,7 @@ def _check_hashes(file_content, trusted_hashes): ) -def _get_target_hash(target_filepath, hash_function="sha256"): +def _get_filepath_hash(target_filepath, hash_function="sha256"): """ TODO """ From 888f022dbf08f09fb3b2dde0d964708e79ad049b Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 4 May 2021 11:20:21 +0300 Subject: [PATCH 06/10] new updater: Remove misleading comment Signed-off-by: Jussi Kukkonen --- tuf/client_rework/updater_rework.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index c74b4afee1..2fbdf4f21d 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -700,10 +700,6 @@ def _check_file_length(file_object, trusted_file_length): observed_length = file_object.tell() file_object.seek(0) - # Return and log a message if the length 'file_object' is equal to - # 'trusted_file_length', otherwise raise an exception. A hard check - # ensures that a downloaded file strictly matches a known, or trusted, - # file length. if observed_length != trusted_file_length: raise exceptions.DownloadLengthMismatchError( trusted_file_length, observed_length From 3a02583398a59a1d5104eb42405d8614e21eee7d Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 4 May 2021 11:31:23 +0300 Subject: [PATCH 07/10] Rename url prefixes so they are consistent Signed-off-by: Jussi Kukkonen --- tuf/client_rework/updater_rework.py | 37 +++++++++++++++-------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index 2fbdf4f21d..f02f4f0ed3 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -42,20 +42,14 @@ class Updater: def __init__( self, repository_name: str, - metadata_url: str, - default_target_url: Optional[str] = None, + metadata_base_url: str, + target_base_url: Optional[str] = None, fetcher: Optional[FetcherInterface] = None, ): self._repository_name = repository_name - self._metadata_url = metadata_url - # Should we accept metadata url as a default for targets or - # targets_url should be provided either in this constructor - # or as a download_target parameter? - if default_target_url is None: - self._default_target_url = metadata_url - else: - self._default_target_url = default_target_url + self._metadata_base_url = metadata_base_url + self._target_base_url = target_base_url self._consistent_snapshot = False self._metadata = {} @@ -146,16 +140,21 @@ def download_target( self, targetinfo: Dict, destination_directory: str, - target_url: Optional[str] = None, + target_base_url: Optional[str] = None, ): """ This method performs the actual download of the specified target. The file is saved to the 'destination_directory' argument. """ - if target_url is None: - target_url = self._default_target_url + if target_base_url is None and self._target_base_url is None: + raise ValueError( + "target_base_url must be set in either download_target() or " + "constructor" + ) + elif target_base_url is None: + target_base_url = self._target_base_url - full_url = _build_full_url(target_url, targetinfo["filepath"]) + full_url = _build_full_url(target_base_url, targetinfo["filepath"]) with download.download_file( full_url, targetinfo["fileinfo"]["length"], self._fetcher @@ -209,7 +208,7 @@ def _load_root(self) -> None: for next_version in range(lower_bound, upper_bound): try: root_url = _build_full_url( - self._metadata_url, f"{next_version}.root.json" + self._metadata_base_url, f"{next_version}.root.json" ) # For each version of root iterate over the list of mirrors # until an intermediate root is successfully downloaded and @@ -272,7 +271,9 @@ def _load_timestamp(self) -> None: TODO """ # TODO Check if timestamp exists locally - timestamp_url = _build_full_url(self._metadata_url, "timestamp.json") + timestamp_url = _build_full_url( + self._metadata_base_url, "timestamp.json" + ) data = download.download_bytes( timestamp_url, settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, @@ -300,7 +301,7 @@ def _load_snapshot(self) -> None: # version = None # TODO: Check if exists locally - snapshot_url = _build_full_url(self._metadata_url, "snapshot.json") + snapshot_url = _build_full_url(self._metadata_base_url, "snapshot.json") data = download.download_bytes( snapshot_url, length, @@ -331,7 +332,7 @@ def _load_targets(self, targets_role: str, parent_role: str) -> None: # TODO: Check if exists locally targets_url = _build_full_url( - self._metadata_url, f"{targets_role}.json" + self._metadata_base_url, f"{targets_role}.json" ) data = download.download_bytes( targets_url, From ab210b410b8c2b716ce5b0505c196f2223d49b47 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Tue, 4 May 2021 11:55:08 +0300 Subject: [PATCH 08/10] new updater: Clean up url handling * Make sure all base urls (prefixes) end in a slash * Add documentation to get_one_valid_targetinfo(): That is the one place where the API accepts ill-defined "paths" from the caller * Remove checks from download url handling: we control both the base url and the relative path so there should be no surprises here. Signed-off-by: Jussi Kukkonen --- tuf/client_rework/updater_rework.py | 56 ++++++++++++++--------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index f02f4f0ed3..8de5cff708 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -46,10 +46,12 @@ def __init__( target_base_url: Optional[str] = None, fetcher: Optional[FetcherInterface] = None, ): - self._repository_name = repository_name - self._metadata_base_url = metadata_base_url - self._target_base_url = target_base_url + self._metadata_base_url = _ensure_trailing_slash(metadata_base_url) + if target_base_url is None: + self._target_base_url = None + else: + self._target_base_url = _ensure_trailing_slash(target_base_url) self._consistent_snapshot = False self._metadata = {} @@ -77,13 +79,18 @@ def refresh(self) -> None: self._load_snapshot() self._load_targets("targets", "root") - def get_one_valid_targetinfo(self, filename: str) -> Dict: + def get_one_valid_targetinfo(self, target_path: str) -> Dict: """ - Returns the target information for a specific file identified by its - file path. This target method also downloads the metadata of updated - targets. + Returns the target information for a target identified by target_path. + + As a side-effect this method downloads all the metadata it needs to + return the target information. + + Args: + target_path: A path-relative-URL string + (https://url.spec.whatwg.org/#path-relative-url-string) """ - return self._preorder_depth_first_walk(filename) + return self._preorder_depth_first_walk(target_path) @staticmethod def updated_targets(targets: Dict, destination_directory: str) -> Dict: @@ -151,10 +158,12 @@ def download_target( "target_base_url must be set in either download_target() or " "constructor" ) - elif target_base_url is None: + if target_base_url is None: target_base_url = self._target_base_url + else: + target_base_url = _ensure_trailing_slash(target_base_url) - full_url = _build_full_url(target_base_url, targetinfo["filepath"]) + full_url = parse.urljoin(target_base_url, targetinfo["filepath"]) with download.download_file( full_url, targetinfo["fileinfo"]["length"], self._fetcher @@ -207,7 +216,7 @@ def _load_root(self) -> None: for next_version in range(lower_bound, upper_bound): try: - root_url = _build_full_url( + root_url = parse.urljoin( self._metadata_base_url, f"{next_version}.root.json" ) # For each version of root iterate over the list of mirrors @@ -271,9 +280,7 @@ def _load_timestamp(self) -> None: TODO """ # TODO Check if timestamp exists locally - timestamp_url = _build_full_url( - self._metadata_base_url, "timestamp.json" - ) + timestamp_url = parse.urljoin(self._metadata_base_url, "timestamp.json") data = download.download_bytes( timestamp_url, settings.DEFAULT_TIMESTAMP_REQUIRED_LENGTH, @@ -301,7 +308,7 @@ def _load_snapshot(self) -> None: # version = None # TODO: Check if exists locally - snapshot_url = _build_full_url(self._metadata_base_url, "snapshot.json") + snapshot_url = parse.urljoin(self._metadata_base_url, "snapshot.json") data = download.download_bytes( snapshot_url, length, @@ -331,7 +338,7 @@ def _load_targets(self, targets_role: str, parent_role: str) -> None: # TODO: Check if exists locally - targets_url = _build_full_url( + targets_url = parse.urljoin( self._metadata_base_url, f"{targets_role}.json" ) data = download.download_bytes( @@ -761,17 +768,6 @@ def _get_filepath_hash(target_filepath, hash_function="sha256"): return target_filepath_hash -def _build_full_url(base_url, filepath): - """ - Build a full “absolute" URL by combining a base URL with - a relative file path. - """ - # Are these steps enough? Or too much? Is this the right place? - filepath = parse.quote(filepath) - # Assuming that base_url ends with a '/' character, otherwise parse.urljoin - # omits (correcly) the last part of the base URL path - full_url = parse.urljoin(base_url, filepath) - # Avoid windows path separators. Should we keep this check? Or correct - # paths should be required from the user? - # full_url.replace("\\", "/") - return full_url +def _ensure_trailing_slash(url: str): + """Return url guaranteed to end in a slash""" + return url if url.endswith("/") else f"{url}/" From 9605e19db202f3166abeeab5f6a13889282697d6 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Fri, 7 May 2021 12:38:27 +0300 Subject: [PATCH 09/10] new updater: Improve docstrings Signed-off-by: Jussi Kukkonen --- tuf/client_rework/updater_rework.py | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/tuf/client_rework/updater_rework.py b/tuf/client_rework/updater_rework.py index 8de5cff708..e117e4fc2c 100644 --- a/tuf/client_rework/updater_rework.py +++ b/tuf/client_rework/updater_rework.py @@ -46,6 +46,16 @@ def __init__( target_base_url: Optional[str] = None, fetcher: Optional[FetcherInterface] = None, ): + """ + Args: + repository_name: directory name (within a local directory + defined by 'tuf.settings.repositories_directory') + metadata_base_url: Base URL for all remote metadata downloads + target_base_url: Optional; Default base URL for all remote target + downloads. Can be individually set in download_target() + fetcher: Optional; FetcherInterface implementation used to download + both metadata and targets. Default is RequestsFetcher + """ self._repository_name = repository_name self._metadata_base_url = _ensure_trailing_slash(metadata_base_url) if target_base_url is None: @@ -87,8 +97,10 @@ def get_one_valid_targetinfo(self, target_path: str) -> Dict: return the target information. Args: - target_path: A path-relative-URL string - (https://url.spec.whatwg.org/#path-relative-url-string) + target_path: A target identifier that is a path-relative-URL string + (https://url.spec.whatwg.org/#path-relative-url-string). + Typically this is also the unix file path of the eventually + downloaded file. """ return self._preorder_depth_first_walk(target_path) @@ -150,8 +162,15 @@ def download_target( target_base_url: Optional[str] = None, ): """ - This method performs the actual download of the specified target. - The file is saved to the 'destination_directory' argument. + Download target specified by 'targetinfo' into 'destination_directory'. + + Args: + targetinfo: data received from get_one_valid_targetinfo() + destination_directory: existing local directory to download into. + Note that new directories may be created inside + destination_directory as required. + target_base_url: Optional; Base URL used to form the final target + download URL. Default is the value provided in Updater() """ if target_base_url is None and self._target_base_url is None: raise ValueError( From ec4c5ce00d8c77e68fe4049d9e8b50c634ea06e5 Mon Sep 17 00:00:00 2001 From: Jussi Kukkonen Date: Wed, 12 May 2021 15:42:53 +0300 Subject: [PATCH 10/10] tests: Don't use os.path.join() for URLS The test has issues like this alsready but let's not add more... Signed-off-by: Jussi Kukkonen --- tests/test_updater_rework.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_updater_rework.py b/tests/test_updater_rework.py index 162fa5b1f9..b564fbf57e 100644 --- a/tests/test_updater_rework.py +++ b/tests/test_updater_rework.py @@ -123,8 +123,8 @@ def setUp(self): # directory copied from the original repository files. tuf.settings.repositories_directory = self.client_directory - metadata_url = os.path.join(url_prefix, 'metadata/') - targets_url = os.path.join(url_prefix, 'targets/') + metadata_url = f"{url_prefix}/metadata/" + targets_url = f"{url_prefix}/targets/" # Creating a repository instance. The test cases will use this client # updater to refresh metadata, fetch target files, etc. self.repository_updater = updater.Updater(self.repository_name,