ngclient rollback improvements

tests/test_trusted_metadata_set.py

@@ -268,9 +268,13 @@ def test_update_timestamp_expired(self):
         def timestamp_expired_modifier(timestamp: Timestamp) -> None:
             timestamp.expires = datetime(1970, 1, 1)
 
+        # intermediate timestamp is allowed to be expired
         timestamp = self.modify_metadata("timestamp", timestamp_expired_modifier)
+        self.trusted_set.update_timestamp(timestamp)
+
+        # update snapshot to trigger final timestamp expiry check
         with self.assertRaises(exceptions.ExpiredMetadataError):
-            self.trusted_set.update_timestamp(timestamp)
+            self.trusted_set.update_snapshot(self.metadata["snapshot"])
 
     def test_update_snapshot_length_or_hash_mismatch(self):
         def modify_snapshot_length(timestamp: Timestamp) -> None:
@@ -328,13 +332,16 @@ def version_meta_modifier(snapshot: Snapshot) -> None:
 
     def test_update_snapshot_expired_new_snapshot(self):
         self._root_updated_and_update_timestamp(self.metadata["timestamp"])
-        # new_snapshot has expired
         def snapshot_expired_modifier(snapshot: Snapshot) -> None:
             snapshot.expires = datetime(1970, 1, 1)
 
+        # intermediate snapshot is allowed to be expired
         snapshot = self.modify_metadata("snapshot", snapshot_expired_modifier)
+        self.trusted_set.update_snapshot(snapshot)
+
+        # update targets to trigger final snapshot expiry check
         with self.assertRaises(exceptions.ExpiredMetadataError):
-            self.trusted_set.update_snapshot(snapshot)
+            self.trusted_set.update_targets(self.metadata["targets"])
 
     def test_update_targets_no_meta_in_snapshot(self):
         def no_meta_modifier(snapshot: Snapshot) -> None:

tuf/ngclient/_internal/trusted_metadata_set.py

@@ -178,6 +178,10 @@ def update_root(self, data: bytes):
     def update_timestamp(self, data: bytes):
         """Verifies and loads 'data' as new timestamp metadata.
 
+        Note that an expired intermediate timestamp is considered valid so it
+        can be used for rollback checks on newer, final timestamp. Expiry is
+        only checked for the final timestamp in update_snapshot().
+
         Args:
             data: unverified new timestamp metadata as bytes
 
@@ -227,15 +231,19 @@ def update_timestamp(self, data: bytes):
                     self.timestamp.signed.meta["snapshot.json"].version,
                 )
 
-        if new_timestamp.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError("New timestamp is expired")
+        # expiry not checked to allow old timestamp to be used for rollback
+        # protection of new timestamp: expiry is checked in update_snapshot()
 
         self._trusted_set["timestamp"] = new_timestamp
         logger.debug("Updated timestamp")
 
     def update_snapshot(self, data: bytes):
         """Verifies and loads 'data' as new snapshot metadata.
 
+        Note that an expired intermediate snapshot is considered valid so it
+        can be used for rollback checks on newer, final snapshot. Expiry is
+        only checked for the final snapshot in update_delegated_targets().
+
         Args:
             data: unverified new snapshot metadata as bytes
 
@@ -250,6 +258,11 @@ def update_snapshot(self, data: bytes):
             raise RuntimeError("Cannot update snapshot after targets")
         logger.debug("Updating snapshot")
 
+        # Local timestamp was allowed to be expired to allow for rollback
+        # checks on new timestamp but now timestamp must not be expired
+        if self.timestamp.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError("timestamp.json is expired")
+
         meta = self.timestamp.signed.meta["snapshot.json"]
 
         # Verify against the hashes in timestamp, if any
@@ -301,8 +314,8 @@ def update_snapshot(self, data: bytes):
                         f"{new_fileinfo.version}, got {fileinfo.version}."
                     )
 
-        if new_snapshot.signed.is_expired(self.reference_time):
-            raise exceptions.ExpiredMetadataError("New snapshot is expired")
+        # expiry not checked to allow old snapshot to be used for rollback
+        # protection of new snapshot: expiry is checked in update_targets()
 
         self._trusted_set["snapshot"] = new_snapshot
         logger.debug("Updated snapshot")
@@ -336,6 +349,11 @@ def update_delegated_targets(
         if self.snapshot is None:
             raise RuntimeError("Cannot load targets before snapshot")
 
+        # Local snapshot was allowed to be expired to allow for rollback
+        # checks on new snapshot but now snapshot must not be expired
+        if self.snapshot.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError("snapshot.json is expired")
+
         delegator: Optional[Metadata] = self.get(delegator_name)
         if delegator is None:
             raise RuntimeError("Cannot load targets before delegator")