Review test_indefinite_freeze_attack.py and updater.py. Minor edits made

Author: vladimir-v-diaz

tests/test_indefinite_freeze_attack.py

@@ -191,15 +191,17 @@ def test_without_tuf(self):
 
     # Test 2 Begin:
     #
-    # 'timestamp.json' specifies the latest version of the repository files.
-    # A client should only accept the same version of this file up to a certain
-    # point, or else it cannot detect that new files are available for download.
-    # Modify the repository's timestamp.json' so that it expires soon, copy it
-    # over the to client, and attempt to re-fetch the same expired version. 
+    # 'timestamp.json' specifies the latest version of the repository files.  A
+    # client should only accept the same version of this file up to a certain
+    # point, or else it cannot detect that new files are available for
+    # download.  Modify the repository's timestamp.json' so that it expires
+    # soon, copy it over to the client, and attempt to re-fetch the same
+    # expired version. 
     #
     # A non-TUF client (without a way to detect when metadata has expired) is
     # expected to download the same version, and thus the same outdated files.
-    # Verify that the same file size and hash of 'timestamp.json' is downloaded.
+    # Verify that the downloaded 'timestamp.json' contains the same file size
+    # and hash as the one available locally.
 
     timestamp_path = os.path.join(self.repository_directory, 'metadata',
                                   'timestamp.json')
@@ -285,17 +287,18 @@ def test_with_tuf(self):
     repository.snapshot.load_signing_key(snapshot_private)
 
     # Expire snapshot in 8s. This should be far enough into the future that we
-    # haven't reached it before the first refresh validates timestamp expiry. We
-    # want a successful refresh before expiry, then a second refresh after
+    # haven't reached it before the first refresh validates timestamp expiry.
+    # We want a successful refresh before expiry, then a second refresh after
     # expiry (which we then expect to raise an exception due to expired
     # metadata).
     expiry_time = time.time() + 8
     datetime_object = tuf.formats.unix_timestamp_to_datetime(int(expiry_time))
 
     repository.snapshot.expiration = datetime_object
 
-    # Now write to the repository
+    # Now write to the repository.
     repository.write()
+
     # And move the staged metadata to the "live" metadata.
     shutil.rmtree(os.path.join(self.repository_directory, 'metadata'))
     shutil.copytree(os.path.join(self.repository_directory, 'metadata.staged'),
@@ -304,27 +307,28 @@ def test_with_tuf(self):
     # Refresh metadata on the client. For this refresh, all data is not expired.
     logger.info('Test: Refreshing #1 - Initial metadata refresh occurring.')
     self.repository_updater.refresh()
-    logger.info("Test: Refreshed #1 - Initial metadata refresh completed "
-                "successfully. Now sleeping until snapshot metadata expires.")
+    logger.info('Test: Refreshed #1 - Initial metadata refresh completed '
+                'successfully. Now sleeping until snapshot metadata expires.')
 
     # Sleep until expiry_time ('repository.snapshot.expiration')
-    time.sleep( max(0, expiry_time - time.time()) )
+    time.sleep(max(0, expiry_time - time.time()))
 
-    logger.info("Test: Refreshing #2 - Now trying to refresh again after local "
-      "snapshot expiry.")
+    logger.info('Test: Refreshing #2 - Now trying to refresh again after local'
+      ' snapshot expiry.')
     try:
       self.repository_updater.refresh() # We expect this to fail!
 
-    except tuf.ExpiredMetadataError as e:
-      logger.info("Test: Refresh #2 - failed as expected. Expired local "
-                  "snapshot case generated a tuf.ExpiredMetadataError exception"
-                  " as expected. Test pass.")
+    except tuf.ExpiredMetadataError:
+      logger.info('Test: Refresh #2 - failed as expected. Expired local'
+                  ' snapshot case generated a tuf.ExpiredMetadataError'
+                  ' exception as expected. Test pass.')
+    
     # I think that I only expect tuf.ExpiredMetadata error here. A
     # NoWorkingMirrorError indicates something else in this case - unavailable
     # repo, for example.
     else:
-      self.fail("TUF failed to detect expired stale snapshot metadata. Freeze "
-        "attack successful.")
+      self.fail('TUF failed to detect expired stale snapshot metadata. Freeze'
+        ' attack successful.')
 
 
 
@@ -334,13 +338,14 @@ def test_with_tuf(self):
     # 'timestamp.json' specifies the latest version of the repository files.
     # A client should only accept the same version of this file up to a certain
     # point, or else it cannot detect that new files are available for download.
-    # Modify the repository's timestamp.json' so that it is about to expire,
+    # Modify the repository's 'timestamp.json' so that it is about to expire,
     # copy it over the to client, wait a moment until it expires, and attempt to
     # re-fetch the same expired version.
 
-    # The same scenario as in test_without_tuf() is followed here, except with a
-    # TUF client. The TUF client performs a refresh of top-level metadata, which
-    # includes 'timestamp.json'.
+    # The same scenario as in test_without_tuf() is followed here, except with
+    # a TUF client. The TUF client performs a refresh of top-level metadata,
+    # which includes 'timestamp.json', and should detect a freeze attack if
+    # the repository serves an outdated 'timestamp.json'.
 
     # Modify the timestamp file on the remote repository.  'timestamp.json'
     # must be properly updated and signed with 'repository_tool.py', otherwise
@@ -368,11 +373,11 @@ def test_with_tuf(self):
     shutil.copytree(os.path.join(self.repository_directory, 'metadata.staged'),
                     os.path.join(self.repository_directory, 'metadata'))
 
-    # Wait just long enough for the timestamp metadata (which is now both on the
-    # repository and on the client) to expire.
-    time.sleep( max(0, expiry_time - time.time()) )
+    # Wait just long enough for the timestamp metadata (which is now both on
+    # the repository and on the client) to expire.
+    time.sleep(max(0, expiry_time - time.time()))
 
-    # Try to refresh metadata on the client. Since we're already past
+    # Try to refresh top-level metadata on the client. Since we're already past
     # 'repository.timestamp.expiration', the TUF client is expected to detect
     # that timestamp metadata is outdated and refuse to continue the update
     # process.
@@ -389,8 +394,8 @@ def test_with_tuf(self):
         self.assertTrue(isinstance(mirror_error, tuf.ExpiredMetadataError))
 
     else:
-      self.fail("TUF failed to detect expired stale timestamp metadata. Freeze "
-        "attack successful.")
+      self.fail('TUF failed to detect expired, stale timestamp metadata.'
+        ' Freeze attack successful.')
 
 
 if __name__ == '__main__':

tuf/client/updater.py

@@ -565,20 +565,22 @@ def refresh(self, unsafely_update_root_if_necessary=True):
     <Purpose>
       Update the latest copies of the metadata for the top-level roles. The
       update request process follows a specific order to ensure the metadata
-      files are securely updated: timestamp -> snapshot -> root -> targets.
+      files are securely updated:
+      timestamp -> snapshot -> root (if necessary) -> targets.
       
       Delegated metadata is not refreshed by this method. After this method is
-      called, the use of target methods (e.g., all_targets(), targets_of_role(),
-      or target()) will update delegated metadata.
-      Calling refresh() ensures that top-level metadata is up-to-date, so that
-      the target methods can refer to the latest available content. Thus,
-      refresh() should always be called by the client before any requests of
-      target file information.
+      called, the use of target methods (e.g., all_targets(),
+      targets_of_role(), or target()) will update delegated metadata, when
+      required.  Calling refresh() ensures that top-level metadata is
+      up-to-date, so that the target methods can refer to the latest available
+      content. Thus, refresh() should always be called by the client before any
+      requests of target file information.
 
-      The expiration time for downloaded metadata is also verified.
+      The expiration time for downloaded metadata is also verified, including
+      local metadata that the repository claims is up to date.
 
       If the refresh fails for any reason, then unless
-      unsafely_update_root_if_necessary is set, refresh will be retried once
+      'unsafely_update_root_if_necessary' is set, refresh will be retried once
       after first attempting to update the root metadata file. Only after this
       check will the exceptions listed here potentially be raised.
 
@@ -656,7 +658,7 @@ def refresh(self, unsafely_update_root_if_necessary=True):
 
     # If an exception is raised during the metadata update attempts, we will
     # attempt to update root metadata once by recursing with a special argument
-    # to avoid further recursion.
+    # (unsafely_update_root_if_necessary) to avoid further recursion.
 
     # Use default but sane information for timestamp metadata, and do not
     # require strict checks on its required length.
@@ -675,35 +677,33 @@ def refresh(self, unsafely_update_root_if_necessary=True):
     #      If a change to a metadata file IS detected in an
     #      _update_metadata_if_changed call, but we are unable to download a
     #      valid (not expired, properly signed, valid) version of that metadata
-    #      file, and unexpired version of that metadata file, a
-    #      tuf.NoWorkingMirrorError rises to this point.
+    #      file, a tuf.NoWorkingMirrorError rises to this point.
     # 
     #   - tuf.ExpiredMetadataError:
     #
-    #      If, on the other hand, a change to a metadata file IS NOT detected in
-    #      a given _update_metadata_if_changed call, but we observe that the
+    #      If, on the other hand, a change to a metadata file IS NOT detected
+    #      in a given _update_metadata_if_changed call, but we observe that the
     #      version of the metadata file we have on hand is now expired, a
     #      tuf.ExpiredMetadataError exception rises to this point.
     #
-    except tuf.NoWorkingMirrorError as e:
+    except tuf.NoWorkingMirrorError:
       if unsafely_update_root_if_necessary:
-        logger.info('Valid top-level metadata cannot be downloaded.  Unsafely '
-          'update the Root metadata.')
+        logger.info('Valid top-level metadata cannot be downloaded.  Unsafely'
+          ' update the Root metadata.')
         self._update_metadata('root', DEFAULT_ROOT_UPPERLENGTH)
         self.refresh(unsafely_update_root_if_necessary=False)
 
       else:
         raise
 
-    except tuf.ExpiredMetadataError as e:
+    except tuf.ExpiredMetadataError:
       if unsafely_update_root_if_necessary:
         logger.info('No changes were detected from the mirrors for a given role'
-          ', and that metadata that is available on disk has been found to be '
-          'expired. Trying to update root in case of foul play.')
+          ', and that metadata that is available on disk has been found to be'
+          ' expired. Trying to update root in case of foul play.')
         self._update_metadata('root', DEFAULT_ROOT_UPPERLENGTH)
         self.refresh(unsafely_update_root_if_necessary=False)
 
-      
       # The caller explicitly requested not to unsafely fetch an expired Root.
       else:
         logger.info('No changes were detected from the mirrors for a given role'
@@ -713,6 +713,8 @@ def refresh(self, unsafely_update_root_if_necessary=True):
 
 
 
+
+
   def _check_hashes(self, file_object, trusted_hashes):
     """
     <Purpose>
@@ -1624,7 +1626,7 @@ def _update_metadata_if_changed(self, metadata_role,
       logger.info(repr(uncompressed_metadata_filename) + ' up-to-date.')
 
       # Since we have not downloaded a new version of this metadata, we
-      # should check to see if our version is stale and notify the user
+      # should check to see if our local version is stale and notify the user
       # if so. This raises tuf.ExpiredMetadataError if the metadata we
       # have is expired. Resolves issue #322.
       self._ensure_not_expired(self.metadata['current'][metadata_role],