Repository: theupdateframework/python-tuf (python)

Issue:
move verify_delegate() from Metadata to Signed
verify_delegate() is currently an instance method of Metadata. It could be an instance method of Signed since it does not need anything from metadata and since this would lead to cleaner code.

* Generally code that uses tuf.repository can mostly look clean as it does not need to deal with the actual `Metadata`:
    ```
    root = repo.root()
    print (f"root version is {root.version}")
    ```
    This no longer works if one wants to verify a delegate just because of API design
* We could add a `Signed.verify_delegate()` and similar code should work:
    ```
    root = repo.root()
    root.verify_delegate("targets", metadata)
    ```
* nothing prevents leaving `Metadata.verify_delegate()` in place as a deprecated method


Pull request description:
Move verify_delegate() to Root/Targets
Fixes #2361 

**Description of the changes being introduced by the pull request**:

* Add an internal `_Delegator` class that implements verify_delegate()
* Inherit that class in Root, Targets: the internal class is just an implementation detail for sharing the implementation, not something users of the library should care about
* Document Metadata.verify_delegate() as deprecated (and use the new implementation internally)
* Use the new method in tests, examples and ngclient

This will make both ngclient and external repository code slightly easier to read and write (but note that the ngclient cleanups are not included here as they touch quite a lot of lines even if they are simple).

There are several ways to add verify_delegate() to Root and Targets, I think this helper class is a decent compromise between modularity and simplicity: 
* the _Delegator class isn't intended to be part of a longer "inheritance path" (and does not have a matching element in the metadata format): it just provides an extra method for Root and Targets. If there's a naming style for this style of inheritance I'm happy to use it
* _Delegator is internal: users of the library don't need to care about it

**Please verify and check that the pull request fulfills the following
requirements**:

- [x] The code follows the [Code Style Guidelines](https://github.com/secure-systems-lab/code-style-guidelines#code-style-guidelines)
- [x] Tests have been added for the bug fix or new feature
- [x] Docs have been added for the bug fix or new feature




Patch:
diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml
index 2a90f2c4..5788cb67 100644
--- a/.github/workflows/cd.yml
+++ b/.github/workflows/cd.yml
@@ -100,7 +100,7 @@ jobs:
       - name: Publish binary wheel and source tarball on PyPI
         # Only attempt pypi upload in upstream repository
         if: github.repository == 'theupdateframework/python-tuf'
-        uses: pypa/gh-action-pypi-publish@b7f401de30cb6434a1e19f805ff006643653240e
+        uses: pypa/gh-action-pypi-publish@f8c70e705ffc13c3b4d1221169b84f12a75d6ca8
 
       - name: Finalize GitHub release
         uses: actions/github-script@d7906e4ad0b1822421a7e6a35d5ca353c962f410
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 0ce99ca9..b1dfd723 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -25,9 +25,9 @@ jobs:
       uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9
 
     - name: Initialize CodeQL
-      uses: github/codeql-action/init@a09933a12a80f87b87005513f0abb1494c27a716
+      uses: github/codeql-action/init@0ba4244466797eb048eb91a6cd43d5c03ca8bd05
       with:
         languages: 'python'
 
     - name: Perform CodeQL Analysis
-      uses: github/codeql-action/analyze@a09933a12a80f87b87005513f0abb1494c27a716
+      uses: github/codeql-action/analyze@0ba4244466797eb048eb91a6cd43d5c03ca8bd05
diff --git a/.github/workflows/dependency-review.yml b/.github/workflows/dependency-review.yml
index 8744ee3f..7db69e64 100644
--- a/.github/workflows/dependency-review.yml
+++ b/.github/workflows/dependency-review.yml
@@ -17,4 +17,4 @@ jobs:
       - name: 'Checkout Repository'
         uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9
       - name: 'Dependency Review'
-        uses: actions/dependency-review-action@f6fff72a3217f580d5afd49a46826795305b63c7
+        uses: actions/dependency-review-action@1360a344ccb0ab6e9475edef90ad2f46bf8003b1
diff --git a/.github/workflows/scorecards.yml b/.github/workflows/scorecards.yml
index 2b11169b..8e531053 100644
--- a/.github/workflows/scorecards.yml
+++ b/.github/workflows/scorecards.yml
@@ -34,6 +34,6 @@ jobs:
           publish_results: true
 
       - name: "Upload to code-scanning dashboard"
-        uses: github/codeql-action/upload-sarif@a09933a12a80f87b87005513f0abb1494c27a716
+        uses: github/codeql-action/upload-sarif@0ba4244466797eb048eb91a6cd43d5c03ca8bd05
         with:
           sarif_file: results.sarif
diff --git a/examples/repository/_simplerepo.py b/examples/repository/_simplerepo.py
index 15092958..ece4d99f 100644
--- a/examples/repository/_simplerepo.py
+++ b/examples/repository/_simplerepo.py
@@ -177,8 +177,7 @@ def submit_role(self, role: str, data: bytes) -> bool:
                 if not targetpath.startswith(f"{role}/"):
                     raise ValueError(f"targets allowed under {role}/ only")
 
-            targets_md = self.open("targets")
-            targets_md.verify_delegate(role, md)
+            self.targets().verify_delegate(role, md.signed_bytes, md.signatures)
 
             if md.signed.version != self.targets(role).version + 1:
                 raise ValueError("Invalid version {md.signed.version}")
diff --git a/requirements/lint.txt b/requirements/lint.txt
index e06bae74..63e13989 100644
--- a/requirements/lint.txt
+++ b/requirements/lint.txt
@@ -9,6 +9,6 @@
 black==23.7.0
 isort==5.12.0
 pylint==2.17.5
-mypy==1.5.0
+mypy==1.3.0
 bandit==1.7.5
 pydocstyle==6.3.0
diff --git a/requirements/pinned.txt b/requirements/pinned.txt
index 2da4a26e..bdb50839 100644
--- a/requirements/pinned.txt
+++ b/requirements/pinned.txt
@@ -1,7 +1,7 @@
 certifi==2023.7.22        # via requests
 cffi==1.15.1              # via cryptography, pynacl
 charset-normalizer==3.2.0 # via requests
-cryptography==41.0.3      # via securesystemslib
+cryptography==41.0.2      # via securesystemslib
 idna==3.4                 # via requests
 pycparser==2.21           # via cffi
 pynacl==1.5.0             # via securesystemslib
diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py
index 45840512..3134271b 100644
--- a/tuf/api/metadata.py
+++ b/tuf/api/metadata.py
@@ -150,6 +150,16 @@ def __eq__(self, other: Any) -> bool:
             and self.unrecognized_fields == other.unrecognized_fields
         )
 
+    @property
+    def signed_bytes(self) -> bytes:
+        """Default canonical json byte representation of ``self.signed``."""
+
+        # Use local scope import to avoid circular import errors
+        # pylint: disable=import-outside-toplevel
+        from tuf.api.serialization.json import CanonicalJSONSerializer
+
+        return CanonicalJSONSerializer().serialize(self.signed)
+
     @classmethod
     def from_dict(cls, metadata: Dict[str, Any]) -> "Metadata[T]":
         """Create ``Metadata`` object from its json/dict representation.
@@ -366,13 +376,9 @@ def sign(
         """
 
         if signed_serializer is None:
-            # Use local scope import to avoid circular import errors
-            # pylint: disable=import-outside-toplevel
-            from tuf.api.serialization.json import CanonicalJSONSerializer
-
-            signed_serializer = CanonicalJSONSerializer()
-
-        bytes_data = signed_serializer.serialize(self.signed)
+            bytes_data = self.signed_bytes
+        else:
+            bytes_data = signed_serializer.serialize(self.signed)
 
         try:
             signature = signer.sign(bytes_data)
@@ -393,58 +399,24 @@ def verify_delegate(
         signed_serializer: Optional[SignedSerializer] = None,
     ) -> None:
         """Verify that ``delegated_metadata`` is signed with the required
-        threshold of keys for the delegated role ``delegated_role``.
+        threshold of keys for ``delegated_role``.
 
-        Args:
-            delegated_role: Name of the delegated role to verify
-            delegated_metadata: ``Metadata`` object for the delegated role
-            signed_serializer: Serializer used for delegate
-                serialization. Default is ``CanonicalJSONSerializer``.
-
-        Raises:
-            UnsignedMetadataError: ``delegated_role`` was not signed with
-                required threshold of keys for ``role_name``.
-            ValueError: no delegation was found for ``delegated_role``.
-            TypeError: called this function on non-delegating metadata class.
+        .. deprecated:: 3.1.0
+           Please use ``Root.verify_delegate()`` or ``Targets.verify_delegate()``.
         """
 
         if self.signed.type not in ["root", "targets"]:
             raise TypeError("Call is valid only on delegator metadata")
 
         if signed_serializer is None:
-            # pylint: disable=import-outside-toplevel
-            from tuf.api.serialization.json import CanonicalJSONSerializer
-
-            signed_serializer = CanonicalJSONSerializer()
-
-        data = signed_serializer.serialize(delegated_metadata.signed)
-        role = self.signed.get_delegated_role(delegated_role)
-
-        # verify that delegated_metadata is signed by threshold of unique keys
-        signing_keys = set()
-        for keyid in role.keyids:
-            try:
-                key = self.signed.get_key(keyid)
-            except ValueError:
-                logger.info("No key for keyid %s", keyid)
-                continue
+            payload = delegated_metadata.signed_bytes
 
-            if keyid not in delegated_metadata.signatures:
-                logger.info("No signature for keyid %s", keyid)
-                continue
-
-            sig = delegated_metadata.signatures[keyid]
-            try:
-                key.verify_signature(sig, data)
-                signing_keys.add(keyid)
-            except sslib_exceptions.UnverifiedSignatureError:
-                logger.info("Key %s failed to verify %s", keyid, delegated_role)
+        else:
+            payload = signed_serializer.serialize(delegated_metadata.signed)
 
-        if len(signing_keys) < role.threshold:
-            raise UnsignedMetadataError(
-                f"{delegated_role} was signed by {len(signing_keys)}/"
-                f"{role.threshold} keys",
-            )
+        self.signed.verify_delegate(
+            delegated_role, payload, delegated_metadata.signatures
+        )
 
 
 class Signed(metaclass=abc.ABCMeta):
@@ -674,7 +646,77 @@ def to_dict(self) -> Dict[str, Any]:
         }
 
 
-class Root(Signed):
+class _DelegatorMixin(metaclass=abc.ABCMeta):
+    """Class that implements verify_delegate() for Root and Targets"""
+
+    @abc.abstractmethod
+    def get_delegated_role(self, delegated_role: str) -> Role:
+        """Return the role object for the given delegated role.
+
+        Raises ValueError if delegated_role is not actually delegated.
+        """
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def get_key(self, keyid: str) -> Key:
+        """Return the key object for the given keyid.
+
+        Raises ValueError if key is not found.
+        """
+        raise NotImplementedError
+
+    def verify_delegate(
+        self,
+        delegated_role: str,
+        payload: bytes,
+        signatures: Dict[str, Signature],
+    ) -> None:
+        """Verify signature threshold for delegated role.
+
+        Verify that there are enough valid ``signatures`` over ``payload``, to
+        meet the threshold of keys for ``delegated_role``, as defined by the
+        delegator (``self``).
+
+        Args:
+            delegated_role: Name of the delegated role to verify
+            payload: Signed payload bytes for the delegated role
+            signatures: Signatures over payload bytes
+
+        Raises:
+            UnsignedMetadataError: ``delegated_role`` was not signed with
+                required threshold of keys for ``role_name``.
+            ValueError: no delegation was found for ``delegated_role``.
+        """
+        role = self.get_delegated_role(delegated_role)
+
+        # verify that delegated_metadata is signed by threshold of unique keys
+        signing_keys = set()
+        for keyid in role.keyids:
+            try:
+                key = self.get_key(keyid)
+            except ValueError:
+                logger.info("No key for keyid %s", keyid)
+                continue
+
+            if keyid not in signatures:
+                logger.info("No signature for keyid %s", keyid)
+                continue
+
+            sig = signatures[keyid]
+            try:
+                key.verify_signature(sig, payload)
+                signing_keys.add(keyid)
+            except sslib_exceptions.UnverifiedSignatureError:
+                logger.info("Key %s failed to verify %s", keyid, delegated_role)
+
+        if len(signing_keys) < role.threshold:
+            raise UnsignedMetadataError(
+                f"{delegated_role} was signed by {len(signing_keys)}/"
+                f"{role.threshold} keys",
+            )
+
+
+class Root(Signed, _DelegatorMixin):
     """A container for the signed part of root metadata.
 
     Parameters listed below are also instance attributes.
@@ -823,11 +865,7 @@ def get_delegated_role(self, delegated_role: str) -> Role:
 
         return self.roles[delegated_role]
 
-    def get_key(self, keyid: str) -> Key:
-        """Return the key object for the given keyid.
-
-        Raises ValueError if key is not found.
-        """
+    def get_key(self, keyid: str) -> Key:  # noqa: D102
         if keyid not in self.keys:
             raise ValueError(f"Key {keyid} not found")
 
@@ -1748,7 +1786,7 @@ def get_prefixed_paths(self) -> List[str]:
         return paths
 
 
-class Targets(Signed):
+class Targets(Signed, _DelegatorMixin):
     """A container for the signed part of targets metadata.
 
     Targets contains verifying information about target files and also
@@ -1922,11 +1960,7 @@ def get_delegated_role(self, delegated_role: str) -> Role:
 
         return role
 
-    def get_key(self, keyid: str) -> Key:
-        """Return the key object for the given keyid.
-
-        Raises ValueError if keyid is not found.
-        """
+    def get_key(self, keyid: str) -> Key:  # noqa: D102
         if self.delegations is None:
             raise ValueError("No delegations found")
         if keyid not in self.delegations.keys:
diff --git a/tuf/ngclient/_internal/trusted_metadata_set.py b/tuf/ngclient/_internal/trusted_metadata_set.py
index f388591e..c99a365d 100644
--- a/tuf/ngclient/_internal/trusted_metadata_set.py
+++ b/tuf/ngclient/_internal/trusted_metadata_set.py
@@ -161,7 +161,9 @@ def update_root(self, data: bytes) -> Metadata[Root]:
             )
 
         # Verify that new root is signed by trusted root
-        self.root.verify_delegate(Root.type, new_root)
+        self.root.signed.verify_delegate(
+            Root.type, new_root.signed_bytes, new_root.signatures
+        )
 
         if new_root.signed.version != self.root.signed.version + 1:
             raise exceptions.BadVersionNumberError(
@@ -170,7 +172,9 @@ def update_root(self, data: bytes) -> Metadata[Root]:
             )
 
         # Verify that new root is signed by itself
-        new_root.verify_delegate(Root.type, new_root)
+        new_root.signed.verify_delegate(
+            Root.type, new_root.signed_bytes, new_root.signatures
+        )
 
         self._trusted_set[Root.type] = new_root
         logger.debug("Updated root v%d", new_root.signed.version)
@@ -215,7 +219,9 @@ def update_timestamp(self, data: bytes) -> Metadata[Timestamp]:
                 f"Expected 'timestamp', got '{new_timestamp.signed.type}'"
             )
 
-        self.root.verify_delegate(Timestamp.type, new_timestamp)
+        self.root.signed.verify_delegate(
+            Timestamp.type, new_timestamp.signed_bytes, new_timestamp.signatures
+        )
 
         # If an existing trusted timestamp is updated,
         # check for a rollback attack
@@ -310,7 +316,9 @@ def update_snapshot(
                 f"Expected 'snapshot', got '{new_snapshot.signed.type}'"
             )
 
-        self.root.verify_delegate(Snapshot.type, new_snapshot)
+        self.root.signed.verify_delegate(
+            Snapshot.type, new_snapshot.signed_bytes, new_snapshot.signatures
+        )
 
         # version not checked against meta version to allow old snapshot to be
         # used in rollback protection: it is checked when targets is updated
@@ -418,7 +426,9 @@ def update_delegated_targets(
                 f"Expected 'targets', got '{new_delegate.signed.type}'"
             )
 
-        delegator.verify_delegate(role_name, new_delegate)
+        delegator.signed.verify_delegate(
+            role_name, new_delegate.signed_bytes, new_delegate.signatures
+        )
 
         version = new_delegate.signed.version
         if version != meta.version:
@@ -447,7 +457,9 @@ def _load_trusted_root(self, data: bytes) -> None:
                 f"Expected 'root', got '{new_root.signed.type}'"
             )
 
-        new_root.verify_delegate(Root.type, new_root)
+        new_root.signed.verify_delegate(
+            Root.type, new_root.signed_bytes, new_root.signatures
+        )
 
         self._trusted_set[Root.type] = new_root
         logger.debug("Loaded trusted root v%d", new_root.signed.version)


Test patch:
diff --git a/tests/test_api.py b/tests/test_api.py
index 14ae12c9..f10cc57a 100755
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -43,7 +43,7 @@
     Timestamp,
 )
 from tuf.api.serialization import DeserializationError, SerializationError
-from tuf.api.serialization.json import CanonicalJSONSerializer, JSONSerializer
+from tuf.api.serialization.json import JSONSerializer
 
 logger = logging.getLogger(__name__)
 
@@ -202,7 +202,7 @@ def test_sign_verify(self) -> None:
         # Load sample metadata (targets) and assert ...
         md_obj = Metadata.from_file(os.path.join(path, "targets.json"))
         sig = md_obj.signatures[targets_keyid]
-        data = CanonicalJSONSerializer().serialize(md_obj.signed)
+        data = md_obj.signed_bytes
 
         # ... it has a single existing signature,
         self.assertEqual(len(md_obj.signatures), 1)
@@ -257,7 +257,7 @@ def test_key_verify_failures(self) -> None:
         path = os.path.join(self.repo_dir, "metadata", "timestamp.json")
         md_obj = Metadata.from_file(path)
         sig = md_obj.signatures[timestamp_keyid]
-        data = CanonicalJSONSerializer().serialize(md_obj.signed)
+        data = md_obj.signed_bytes
 
         # Test failure on unknown scheme (securesystemslib
         # UnsupportedAlgorithmError)
@@ -345,44 +345,113 @@ def test_metadata_verify_delegate(self) -> None:
         with self.assertRaises(ValueError):
             role2.verify_delegate("role1", role1)
 
+    def test_signed_verify_delegate(self) -> None:
+        # pylint: disable=too-many-locals,too-many-statements
+        root_path = os.path.join(self.repo_dir, "metadata", "root.json")
+        root_md = Metadata[Root].from_file(root_path)
+        root = root_md.signed
+        snapshot_path = os.path.join(self.repo_dir, "metadata", "snapshot.json")
+        snapshot_md = Metadata[Snapshot].from_file(snapshot_path)
+        snapshot = snapshot_md.signed
+        targets_path = os.path.join(self.repo_dir, "metadata", "targets.json")
+        targets_md = Metadata[Targets].from_file(targets_path)
+        targets = targets_md.signed
+        role1_path = os.path.join(self.repo_dir, "metadata", "role1.json")
+        role1_md = Metadata[Targets].from_file(role1_path)
+        role1 = role1_md.signed
+        role2_path = os.path.join(self.repo_dir, "metadata", "role2.json")
+        role2_md = Metadata[Targets].from_file(role2_path)
+        role2 = role2_md.signed
+
+        # test the expected delegation tree
+        root.verify_delegate(
+            Root.type, root_md.signed_bytes, root_md.signatures
+        )
+        root.verify_delegate(
+            Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+        )
+        root.verify_delegate(
+            Targets.type, targets_md.signed_bytes, targets_md.signatures
+        )
+        targets.verify_delegate(
+            "role1", role1_md.signed_bytes, role1_md.signatures
+        )
+        role1.verify_delegate(
+            "role2", role2_md.signed_bytes, role2_md.signatures
+        )
+
+        # only root and targets can verify delegates
+        with self.assertRaises(AttributeError):
+            snapshot.verify_delegate(
+                Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+            )
+        # verify fails for roles that are not delegated by delegator
+        with self.assertRaises(ValueError):
+            root.verify_delegate(
+                "role1", role1_md.signed_bytes, role1_md.signatures
+            )
+        with self.assertRaises(ValueError):
+            targets.verify_delegate(
+                Targets.type, targets_md.signed_bytes, targets_md.signatures
+            )
+        # verify fails when delegator has no delegations
+        with self.assertRaises(ValueError):
+            role2.verify_delegate(
+                "role1", role1_md.signed_bytes, role1_md.signatures
+            )
+
         # verify fails when delegate content is modified
-        expires = snapshot.signed.expires
-        snapshot.signed.expires = expires + timedelta(days=1)
+        expires = snapshot.expires
+        snapshot.expires = expires + timedelta(days=1)
         with self.assertRaises(exceptions.UnsignedMetadataError):
-            root.verify_delegate(Snapshot.type, snapshot)
-        snapshot.signed.expires = expires
+            root.verify_delegate(
+                Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+            )
+        snapshot.expires = expires
 
         # verify fails if sslib verify fails with VerificationError
         # (in this case signature is malformed)
-        keyid = next(iter(root.signed.roles[Snapshot.type].keyids))
-        good_sig = snapshot.signatures[keyid].signature
-        snapshot.signatures[keyid].signature = "foo"
+        keyid = next(iter(root.roles[Snapshot.type].keyids))
+        good_sig = snapshot_md.signatures[keyid].signature
+        snapshot_md.signatures[keyid].signature = "foo"
         with self.assertRaises(exceptions.UnsignedMetadataError):
-            root.verify_delegate(Snapshot.type, snapshot)
-        snapshot.signatures[keyid].signature = good_sig
+            root.verify_delegate(
+                Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+            )
+        snapshot_md.signatures[keyid].signature = good_sig
 
         # verify fails if roles keys do not sign the metadata
         with self.assertRaises(exceptions.UnsignedMetadataError):
-            root.verify_delegate(Timestamp.type, snapshot)
+            root.verify_delegate(
+                Timestamp.type, snapshot_md.signed_bytes, snapshot_md.signatures
+            )
 
         # Add a key to snapshot role, make sure the new sig fails to verify
-        ts_keyid = next(iter(root.signed.roles[Timestamp.type].keyids))
-        root.signed.add_key(root.signed.keys[ts_keyid], Snapshot.type)
-        snapshot.signatures[ts_keyid] = Signature(ts_keyid, "ff" * 64)
+        ts_keyid = next(iter(root.roles[Timestamp.type].keyids))
+        root.add_key(root.keys[ts_keyid], Snapshot.type)
+        snapshot_md.signatures[ts_keyid] = Signature(ts_keyid, "ff" * 64)
 
         # verify succeeds if threshold is reached even if some signatures
         # fail to verify
-        root.verify_delegate(Snapshot.type, snapshot)
+        root.verify_delegate(
+            Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+        )
 
         # verify fails if threshold of signatures is not reached
-        root.signed.roles[Snapshot.type].threshold = 2
+        root.roles[Snapshot.type].threshold = 2
         with self.assertRaises(exceptions.UnsignedMetadataError):
-            root.verify_delegate(Snapshot.type, snapshot)
+            root.verify_delegate(
+                Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+            )
 
         # verify succeeds when we correct the new signature and reach the
         # threshold of 2 keys
-        snapshot.sign(SSlibSigner(self.keystore[Timestamp.type]), append=True)
-        root.verify_delegate(Snapshot.type, snapshot)
+        snapshot_md.sign(
+            SSlibSigner(self.keystore[Timestamp.type]), append=True
+        )
+        root.verify_delegate(
+            Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
+        )
 
     def test_key_class(self) -> None:
         # Test if from_securesystemslib_key removes the private key from keyval