mirror of
https://github.com/theupdateframework/python-tuf
synced 2026-05-24 10:08:28 +00:00
Redundantly verify file length in updater.
You may argue that the redundancy is unnecessary (pun intended), but it is there because redundancy means one safety check will work where another fails. I introduced this redundant file length check because the updater unit test is mocking the download functions, which means that file length checks in the download functions are being bypassed. Redundancy is a good thing for safety.
This commit is contained in:
dachshund
parent
a7f14e5334
commit
9ddd2617f5
6 changed files with 153 additions and 43 deletions
|
|
@ -192,7 +192,14 @@ class DownloadError(Error):
|
|||
|
||||
class DownloadLengthMismatchError(DownloadError):
|
||||
"""Indicate that a mismatch of lengths was seen while downloading a file."""
|
||||
pass
|
||||
|
||||
def __init__(self, expected_length, observed_length):
|
||||
self.expected_length = expected_length #bytes
|
||||
self.observed_length = observed_length #bytes
|
||||
|
||||
def __str__(self):
|
||||
return 'Observed length ('+str(self.observed_length)+\
|
||||
') <= expected length ('+str(self.expected_length)+')'
|
||||
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -592,17 +592,17 @@ def refresh(self):
|
|||
|
||||
|
||||
|
||||
def __check_hashes(self, input_file, trusted_hashes):
|
||||
def __check_hashes(self, file_object, trusted_hashes):
|
||||
"""
|
||||
<Purpose>
|
||||
A helper function that verifies multiple secure hashes of the downloaded
|
||||
file. If any of these fail it raises an exception. This is to conform
|
||||
with the TUF specs, which support clients with different hashing
|
||||
algorithms. The 'hash.py' module is used to compute the hashes of the
|
||||
'input_file'.
|
||||
'file_object'.
|
||||
|
||||
<Arguments>
|
||||
input_file:
|
||||
file_object:
|
||||
A file-like object.
|
||||
|
||||
trusted_hashes:
|
||||
|
|
@ -624,7 +624,7 @@ def __check_hashes(self, input_file, trusted_hashes):
|
|||
# any of the hashes are incorrect and return if all are correct.
|
||||
for algorithm, trusted_hash in trusted_hashes.items():
|
||||
digest_object = tuf.hash.digest(algorithm)
|
||||
digest_object.update(input_file.read())
|
||||
digest_object.update(file_object.read())
|
||||
computed_hash = digest_object.hexdigest()
|
||||
if trusted_hash != computed_hash:
|
||||
raise tuf.BadHashError('Hashes do not match! Expected '+
|
||||
|
|
@ -636,6 +636,74 @@ def __check_hashes(self, input_file, trusted_hashes):
|
|||
|
||||
|
||||
|
||||
def __hard_check_length(self, file_object, trusted_length):
|
||||
"""
|
||||
<Purpose>
|
||||
A helper function that checks the expected length of a file-like object.
|
||||
The length of the file must be strictly equal to the expected length.
|
||||
This is a deliberately redundant implementation designed to complement
|
||||
tuf.download._check_downloaded_length().
|
||||
|
||||
<Arguments>
|
||||
file_object:
|
||||
A file-like object.
|
||||
|
||||
trusted_length:
|
||||
A nonnegative integer that is the expected length of the file.
|
||||
|
||||
<Exceptions>
|
||||
tuf.DownloadLengthMismatchError, if the lengths don't match.
|
||||
|
||||
<Side Effects>
|
||||
None.
|
||||
|
||||
<Returns>
|
||||
None.
|
||||
|
||||
"""
|
||||
|
||||
observed_length = len(file_object)
|
||||
if observed_length != trusted_length:
|
||||
raise tuf.DownloadLengthMismatchError(trusted_length, observed_length)
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
def __soft_check_length(self, file_object, trusted_length):
|
||||
"""
|
||||
<Purpose>
|
||||
A helper function that checks the expected length of a file-like 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-like object.
|
||||
|
||||
trusted_length:
|
||||
A nonnegative integer that is the expected length of the file.
|
||||
|
||||
<Exceptions>
|
||||
tuf.DownloadLengthMismatchError, if the lengths don't match.
|
||||
|
||||
<Side Effects>
|
||||
None.
|
||||
|
||||
<Returns>
|
||||
None.
|
||||
|
||||
"""
|
||||
|
||||
observed_length = len(file_object)
|
||||
if observed_length > trusted_length:
|
||||
raise tuf.DownloadLengthMismatchError(trusted_length, observed_length)
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
def get_target_file(self, target_filepath, file_length, file_hashes):
|
||||
"""
|
||||
<Purpose>
|
||||
|
|
@ -667,21 +735,25 @@ def get_target_file(self, target_filepath, file_length, file_hashes):
|
|||
|
||||
"""
|
||||
|
||||
def verify_target_file(target_file_object):
|
||||
# Every target file must have its hashes inspected.
|
||||
def verify_decompressed_target_file(target_file_object):
|
||||
# Every target file must have its length and hashes inspected.
|
||||
self.__hard_check_length(target_file_object, file_length)
|
||||
self.__check_hashes(target_file_object, file_hashes)
|
||||
|
||||
return self.__get_file(target_filepath, verify_target_file, 'target',
|
||||
file_length, download_safely=True, compression=None)
|
||||
return self.__get_file(target_filepath, verify_decompressed_target_file,
|
||||
'target', file_length, download_safely=True,
|
||||
compression=None)
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
def __verify_metadata_file(self, metadata_file_object, metadata_role):
|
||||
def __verify_decompressed_metadata_file(self, metadata_file_object,
|
||||
metadata_role):
|
||||
"""
|
||||
<Purpose>
|
||||
A private helpe function to verify a downloaded metadata file.
|
||||
A private helper function to verify a decompressed downloaded metadata
|
||||
file.
|
||||
|
||||
<Arguments>
|
||||
metadata_file_object:
|
||||
|
|
@ -787,11 +859,14 @@ def unsafely_get_metadata_file(self, metadata_role, metadata_filepath,
|
|||
|
||||
"""
|
||||
|
||||
def unsafely_verify_metadata_file(metadata_file_object):
|
||||
self.__verify_metadata_file(metadata_file_object, metadata_role)
|
||||
def unsafely_verify_decompressed_metadata_file(metadata_file_object):
|
||||
self.__soft_check_length(metadata_file_object, file_length)
|
||||
self.__verify_decompressed_metadata_file(metadata_file_object,
|
||||
metadata_role)
|
||||
|
||||
return self.__get_file(metadata_filepath, unsafely_verify_metadata_file,
|
||||
'meta', file_length, download_safely=False,
|
||||
return self.__get_file(metadata_filepath,
|
||||
unsafely_verify_decompressed_metadata_file, 'meta',
|
||||
file_length, download_safely=False,
|
||||
compression=None)
|
||||
|
||||
|
||||
|
|
@ -836,12 +911,15 @@ def safely_get_metadata_file(self, metadata_role, metadata_filepath,
|
|||
|
||||
"""
|
||||
|
||||
def safely_verify_metadata_file(metadata_file_object):
|
||||
def safely_verify_decompressed_metadata_file(metadata_file_object):
|
||||
self.__hard_check_length(metadata_file_object, file_length)
|
||||
self.__check_hashes(metadata_file_object, file_hashes)
|
||||
self.__verify_metadata_file(metadata_file_object, metadata_role)
|
||||
self.__verify_decompressed_metadata_file(metadata_file_object,
|
||||
metadata_role)
|
||||
|
||||
return self.__get_file(metadata_filepath, safely_verify_metadata_file,
|
||||
'meta', file_length, download_safely=True,
|
||||
return self.__get_file(metadata_filepath,
|
||||
safely_verify_decompressed_metadata_file, 'meta',
|
||||
file_length, download_safely=True,
|
||||
compression=compression)
|
||||
|
||||
|
||||
|
|
@ -851,7 +929,7 @@ def safely_verify_metadata_file(metadata_file_object):
|
|||
# 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.
|
||||
def __get_file(self, filepath, verify_file, file_type,
|
||||
def __get_file(self, filepath, verify_decompressed_file, file_type,
|
||||
file_length, download_safely, compression):
|
||||
"""
|
||||
<Purpose>
|
||||
|
|
@ -863,9 +941,9 @@ def __get_file(self, filepath, verify_file, file_type,
|
|||
filepath:
|
||||
The relative metadata or target filepath.
|
||||
|
||||
verify_file:
|
||||
A function which expects a file-like object and which will raise an
|
||||
exception in case the file is not valid for any reason.
|
||||
verify_decompressed_file:
|
||||
A function which expects a decompressed file-like object and which will
|
||||
raise an exception in case the file is not valid for any reason.
|
||||
|
||||
file_type:
|
||||
Type of data needed for download, must correspond to one of the strings
|
||||
|
|
@ -911,9 +989,12 @@ def __get_file(self, filepath, verify_file, file_type,
|
|||
file_object = tuf.download.unsafe_download(file_mirror, file_length)
|
||||
|
||||
if compression:
|
||||
logger.debug('Decompressing '+str(file_mirror))
|
||||
file_object.decompress_temp_file_object(compression)
|
||||
else:
|
||||
logger.debug('Not decompressing '+str(file_mirror))
|
||||
|
||||
verify_file(file_object)
|
||||
verify_decompressed_file(file_object)
|
||||
|
||||
except Exception, exception:
|
||||
# Remember the error from this mirror, and "reset" the target file.
|
||||
|
|
@ -2155,10 +2236,10 @@ def _visit_child_role(self, child_role, target_filepath):
|
|||
Ensure that we explore only delegated roles trusted with the target. We
|
||||
assume conservation of delegated paths in the complete tree of
|
||||
delegations. Note that the call to _ensure_all_targets_allowed in
|
||||
__verify_metadata_file should already ensure that all targets metadata is
|
||||
valid; i.e. that the targets signed by a delegatee is a proper subset of
|
||||
the targets delegated to it by the delegator. Nevertheless, we check it
|
||||
again here for performance and safety reasons.
|
||||
__verify_decompressed_metadata_file should already ensure that all
|
||||
targets metadata is valid; i.e. that the targets signed by a delegatee is
|
||||
a proper subset of the targets delegated to it by the delegator.
|
||||
Nevertheless, we check it again here for performance and safety reasons.
|
||||
|
||||
TODO: Should the TUF spec restrict the repository to one particular
|
||||
algorithm? Should we allow the repository to specify in the role
|
||||
|
|
|
|||
|
|
@ -647,7 +647,7 @@ def _check_downloaded_length(total_downloaded, required_length,
|
|||
if STRICT_REQUIRED_LENGTH:
|
||||
# This must be due to a programming error, and must never happen!
|
||||
logger.error(message)
|
||||
raise tuf.DownloadLengthMismatchError(message)
|
||||
raise tuf.DownloadLengthMismatchError(required_length, total_downloaded)
|
||||
else:
|
||||
# We specifically disabled strict checking of required length, but we
|
||||
# will log a warning anyway. This is useful when we wish to download the
|
||||
|
|
|
|||
|
|
@ -534,15 +534,15 @@ def test_3__update_metadata(self):
|
|||
added_target_2 = self._add_target_to_targets_dir(targets_keyids)
|
||||
|
||||
# To test compressed file handling, compress targets metadata file.
|
||||
targets_filepath_compressed = self._compress_file(self.targets_filepath)
|
||||
targets_filepath_compressed = self._compress_file(self.targets_filepath)
|
||||
|
||||
# Re-patch 'download.download_url_to_tempfileobj' function.
|
||||
self._mock_download_url_to_tempfileobj(targets_filepath_compressed)
|
||||
# TODO: Not convinced this is actually being tested correctly.
|
||||
# See how we get fileinfo in tuf.client.updater._update_metadata_if_changed
|
||||
# FIXME: The length (but not the hash) passed to this function is
|
||||
# incorrect. The length must be that of the compressed file, whereas the
|
||||
# hash must be that of the uncompressed file.
|
||||
_update_metadata('targets',
|
||||
#signerlib.get_metadata_file_info(self.targets_filepath),
|
||||
None,
|
||||
signerlib.get_metadata_file_info(self.targets_filepath),
|
||||
compression='gzip')
|
||||
list_of_targets = self.Repository.metadata['current']['targets']['targets']
|
||||
|
||||
|
|
@ -696,12 +696,12 @@ def test_3__update_metadata_if_changed(self):
|
|||
# Patch 'download.download_url_to_tempfileobj' and update targets.
|
||||
self._mock_download_url_to_tempfileobj(self.root_filepath)
|
||||
|
||||
# TODO: Is this the original intent of this test?
|
||||
# FIXME: What is the original intent of this test?
|
||||
try:
|
||||
update_if_changed('targets')
|
||||
except tuf.NoWorkingMirrorError, exception:
|
||||
for mirror_url, mirror_error in exception.mirror_errors.iteritems():
|
||||
assert isinstance(mirror_error, tuf.BadHashError)
|
||||
assert isinstance(mirror_error, tuf.DownloadLengthMismatchError)
|
||||
|
||||
# Restoring repositories to the initial state.
|
||||
os.remove(release_filepath_compressed)
|
||||
|
|
|
|||
|
|
@ -1,3 +1,5 @@
|
|||
#!/usr/bin/env python
|
||||
|
||||
"""
|
||||
<Program Name>
|
||||
test_util.py
|
||||
|
|
@ -37,7 +39,6 @@ class TestUtil(unittest_toolbox.Modified_TestCase):
|
|||
|
||||
def setUp(self):
|
||||
unittest_toolbox.Modified_TestCase.setUp(self)
|
||||
util.tempfile.TemporaryFile = tempfile.TemporaryFile
|
||||
self.temp_fileobj = util.TempFile()
|
||||
|
||||
|
||||
|
|
@ -71,10 +72,10 @@ def _extract_tempfile_directory(self, config_temp_dir=None):
|
|||
# Patching 'tempfile.TemporaryFile()' (by substituting
|
||||
# temfile.TemporaryFile() with tempfile.mkstemp()) in order to get the
|
||||
# directory of the stored tempfile object.
|
||||
saved_tempfile_TemporaryFile = util.tempfile.TemporaryFile
|
||||
util.tempfile.TemporaryFile = tempfile.mkstemp
|
||||
saved_tempfile_TemporaryFile = util.tempfile.NamedTemporaryFile
|
||||
util.tempfile.NamedTemporaryFile = tempfile.mkstemp
|
||||
_temp_fileobj = util.TempFile()
|
||||
util.tempfile.TemporaryFile = saved_tempfile_TemporaryFile
|
||||
util.tempfile.NamedTemporaryFile = saved_tempfile_TemporaryFile
|
||||
junk, _tempfilepath = _temp_fileobj.temporary_file
|
||||
_tempfile_dir = os.path.dirname(_tempfilepath)
|
||||
|
||||
|
|
|
|||
27
tuf/util.py
27
tuf/util.py
|
|
@ -52,7 +52,7 @@ class TempFile(object):
|
|||
def _default_temporary_directory(self, prefix):
|
||||
"""__init__ helper."""
|
||||
try:
|
||||
self.temporary_file = tempfile.TemporaryFile(prefix=prefix)
|
||||
self.temporary_file = tempfile.NamedTemporaryFile(prefix=prefix)
|
||||
except OSError, err:
|
||||
logger.critical('Temp file in '+temp_dir+'failed: '+repr(err))
|
||||
raise tuf.Error(err)
|
||||
|
|
@ -66,7 +66,7 @@ def __init__(self, prefix='tuf_temp_'):
|
|||
|
||||
<Arguments>
|
||||
prefix:
|
||||
A string argument to be used with tempfile.TemporaryFile function.
|
||||
A string argument to be used with tempfile.NamedTemporaryFile function.
|
||||
|
||||
<Exceptions>
|
||||
tuf.Error on failure to load temp dir.
|
||||
|
|
@ -82,7 +82,8 @@ def __init__(self, prefix='tuf_temp_'):
|
|||
temp_dir = tuf.conf.temporary_directory
|
||||
if temp_dir is not None and isinstance(temp_dir, str):
|
||||
try:
|
||||
self.temporary_file = tempfile.TemporaryFile(prefix=prefix, dir=temp_dir)
|
||||
self.temporary_file = tempfile.NamedTemporaryFile(prefix=prefix,
|
||||
dir=temp_dir)
|
||||
except OSError, err:
|
||||
logger.error('Temp file in '+temp_dir+' failed: '+repr(err))
|
||||
logger.error('Will attempt to use system default temp dir.')
|
||||
|
|
@ -92,6 +93,26 @@ def __init__(self, prefix='tuf_temp_'):
|
|||
|
||||
|
||||
|
||||
def __len__(self):
|
||||
"""
|
||||
<Purpose>
|
||||
Initializes TempFile.
|
||||
|
||||
<Arguments>
|
||||
None.
|
||||
|
||||
<Exceptions>
|
||||
OSError.
|
||||
|
||||
<Return>
|
||||
Nonnegative integer representing file size.
|
||||
|
||||
"""
|
||||
|
||||
return os.stat(self.temporary_file.name).st_size
|
||||
|
||||
|
||||
|
||||
def flush(self):
|
||||
"""
|
||||
<Purpose>
|
||||
|
|
|
|||
Loading…
Reference in a new issue