From bcabe0072bbe22247833bc828fbbb092a924259e Mon Sep 17 00:00:00 2001 From: Vladimir Diaz Date: Wed, 6 Sep 2017 16:54:13 -0400 Subject: [PATCH] Refactor _visit_child_role, remove obsolete test conditions, and improve coverage --- tests/test_updater.py | 28 +++++++---------- tuf/client/updater.py | 73 ++++++++++++++----------------------------- 2 files changed, 36 insertions(+), 65 deletions(-) diff --git a/tests/test_updater.py b/tests/test_updater.py index 6accdd37..384da5b3 100644 --- a/tests/test_updater.py +++ b/tests/test_updater.py @@ -820,6 +820,8 @@ def test_3__update_metadata_if_changed(self): target3 = os.path.join(self.repository_directory, 'targets', 'file3.txt') repository.targets.add_target(target3) + repository.root.version = repository.root.version + 1 + repository.root.load_signing_key(self.role_keys['root']['private']) repository.targets.load_signing_key(self.role_keys['targets']['private']) repository.snapshot.load_signing_key(self.role_keys['snapshot']['private']) repository.timestamp.load_signing_key(self.role_keys['timestamp']['private']) @@ -838,6 +840,7 @@ def test_3__update_metadata_if_changed(self): self.repository_updater._update_metadata('timestamp', DEFAULT_TIMESTAMP_FILELENGTH) self.repository_updater._update_metadata_if_changed('snapshot', 'timestamp') self.repository_updater._update_metadata_if_changed('targets') + self.repository_updater._update_metadata_if_changed('root') targets_path = os.path.join(self.client_metadata_current, 'targets.json') self.assertTrue(os.path.exists(targets_path)) self.assertTrue(self.repository_updater.metadata['current']['targets']) @@ -845,8 +848,7 @@ def test_3__update_metadata_if_changed(self): # Test for an invalid 'referenced_metadata' argument. self.assertRaises(tuf.exceptions.RepositoryError, - self.repository_updater._update_metadata_if_changed, - 'snapshot', 'bad_role') + self.repository_updater._update_metadata_if_changed, 'snapshot', 'bad_role') @@ -951,17 +953,11 @@ def test_4__refresh_targets_metadata(self): # Verify that client's metadata files were refreshed successfully. self.assertEqual(len(self.repository_updater.metadata['current']), 6) - # Test for compressed metadata roles. - self.repository_updater.metadata['current']['snapshot']['meta']['targets.json.gz'] = \ - self.repository_updater.metadata['current']['snapshot']['meta']['targets.json'] - self.repository_updater._refresh_targets_metadata(refresh_all_delegated_roles=True) - # Test for non-existing rolename. self.repository_updater._refresh_targets_metadata('bad_rolename', refresh_all_delegated_roles=False) # Test that non-json metadata in Snapshot is ignored. - print('attempting non-json file test') self.repository_updater.metadata['current']['snapshot']['meta']['bad_role.xml'] = {} self.repository_updater._refresh_targets_metadata(refresh_all_delegated_roles=True) @@ -1555,27 +1551,27 @@ def test_10__visit_child_role(self): file_object.write(repo_lib._get_written_metadata(role1_metadata)) self.assertEqual(self.repository_updater._visit_child_role(child_role, - '/target.exe', targets_role['delegations']), child_role['name']) + '/target.exe'), child_role['name']) # Test path hash prefixes. - child_role['path_hash_prefixes'] = ['8baf', '0000'] + child_role['path_hash_prefixes'] = ['8baf'] self.assertEqual(self.repository_updater._visit_child_role(child_role, - '/file3.txt', targets_role['delegations']), child_role['name']) + '/file3.txt'), child_role['name']) - # Test for forbidden target. - self.repository_updater._visit_child_role(child_role, - '/target.exe', targets_role['delegations']) + # Test for a forbidden target. + del child_role['path_hash_prefixes'] + self.repository_updater._visit_child_role(child_role, '/forbidden.tgz') # Verify that unequal path_hash_prefixes are skipped. child_role['path_hash_prefixes'] = ['bad', 'bad'] self.assertEqual(None, self.repository_updater._visit_child_role(child_role, - '/unknown.exe', targets_role['delegations'])) + '/unknown.exe')) # Test if both 'path' and 'path_hash_prefixes' are missing. del child_role['paths'] del child_role['path_hash_prefixes'] self.assertRaises(securesystemslib.exceptions.FormatError, self.repository_updater._visit_child_role, - child_role, targets_role['delegations'], child_role['name']) + child_role, child_role['name']) diff --git a/tuf/client/updater.py b/tuf/client/updater.py index cad20980..4d9c3aa4 100755 --- a/tuf/client/updater.py +++ b/tuf/client/updater.py @@ -1366,7 +1366,7 @@ def _update_metadata(self, metadata_role, upperbound_filelength, version=None): def _update_metadata_if_changed(self, metadata_role, - referenced_metadata='snapshot'): + referenced_metadata='snapshot'): """ Non-public method that updates the metadata for 'metadata_role' if it has @@ -1971,7 +1971,7 @@ def all_targets(self): def _refresh_targets_metadata(self, rolename='targets', - refresh_all_delegated_roles=False): + refresh_all_delegated_roles=False): """ Non-public method that refreshes the targets metadata of 'rolename'. If @@ -2301,8 +2301,7 @@ def _preorder_depth_first_walk(self, target_filepath): child_roles_to_visit = [] # NOTE: This may be a slow operation if there are many delegated roles. for child_role in child_roles: - child_role_name = self._visit_child_role(child_role, target_filepath, - delegations) + child_role_name = self._visit_child_role(child_role, target_filepath) if child_role['terminating'] and child_role_name is not None: logger.debug('Adding child role ' + repr(child_role_name)) logger.debug('Not backtracking to other roles.') @@ -2387,11 +2386,11 @@ def _get_target_from_targets_role(self, role_name, targets, target_filepath): - def _visit_child_role(self, child_role, target_filepath, parent_delegations): + def _visit_child_role(self, child_role, target_filepath): """ - Non-public method that determines whether the given 'child_role' has been - delegated the target with the name 'target_filepath'. + Non-public method that determines whether the given 'target_filepath' + is an allowed path of 'child_role'. Ensure that we explore only delegated roles trusted with the target. The metadata for 'child_role' should have been refreshed prior to this point, @@ -2399,7 +2398,7 @@ def _visit_child_role(self, child_role, target_filepath, parent_delegations): verified (as intended). The paths/targets that 'child_role' is allowed to specify in its metadata depends on the delegating role, and thus is left to the caller to verify. We verify here that 'target_filepath' - is an allowed path according to its parent role ('parent_delegations'). + is an allowed path according to the delegated 'child_role'. TODO: Should the TUF spec restrict the repository to one particular algorithm? Should we allow the repository to specify in the role @@ -2408,16 +2407,12 @@ def _visit_child_role(self, child_role, target_filepath, parent_delegations): child_role: The delegation targets role object of 'child_role', containing its - paths, path_hash_prefixes, keys and so on. + paths, path_hash_prefixes, keys, and so on. target_filepath: The path to the target file on the repository. This will be relative to the 'targets' (or equivalent) directory on a given mirror. - parent_delegations: - The 'delegations' entry of 'child_role's delegating role. A delegating - role specifies the paths/targets that a child role is trusted to sign. - None. @@ -2435,61 +2430,41 @@ def _visit_child_role(self, child_role, target_filepath, parent_delegations): child_role_paths = child_role.get('paths') child_role_path_hash_prefixes = child_role.get('path_hash_prefixes') - # A boolean indicator that tells us whether 'child_role' has been delegated - # the target with the name 'target_filepath'. - child_role_is_relevant = False - if child_role_path_hash_prefixes is not None: target_filepath_hash = self._get_target_hash(target_filepath) for child_role_path_hash_prefix in child_role_path_hash_prefixes: if target_filepath_hash.startswith(child_role_path_hash_prefix): - child_role_is_relevant = True + return child_role_name else: continue elif child_role_paths is not None: + # Is 'child_role_name' allowed to sign for 'target_filepath'? for child_role_path in child_role_paths: # A child role path may be an explicit path or pattern (Unix - # shell-style wildcards). The child role 'child_role_name' is added if - # 'target_filepath' is equal or matches 'child_role_path'. Explicit - # filepaths are also added. + # shell-style wildcards). The child role 'child_role_name' is returned + # if 'target_filepath' is equal to or matches 'child_role_path'. + # Explicit filepaths are also considered matches. if fnmatch.fnmatch(target_filepath, child_role_path): - child_role_is_relevant = True + logger.debug('Child role ' + repr(child_role_name) + ' is allowed to' + ' sign for ' + repr(target_filepath)) + + return child_role_name else: - logger.debug('Target path' + repr(target_filepath) + ' does not' - ' match child role path ' + repr(child_role_path)) + logger.debug('The given target path' + repr(target_filepath) + ' is' + ' not an allowed trusted path of ' + repr(child_role_path)) + + continue else: # 'role_name' should have been validated when it was downloaded. # The 'paths' or 'path_hash_prefixes' fields should not be missing, # so we raise a format error here in case they are both missing. - raise securesystemslib.exceptions.FormatError(repr(child_role_name) + ' has neither ' - '"paths" nor "path_hash_prefixes".') - - if child_role_is_relevant: - # Is the child role allowed by its parent role to specify this path - # in its metadata? - try: - securesystemslib.util.ensure_all_targets_allowed(child_role_name, - [target_filepath], parent_delegations) - - except tuf.exceptions.ForbiddenTargetError: - logger.debug('Child role ' + repr(child_role_name) + ' has target ' + \ - repr(target_filepath) + ', but is not allowed to sign for' - ' it according to its delegating role.') - return None - - else: - logger.debug('Child role ' + repr(child_role_name) + ' has target ' + \ - repr(target_filepath)) - return child_role_name - - else: - logger.debug('Child role ' + repr(child_role_name) + \ - ' does not have target ' + repr(target_filepath)) - return None + raise securesystemslib.exceptions.FormatError(repr(child_role_name) + ' ' + 'has neither a "paths" nor "path_hash_prefixes". At least' + ' one of these attributes must be present.')