ngclient: allow limited use of wrong snapshot version

Spec does not explicitly say so but the intent is that a snapshot
metadata can be trusted for rollback protection checks of newer
snapshots even if current snapshot version does not match the version
in current timestamp meta.

Only do the snapshot version check for the "final" snapshot by doing it
when targets is updated.

Improve test names and comments.

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>

Author: Jussi Kukkonen

tests/test_trusted_metadata_set.py

@@ -300,33 +300,34 @@ def timestamp_version_modifier(timestamp: Timestamp) -> None:
 
         timestamp = self.modify_metadata("timestamp", timestamp_version_modifier)
         self._root_updated_and_update_timestamp(timestamp)
-        # new_snapshot.version != trusted timestamp.meta["snapshot"].version
-        def snapshot_version_modifier(snapshot: Snapshot) -> None:
-            snapshot.version = 3
 
-        snapshot = self.modify_metadata("snapshot", snapshot_version_modifier)
+        #intermediate snapshot is allowed to not match meta version
+        self.trusted_set.update_snapshot(self.metadata["snapshot"])
+
+        # final snapshot must match meta version
         with self.assertRaises(exceptions.BadVersionNumberError):
-            self.trusted_set.update_snapshot(snapshot)
+            self.trusted_set.update_targets(self.metadata["targets"])
+
 
-    def test_update_snapshot_after_successful_update_new_snapshot_no_meta(self):
+    def test_update_snapshot_file_removed_from_meta(self):
         self._update_all_besides_targets(self.metadata["timestamp"])
-        # Test removing a meta_file in new_snapshot compared to the old snapshot
-        def no_meta_modifier(snapshot: Snapshot) -> None:
-            snapshot.meta = {}
+        def remove_file_from_meta(snapshot: Snapshot) -> None:
+            del snapshot.meta["targets.json"]
 
-        snapshot = self.modify_metadata("snapshot", no_meta_modifier)
+        # Test removing a meta_file in new_snapshot compared to the old snapshot
+        snapshot = self.modify_metadata("snapshot", remove_file_from_meta)
         with self.assertRaises(exceptions.RepositoryError):
             self.trusted_set.update_snapshot(snapshot)
 
-    def test_update_snapshot_after_succesfull_update_new_snapshot_meta_version_different(self):
+    def test_update_snapshot_meta_version_decreases(self):
         self._root_updated_and_update_timestamp(self.metadata["timestamp"])
-        # snapshot.meta["project1"].version != new_snapshot.meta["project1"].version
+
         def version_meta_modifier(snapshot: Snapshot) -> None:
-            for metafile_path in snapshot.meta.keys():
-                snapshot.meta[metafile_path].version += 1
+            snapshot.meta["targets.json"].version += 1
 
         snapshot = self.modify_metadata("snapshot", version_meta_modifier)
         self.trusted_set.update_snapshot(snapshot)
+
         with self.assertRaises(exceptions.BadVersionNumberError):
             self.trusted_set.update_snapshot(self.metadata["snapshot"])
 
@@ -343,6 +344,26 @@ def snapshot_expired_modifier(snapshot: Snapshot) -> None:
         with self.assertRaises(exceptions.ExpiredMetadataError):
             self.trusted_set.update_targets(self.metadata["targets"])
 
+    def test_update_snapshot_successful_rollback_checks(self):
+        def meta_version_bump(timestamp: Timestamp) -> None:
+            timestamp.meta["snapshot.json"].version += 1
+
+        def version_bump(snapshot: Snapshot) -> None:
+            snapshot.version += 1
+
+        # load a "local" timestamp, then update to newer one:
+        self.trusted_set.update_timestamp(self.metadata["timestamp"])
+        new_timestamp = self.modify_metadata("timestamp", meta_version_bump)
+        self.trusted_set.update_timestamp(new_timestamp)
+
+        # load a "local" snapshot, then update to newer one:
+        self.trusted_set.update_snapshot(self.metadata["snapshot"])
+        new_snapshot = self.modify_metadata("snapshot", version_bump)
+        self.trusted_set.update_snapshot(new_snapshot)
+
+        # update targets to trigger final snapshot meta version check
+        self.trusted_set.update_targets(self.metadata["targets"])
+
     def test_update_targets_no_meta_in_snapshot(self):
         def no_meta_modifier(snapshot: Snapshot) -> None:
             snapshot.meta = {}

tuf/ngclient/_internal/trusted_metadata_set.py

@@ -240,9 +240,11 @@ def update_timestamp(self, data: bytes):
     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().
+        Note that intermediate snapshot is considered valid even if it is
+        expired or the version does not match the timestamp meta version. This
+        means the intermediate snapshot can be used for rollback checks on
+        newer, final snapshot. Expiry and meta version are only checked for
+        the final snapshot in update_delegated_targets().
 
         Args:
             data: unverified new snapshot metadata as bytes
@@ -285,18 +287,10 @@ def update_snapshot(self, data: bytes):
 
         self.root.verify_delegate("snapshot", new_snapshot)
 
-        if (
-            new_snapshot.signed.version
-            != self.timestamp.signed.meta["snapshot.json"].version
-        ):
-            raise exceptions.BadVersionNumberError(
-                f"Expected snapshot version "
-                f"{self.timestamp.signed.meta['snapshot.json'].version}, "
-                f"got {new_snapshot.signed.version}"
-            )
+        # version not checked against meta version to allow old snapshot to be
+        # used in rollback protection: it is checked when targets is updated
 
-        # If an existing trusted snapshot is updated,
-        # check for a rollback attack
+        # If an existing trusted snapshot is updated, check for rollback attack
         if self.snapshot is not None:
             for filename, fileinfo in self.snapshot.signed.meta.items():
                 new_fileinfo = new_snapshot.signed.meta.get(filename)
@@ -315,11 +309,25 @@ def update_snapshot(self, data: bytes):
                     )
 
         # expiry not checked to allow old snapshot to be used for rollback
-        # protection of new snapshot: expiry is checked in update_targets()
+        # protection of new snapshot: it is checked when targets is updated
 
         self._trusted_set["snapshot"] = new_snapshot
         logger.debug("Updated snapshot")
 
+    def _check_final_snapshot(self):
+        if self.snapshot.signed.is_expired(self.reference_time):
+            raise exceptions.ExpiredMetadataError("snapshot.json is expired")
+
+        if (
+            self.snapshot.signed.version
+            != self.timestamp.signed.meta["snapshot.json"].version
+        ):
+            raise exceptions.BadVersionNumberError(
+                f"Expected snapshot version "
+                f"{self.timestamp.signed.meta['snapshot.json'].version}, "
+                f"got {self.snapshot.signed.version}"
+            )
+
     def update_targets(self, data: bytes):
         """Verifies and loads 'data' as new top-level targets metadata.
 
@@ -349,10 +357,10 @@ 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")
+        # Local snapshot was allowed to be expired and to not match meta
+        # version to allow for rollback checks on new snapshot but now
+        # snapshot must not be expired and must match meta version
+        self._check_final_snapshot()
 
         delegator: Optional[Metadata] = self.get(delegator_name)
         if delegator is None: