From b5fcaaecdb5ab9228fa9205d10cd40fc0df039a5 Mon Sep 17 00:00:00 2001 From: vladdd Date: Tue, 6 Aug 2013 12:38:28 -0400 Subject: [PATCH] Update following Trishank's initial code review --- tuf/client/updater.py | 17 +++++++++++------ tuf/repo/signercli.py | 36 +++++++++++++++++++----------------- tuf/repo/signerlib.py | 2 ++ 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/tuf/client/updater.py b/tuf/client/updater.py index f29c6274..26fd3d87 100755 --- a/tuf/client/updater.py +++ b/tuf/client/updater.py @@ -719,8 +719,8 @@ def _update_metadata(self, metadata_role, fileinfo=None, compression=None): downloaded_version = metadata_signable['signed']['version'] if downloaded_version < current_version: message = repr(mirror_url)+' is older than the version currently '+\ - 'installed.\nDownloaded version: '+str(downloaded_version)+'\n'+\ - 'Current version: '+str(current_version) + 'installed.\nDownloaded version: '+repr(downloaded_version)+'\n'+\ + 'Current version: '+repr(current_version) raise tuf.RepositoryError(message) # Reject the metadata if any specified targets are not allowed. @@ -943,18 +943,23 @@ def _ensure_all_targets_allowed(self, metadata_role, metadata_object): if role_index is not None: role = roles[role_index] allowed_child_paths = role['paths'] - delegated_targets = metadata_object['targets'].keys() + actual_child_targets = metadata_object['targets'].keys() # Check that each delegated target is either explicitly listed or a parent # directory is found under role['paths'], otherwise raise an exception. - for delegated_target in delegated_targets: + # If the parent role explicitly lists target file paths in 'paths', + # this loop will run in O(n^2), the worst-case. The repository + # maintainer will likely delegate entire directories, and opt for + # explicit file paths if the targets in a directory are delegated to + # different roles/developers. + for child_target in actual_child_targets: for allowed_child_path in allowed_child_paths: - prefix = os.path.commonprefix([delegated_target, allowed_child_path]) + prefix = os.path.commonprefix([child_target, allowed_child_path]) if prefix == allowed_child_path: break else: message = 'Role '+repr(metadata_role)+' specifies target '+\ - repr(delegated_target)+' which is not an allowed path according '+\ + repr(child_target)+' which is not an allowed path according '+\ 'to the delegations set by '+repr(parent_role)+'.' raise tuf.RepositoryError(message) diff --git a/tuf/repo/signercli.py b/tuf/repo/signercli.py index f288d689..a4c62898 100755 --- a/tuf/repo/signercli.py +++ b/tuf/repo/signercli.py @@ -53,7 +53,6 @@ import optparse import getpass import time -import datetime import sys import logging import errno @@ -413,7 +412,8 @@ def _get_metadata_version(metadata_filename): If 'metadata_filename' exists, load it and extract the current version. This version number is incremented by one prior to returning. If 'metadata_filename' does not exist, return a version value of 1. - Return 1 if 'metadata_filename' cannot be read or validated. + Raise 'tuf.RepositoryError' if 'metadata_filename' cannot be read or + validated. """ @@ -425,11 +425,13 @@ def _get_metadata_version(metadata_filename): # Open 'metadata_filename', extract the version number, and return it # incremented by 1. A metadata's version is used to determine newer metadata # from older. The client should only accept newer metadata. - signable = tuf.repo.signerlib.read_metadata_file(metadata_filename) try: + signable = tuf.repo.signerlib.read_metadata_file(metadata_filename) tuf.formats.check_signable_object_format(signable) - except tuf.FormatError, e: - return 1 + except (tuf.FormatError, tuf.Error), e: + message = repr(metadata_filename)+' could not be opened or is invalid.'+\ + ' Backup or replace it and try again.' + raise tuf.RepositoryError(message) current_version = signable['signed']['version'] return current_version+1 @@ -441,8 +443,7 @@ def _get_metadata_version(metadata_filename): def _get_metadata_expiration(): """ Prompt the user for the expiration date of the metadata file. - If the entered date is valid, it is returned in seconds till - expiration. + If the entered date is valid, it is returned unmodified. tuf.Error, if the entered expiration date is invalid. @@ -451,19 +452,18 @@ def _get_metadata_expiration(): message = '\nCurrent time: '+tuf.formats.format_time(time.time())+'.\n'+\ 'Enter the expiration date, in UTC, of the metadata file (yyyy-mm-dd HH:MM:SS): ' + try: input_date = _prompt(message, str) - try: - input_date = input_date+' UTC' - expiration_date = tuf.formats.parse_time(input_date) - except tuf.FormatError, e: - raise tuf.Error('Invalid date entered.') - if expiration_date < time.time(): - message = 'The expiration date must occur after the current date.' - raise tuf.Error(message) - except ValueError, e: + input_date = input_date+' UTC' + expiration_date = tuf.formats.parse_time(input_date) + except (tuf.FormatError, ValueError), e: raise tuf.Error('Invalid date entered.') + if expiration_date < time.time(): + message = 'The expiration date must occur after the current date.' + raise tuf.Error(message) + return input_date @@ -816,7 +816,9 @@ def make_targets_metadata(keystore_directory): # Verify the 'keystore_directory' argument. keystore_directory = _check_directory(keystore_directory) - # Retrieve the target files. + # Retrieve the target files. The target paths entered by the user should be + # separated by white space. 'targets' is a list of the target path strings + # extracted from user input. prompt_targets = '\nInput may be a directory, directories, or any '+\ 'number of file paths.\nEnter the target files: ' targets_input = _prompt(prompt_targets, str) diff --git a/tuf/repo/signerlib.py b/tuf/repo/signerlib.py index 5a2f4bd1..01badeee 100755 --- a/tuf/repo/signerlib.py +++ b/tuf/repo/signerlib.py @@ -599,6 +599,8 @@ def read_metadata_file(filename): tuf.FormatError, if 'filename' is improperly formatted. + tuf.Error, if 'filename' cannot be opened. + The contents of 'filename' are extracted.