From b3ada5bb0b3a357890594d0e9c09928d820fb3d4 Mon Sep 17 00:00:00 2001 From: Joshua Lock Date: Thu, 5 Nov 2020 10:42:16 +0000 Subject: [PATCH 1/4] updater: remove unused _soft_check_file_length This internal method isn't used by any code other than tests. Signed-off-by: Joshua Lock --- tests/test_updater.py | 19 ----------------- tuf/client/updater.py | 47 ------------------------------------------- 2 files changed, 66 deletions(-) diff --git a/tests/test_updater.py b/tests/test_updater.py index a9cf90b3..b1f1a1c2 100755 --- a/tests/test_updater.py +++ b/tests/test_updater.py @@ -1629,25 +1629,6 @@ def test_10__hard_check_file_length(self): - def test_10__soft_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'XXX') - temp_file_object.seek(0) - self.assertRaises(tuf.exceptions.DownloadLengthMismatchError, - self.repository_updater._soft_check_file_length, - temp_file_object, 1) - - # Verify that an exception is not raised if the file length <= the observed - # file length. - temp_file_object.seek(0) - self.repository_updater._soft_check_file_length(temp_file_object, 3) - temp_file_object.seek(0) - self.repository_updater._soft_check_file_length(temp_file_object, 4) - - - - def test_10__targets_of_role(self): # Test for non-existent role. self.assertRaises(tuf.exceptions.UnknownRoleError, diff --git a/tuf/client/updater.py b/tuf/client/updater.py index ccab7530..df226a3b 100755 --- a/tuf/client/updater.py +++ b/tuf/client/updater.py @@ -1263,53 +1263,6 @@ def _hard_check_file_length(self, file_object, trusted_file_length): - def _soft_check_file_length(self, file_object, trusted_file_length): - """ - - Non-public method that checks the trusted file length of a file object. - The length of the file must be less than or equal to the expected - length. This is a deliberately redundant implementation designed to - complement tuf.download._check_downloaded_length(). - - - file_object: - A file object. - - trusted_file_length: - A non-negative integer that is the trusted length of the file. - - - tuf.exceptions.DownloadLengthMismatchError, if the lengths do - not match. - - - Reads the contents of 'file_object' and logs a message if 'file_object' - is less than or equal to the trusted length. - Position within file_object is changed. - - - None. - """ - - # Read the entire contents of 'file_object', a - file_object.seek(0) - observed_length = len(file_object.read()) - - # Return and log a message if 'file_object' is less than or equal to - # 'trusted_file_length', otherwise raise an exception. A soft check - # ensures that an upper bound restricts how large a file is downloaded. - if observed_length > trusted_file_length: - raise tuf.exceptions.DownloadLengthMismatchError(trusted_file_length, - observed_length) - - else: - logger.debug('Observed length (' + str(observed_length) +\ - ') <= trusted length (' + str(trusted_file_length) + ')') - - - - - def _get_target_file(self, target_filepath, file_length, file_hashes, prefix_filename_with_hash): """ From ad1335b6edca7abc0cb84dd37a2fc56d8da06b7f Mon Sep 17 00:00:00 2001 From: Joshua Lock Date: Thu, 5 Nov 2020 10:45:06 +0000 Subject: [PATCH 2/4] 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): """ From 02416e33763a805bee869b9e5a72d4e7e4bdae7a Mon Sep 17 00:00:00 2001 From: Joshua Lock Date: Wed, 11 Nov 2020 15:53:07 +0000 Subject: [PATCH 3/4] updater: more optimal file length checking Rather than read to the end of the file in order to determin its size, use the whence value of seek() to move the file object's position to the end of the file, then the tell() method of the file object to read the current position in bytes. Co-authored-by: Jussi Kukkonen Signed-off-by: Joshua Lock --- tuf/client/updater.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tuf/client/updater.py b/tuf/client/updater.py index 7d1f06b5..ef34c906 100755 --- a/tuf/client/updater.py +++ b/tuf/client/updater.py @@ -1244,8 +1244,10 @@ def _check_file_length(self, file_object, trusted_file_length): None. """ - file_object.seek(0) - observed_length = len(file_object.read()) + # seek to the end of the file; that is offset 0 from the end of the file, + # represented by a whence value of 2 + file_object.seek(0, 2) + observed_length = file_object.tell() # Return and log a message if the length 'file_object' is equal to # 'trusted_file_length', otherwise raise an exception. A hard check From 372e2184e0a679b6cb343849cc1242a478a1d268 Mon Sep 17 00:00:00 2001 From: Joshua Lock Date: Thu, 26 Nov 2020 09:40:11 +0000 Subject: [PATCH 4/4] client: simplify loop exit logic Simplify the loop exit logic in _get_target_file() to simply return a verified file_object, once we have it, rather than breaking from the loop and then returning the file_object. This converts a use of a try/except/else to a try/except and is a little easier to read. Signed-off-by: Joshua Lock --- tuf/client/updater.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tuf/client/updater.py b/tuf/client/updater.py index ef34c906..df11074a 100755 --- a/tuf/client/updater.py +++ b/tuf/client/updater.py @@ -1327,6 +1327,8 @@ def _get_target_file(self, target_filepath, file_length, file_hashes, # Verify 'file_object' against the expected length and hashes. self._check_file_length(file_object, file_length) self._check_hashes(file_object, file_hashes) + # If the file verifies, we don't need to try more mirrors + return file_object except Exception as exception: # Remember the error from this mirror, and "reset" the target file. @@ -1334,16 +1336,9 @@ def _get_target_file(self, target_filepath, file_length, file_hashes, 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) + logger.debug('Failed to update ' + repr(target_filepath) + ' from' + ' all mirrors: ' + repr(file_mirror_errors)) + raise tuf.exceptions.NoWorkingMirrorError(file_mirror_errors)