Skip to content

Simplify updater logic for downloading and verifying target files #1202

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

Merged
merged 4 commits into from
Nov 26, 2020
Merged

Simplify updater logic for downloading and verifying target files #1202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b3ada5b
updater: remove unused _soft_check_file_length
joshuagl Nov 5, 2020
ad1335b
updater: simplify Updater.download_target() call stack
joshuagl Nov 5, 2020
02416e3
updater: more optimal file length checking
joshuagl Nov 11, 2020
372e218
client: simplify loop exit logic
joshuagl Nov 26, 2020
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
43 changes: 2 additions & 41 deletions tests/test_updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1616,38 +1616,19 @@ 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)





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,
Expand Down Expand Up @@ -1763,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.
Expand Down
182 changes: 31 additions & 151 deletions tuf/client/updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
<Purpose>
Non-public method that ensures the length of 'file_object' is strictly
Expand All @@ -1244,8 +1244,10 @@ def _hard_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
Expand All @@ -1263,53 +1265,6 @@ def _hard_check_file_length(self, file_object, trusted_file_length):



def _soft_check_file_length(self, file_object, trusted_file_length):
"""
<Purpose>
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().

<Arguments>
file_object:
A file object.

trusted_file_length:
A non-negative integer that is the trusted length of the file.

<Exceptions>
tuf.exceptions.DownloadLengthMismatchError, if the lengths do
not match.

<Side Effects>
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.

<Returns>
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):
"""
Expand Down Expand Up @@ -1351,24 +1306,39 @@ 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.
target_digest = list(file_hashes.values()).pop()
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it is possible to explain why there is a duplicated check of file length here after the one in safe_download?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strictly speaking the checks that are done in functions that safe_download() calls are for the downloaded byte count, whereas _check_file_length() actually looks at the file object. I don't know how that could be of practical difference but...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, but I think better safe than sorry. I think redundant checks are fine as long as no real impact on performance.

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.
logger.debug('Update failed from ' + file_mirror + '.')
file_mirror_errors[file_mirror] = exception
file_object = None

logger.debug('Failed to update ' + repr(target_filepath) + ' from'
' all mirrors: ' + repr(file_mirror_errors))
raise tuf.exceptions.NoWorkingMirrorError(file_mirror_errors)



Expand Down Expand Up @@ -1652,96 +1622,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):
"""
<Purpose>
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.

<Arguments>
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.

<Exceptions>
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.

<Side Effects>
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.

<Returns>
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):
"""
<Purpose>
Expand Down