From ad1335b6edca7abc0cb84dd37a2fc56d8da06b7f Mon Sep 17 00:00:00 2001 From: Joshua Lock Date: Thu, 5 Nov 2020 10:45:06 +0000 Subject: [PATCH] updater: simplify Updater.download_target() call stack The call stack and code for download_target() is more complex than required: * download_target() : builds target destination filepath, gets length and hashes * _get_target_file() : fixes filenames if consistent snapshots enabled, defines verification callback * _get_file() : iterates mirrors, tries to download files, verifies them Remove the verification callback and collapse the call stack by a single level to make the code easier to follow. Signed-off-by: Joshua Lock --- tests/test_updater.py | 24 +------- tuf/client/updater.py | 134 ++++++++++-------------------------------- 2 files changed, 34 insertions(+), 124 deletions(-) diff --git a/tests/test_updater.py b/tests/test_updater.py index b1f1a1c2..e8710a06 100755 --- a/tests/test_updater.py +++ b/tests/test_updater.py @@ -1616,13 +1616,13 @@ def test_9__get_target_hash(self): - def test_10__hard_check_file_length(self): + def test_10__check_file_length(self): # Test for exception if file object is not equal to trusted file length. with tempfile.TemporaryFile() as temp_file_object: temp_file_object.write(b'X') temp_file_object.seek(0) self.assertRaises(tuf.exceptions.DownloadLengthMismatchError, - self.repository_updater._hard_check_file_length, + self.repository_updater._check_file_length, temp_file_object, 10) @@ -1744,26 +1744,6 @@ def test_11__verify_metadata_file(self): metadata_file_object, 'root') - def test_12__get_file(self): - # Test for an "unsafe" download, where the file is downloaded up to - # a required length (and no more). The "safe" download approach - # downloads an exact required length. - targets_path = os.path.join(self.repository_directory, 'metadata', 'targets.json') - - file_size, file_hashes = securesystemslib.util.get_file_details(targets_path) - file_type = 'meta' - - def verify_target_file(targets_path): - # Every target file must have its length and hashes inspected. - self.repository_updater._hard_check_file_length(targets_path, file_size) - self.repository_updater._check_hashes(targets_path, file_hashes) - - self.repository_updater._get_file('targets.json', verify_target_file, - file_type, file_size, download_safely=True).close() - - self.repository_updater._get_file('targets.json', verify_target_file, - file_type, file_size, download_safely=False).close() - def test_13__targets_of_role(self): # Test case where a list of targets is given. By default, the 'targets' # parameter is None. diff --git a/tuf/client/updater.py b/tuf/client/updater.py index df226a3b..7d1f06b5 100755 --- a/tuf/client/updater.py +++ b/tuf/client/updater.py @@ -1217,7 +1217,7 @@ def _check_hashes(self, file_object, trusted_hashes): - def _hard_check_file_length(self, file_object, trusted_file_length): + def _check_file_length(self, file_object, trusted_file_length): """ Non-public method that ensures the length of 'file_object' is strictly @@ -1304,15 +1304,6 @@ def _get_target_file(self, target_filepath, file_length, file_hashes, A file object containing the target. """ - # Define a callable function that is passed as an argument to _get_file() - # and called. The 'verify_target_file' function ensures the file length - # and hashes of 'target_filepath' are strictly equal to the trusted values. - def verify_target_file(target_file_object): - - # Every target file must have its length and hashes inspected. - self._hard_check_file_length(target_file_object, file_length) - self._check_hashes(target_file_object, file_hashes) - if self.consistent_snapshot and prefix_filename_with_hash: # Note: values() does not return a list in Python 3. Use list() # on values() for Python 2+3 compatibility. @@ -1320,8 +1311,37 @@ def verify_target_file(target_file_object): dirname, basename = os.path.split(target_filepath) target_filepath = os.path.join(dirname, target_digest + '.' + basename) - return self._get_file(target_filepath, verify_target_file, - 'target', file_length, download_safely=True) + file_mirrors = tuf.mirrors.get_list_of_mirrors('target', target_filepath, + self.mirrors) + + # file_mirror (URL): error (Exception) + file_mirror_errors = {} + file_object = None + + for file_mirror in file_mirrors: + try: + file_object = tuf.download.safe_download(file_mirror, file_length) + + # Verify 'file_object' against the expected length and hashes. + self._check_file_length(file_object, file_length) + self._check_hashes(file_object, file_hashes) + + except Exception as exception: + # Remember the error from this mirror, and "reset" the target file. + logger.debug('Update failed from ' + file_mirror + '.') + file_mirror_errors[file_mirror] = exception + file_object = None + + else: + break + + if file_object: + return file_object + + else: + logger.debug('Failed to update ' + repr(target_filepath) + ' from' + ' all mirrors: ' + repr(file_mirror_errors)) + raise tuf.exceptions.NoWorkingMirrorError(file_mirror_errors) @@ -1605,96 +1625,6 @@ def _get_metadata_file(self, metadata_role, remote_filename, - def _get_file(self, filepath, verify_file_function, file_type, file_length, - download_safely=True): - """ - - Non-public method that tries downloading, up to a certain length, a - metadata or target file from a list of known mirrors. As soon as the first - valid copy of the file is found, the rest of the mirrors will be skipped. - - - filepath: - The relative metadata or target filepath. - - verify_file_function: - A callable function that expects a file object and raises an exception - if the file is invalid. - Target files and uncompressed versions of metadata may be verified with - 'verify_file_function'. - - 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 the - 'securesystemslib.formats.NAME_SCHEMA' format. - - file_length: - The expected length, or upper bound, of the target or metadata file to - be downloaded. - - download_safely: - A boolean switch to toggle safe or unsafe download of the file. - - - tuf.exceptions.NoWorkingMirrorError: - The metadata could not be fetched. This is raised only when all known - mirrors failed to provide a valid copy of the desired metadata file. - - - The file is downloaded from all known repository mirrors in the worst - case. If a valid copy of the file is found, it is stored in a temporary - file and returned. - - - A file object containing the metadata or target. - """ - - file_mirrors = tuf.mirrors.get_list_of_mirrors(file_type, filepath, - self.mirrors) - - # file_mirror (URL): error (Exception) - file_mirror_errors = {} - file_object = None - - for file_mirror in file_mirrors: - try: - # TODO: Instead of the more fragile 'download_safely' switch, unroll - # the function into two separate ones: one for "safe" download, and the - # other one for "unsafe" download? This should induce safer and more - # readable code. - if download_safely: - file_object = tuf.download.safe_download(file_mirror, file_length) - - else: - file_object = tuf.download.unsafe_download(file_mirror, file_length) - - # Verify 'file_object' according to the callable function. - # 'file_object' is also verified if decompressed above (i.e., the - # uncompressed version). - verify_file_function(file_object) - - except Exception as exception: - # Remember the error from this mirror, and "reset" the target file. - logger.debug('Update failed from ' + file_mirror + '.') - file_mirror_errors[file_mirror] = exception - file_object = None - - else: - break - - if file_object: - return file_object - - else: - logger.debug('Failed to update ' + repr(filepath) + ' from' - ' all mirrors: ' + repr(file_mirror_errors)) - raise tuf.exceptions.NoWorkingMirrorError(file_mirror_errors) - - - - - def _update_metadata(self, metadata_role, upperbound_filelength, version=None): """