ngclient: Remove root_update_finished()

The usefulness was debatable to begin with, and now that it has become
clear that rollback protection requires a second "final verification"
step for all three root, timestamp and snapshot it is clear that
root_update_finished() is not good design.

update_root() still accepts expired root metadata but now the final
root expiry is checked when the "next" metadata (timestamp) is loaded.

Signed-off-by: Jussi Kukkonen <[email protected]>

Author: Jussi Kukkonen

tests/test_trusted_metadata_set.py

@@ -92,7 +92,6 @@ def _root_updated_and_update_timestamp(
 
         """
         timestamp_bytes = timestamp_bytes or self.metadata["timestamp"]
-        self.trusted_set.root_update_finished()
         self.trusted_set.update_timestamp(timestamp_bytes)
 
 
@@ -118,7 +117,6 @@ def _update_all_besides_targets(
 
 
     def test_update(self):
-        self.trusted_set.root_update_finished()
         self.trusted_set.update_timestamp(self.metadata["timestamp"])
         self.trusted_set.update_snapshot(self.metadata["snapshot"])
         self.trusted_set.update_targets(self.metadata["targets"])
@@ -139,24 +137,16 @@ def test_update(self):
         self.assertTrue(count, 6)
 
     def test_out_of_order_ops(self):
-        # Update timestamp before root is finished
-        with self.assertRaises(RuntimeError):
-            self.trusted_set.update_timestamp(self.metadata["timestamp"])
-
-        self.trusted_set.root_update_finished()
-        with self.assertRaises(RuntimeError):
-            self.trusted_set.root_update_finished()
-
-        # Update root after a previous successful root update
-        with self.assertRaises(RuntimeError):
-            self.trusted_set.update_root(self.metadata["root"])
-
         # Update snapshot before timestamp
         with self.assertRaises(RuntimeError):
             self.trusted_set.update_snapshot(self.metadata["snapshot"])
 
         self.trusted_set.update_timestamp(self.metadata["timestamp"])
 
+        # Update root after timestamp
+        with self.assertRaises(RuntimeError):
+            self.trusted_set.update_root(self.metadata["root"])
+
         # Update targets before snapshot
         with self.assertRaises(RuntimeError):
             self.trusted_set.update_targets(self.metadata["targets"])
@@ -198,8 +188,6 @@ def test_update_with_invalid_json(self):
         with self.assertRaises(exceptions.RepositoryError):
             self.trusted_set.update_root(self.metadata["snapshot"])
 
-        self.trusted_set.root_update_finished()
-
         top_level_md = [
             (self.metadata["timestamp"], self.trusted_set.update_timestamp),
             (self.metadata["snapshot"], self.trusted_set.update_snapshot),
@@ -241,15 +229,16 @@ def test_update_root_new_root_ver_same_as_trusted_root_ver(self):
         with self.assertRaises(exceptions.ReplayedMetadataError):
             self.trusted_set.update_root(self.metadata["root"])
 
-    def test_root_update_finished_expired(self):
+    def test_root_expired_final_root(self):
         def root_expired_modifier(root: Root) -> None:
             root.expires = datetime(1970, 1, 1)
 
+        # intermediate root can be expired
         root = self.modify_metadata("root", root_expired_modifier)
         tmp_trusted_set = TrustedMetadataSet(root)
-        # call root_update_finished when trusted root has expired
+        # update timestamp to trigger final root expiry check
         with self.assertRaises(exceptions.ExpiredMetadataError):
-            tmp_trusted_set.root_update_finished()
+            tmp_trusted_set.update_timestamp(self.metadata["timestamp"])
 
 
     def test_update_timestamp_new_timestamp_ver_below_trusted_ver(self):
@@ -275,7 +264,6 @@ def bump_snapshot_version(timestamp: Timestamp) -> None:
             self.trusted_set.update_timestamp(self.metadata["timestamp"])
 
     def test_update_timestamp_expired(self):
-        self.trusted_set.root_update_finished()
         # new_timestamp has expired
         def timestamp_expired_modifier(timestamp: Timestamp) -> None:
             timestamp.expires = datetime(1970, 1, 1)

tuf/ngclient/_internal/trusted_metadata_set.py

@@ -30,8 +30,6 @@
 >>> # update root from remote until no more are available
 >>> with download("root", trusted_set.root.signed.version + 1) as f:
 >>>     trusted_set.update_root(f.read())
->>> # ...
->>> trusted_set.root_update_finished()
 >>>
 >>> # load local timestamp, then update from remote
 >>> try:
@@ -58,9 +56,6 @@
    a generic RepositoryError that covers every issue that server provided
    metadata could inflict (other errors would be user errors), but this is not
    yet the case
- * usefulness of root_update_finished() can be debated: it could be done
-   in the beginning of load_timestamp()...
- * some metadata interactions might work better in Metadata itself
  * Progress through Specification update process should be documented
    (not sure yet how: maybe a spec_logger that logs specification events?)
 """
@@ -99,7 +94,6 @@ def __init__(self, root_data: bytes):
         """
         self._trusted_set = {}  # type: Dict[str: Metadata]
         self.reference_time = datetime.utcnow()
-        self._root_update_finished = False
 
         # Load and validate the local root metadata. Valid initial trusted root
         # metadata is required
@@ -144,7 +138,7 @@ def update_root(self, data: bytes):
         """Verifies and loads 'data' as new root metadata.
 
         Note that an expired intermediate root is considered valid: expiry is
-        only checked for the final root in root_update_finished().
+        only checked for the final root in update_timestamp().
 
         Args:
             data: unverified new root metadata as bytes
@@ -153,10 +147,8 @@ def update_root(self, data: bytes):
             RepositoryError: Metadata failed to load or verify. The actual
                 error type and content will contain more details.
         """
-        if self._root_update_finished:
-            raise RuntimeError(
-                "Cannot update root after root update is finished"
-            )
+        if self.timestamp is not None:
+            raise RuntimeError("Cannot update root after timestamp")
         logger.debug("Updating root")
 
         try:
@@ -183,26 +175,6 @@ def update_root(self, data: bytes):
         self._trusted_set["root"] = new_root
         logger.debug("Updated root")
 
-    def root_update_finished(self):
-        """Marks root metadata as final and verifies it is not expired
-
-        Raises:
-            ExpiredMetadataError: The final root metadata is expired.
-        """
-        if self._root_update_finished:
-            raise RuntimeError("Root update is already finished")
-
-        if self.root.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError("New root.json is expired")
-
-        # No need to delete timestamp/snapshot here as specification instructs
-        # for fast-forward attack recovery: timestamp/snapshot can not be
-        # loaded at this point and when loaded later they will be verified
-        # with current root keys.
-
-        self._root_update_finished = True
-        logger.debug("Verified final root.json")
-
     def update_timestamp(self, data: bytes):
         """Verifies and loads 'data' as new timestamp metadata.
 
@@ -213,11 +185,15 @@ def update_timestamp(self, data: bytes):
             RepositoryError: Metadata failed to load or verify. The actual
                 error type and content will contain more details.
         """
-        if not self._root_update_finished:
-            raise RuntimeError("Cannot update timestamp before root")
         if self.snapshot is not None:
             raise RuntimeError("Cannot update timestamp after snapshot")
 
+        # client workflow 5.3.10: Make sure final root is not expired.
+        if self.root.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError("Final root.json is expired")
+        # No need to check for 5.3.11 (fast forward attack recovery):
+        # timestamp/snapshot can not yet be loaded at this point
+
         try:
             new_timestamp = Metadata.from_bytes(data)
         except DeserializationError as e:

tuf/ngclient/updater.py

@@ -320,9 +320,6 @@ def _load_root(self) -> None:
                 # 404/403 means current root is newest available
                 break
 
-        # Verify final root
-        self._trusted_set.root_update_finished()
-
     def _load_timestamp(self) -> None:
         """Load local and remote timestamp metadata"""
         try: