Repository: theupdateframework/python-tuf (python)

Issue:
VerificationResult should include keys for keyids
The `get_verification_result` method returns a `VerificationResult`, which contains two sets of TUF keyids.

Typical usage of a VerificationResult entails retrieving the `Key` objects for those keyids from the corresponding root metadata keystore. It would be useful, if `VerificationResult` included Key objects right away.

Options are:
1. Use sets of Key objects  - This requires making the `Key` class hashable and immutable, which is possible but maybe overly complicated (see [discussion)](https://github.com/theupdateframework/python-tuf/issues/2449#issuecomment-1695601623))
2. Use lists of Key objects and promise that they only contain unique keys, and optionally provide tooling for set-like operations (see [wrapper workaround](https://github.com/theupdateframework/tuf-on-ci/blob/7899655c04248de8850ed3de2c584290a842cf5a/repo/tuf_on_ci/_repository.py#L59-L102))
3. Use "keyid: Key"- dictionaries  (see [wrapper workaround](https://github.com/repository-service-tuf/repository-service-tuf-cli/blob/2db958ee9334499f474da527e13a0a9f865a1976/repository_service_tuf/cli/admin2.py#L56-L86))



Pull request:
Improve verification results
This is an attempt at fixing the verification result handling that has been found to not work well. This API is useful mostly to repository implementations: clients should likely keep using `verify_delegate() `

**Modify VerificationResult**
* Remove `union()`: this is now handled by the new RootVerificationResult
* include threshold: This is useful and can be added now that union() does not exist
* Use `dict[str, Key]` instead of `set[str]` for signed and unsigned
* Note that the (broken metadata) case of role keyid that does not have a matching key, is now not counted as "unsigned" as it was before: it's just not counted at all 

**Add RootVerificationResult**
* Externally looks like VerificationResult (without threshold)
* It is actually two VerificationResults in a trenchcoat: `Root.get_root_verification_result()` is provided as a straightforward way to get one

Note that `RootVerificationResult.signed` and `RootVerificationResult.unsigned` assume that there are no keyid collisions between the two verifying roles (same keyid is not used for two different keys): This does **not** affect `verified` status in any way (only the `signed` and `unsigned` lists) so I think this is totally acceptable.

So far the usage looks quite nice but I think we want to try to use this in an app or two to avoid redoing the mistake of the first VerificationResult... 

Fixes #2544 

- [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 0963bda8..577ddf4f 100644
--- a/.github/workflows/cd.yml
+++ b/.github/workflows/cd.yml
@@ -34,7 +34,7 @@ jobs:
         run: PIP_CONSTRAINT=requirements/build.txt python3 -m build --sdist --wheel --outdir dist/ .
 
       - name: Store build artifacts
-        uses: actions/upload-artifact@26f96dfa697d77e81fd5907df203aa23a56210a8 # v4.3.0
+        uses: actions/upload-artifact@694cdabd8bdb0f10b2cea11669e1bf5453eed0a6 # v4.2.0
         # NOTE: The GitHub release page contains the release artifacts too, but using
         # GitHub upload/download actions seems robuster: there is no need to compute
         # download URLs and tampering with artifacts between jobs is more limited.
diff --git a/examples/manual_repo/basic_repo.py b/examples/manual_repo/basic_repo.py
index 129bbc39..aa002d0f 100644
--- a/examples/manual_repo/basic_repo.py
+++ b/examples/manual_repo/basic_repo.py
@@ -20,7 +20,6 @@
 NOTE: Metadata files will be written to a 'tmp*'-directory in CWD.
 
 """
-
 import os
 import tempfile
 from datetime import datetime, timedelta
diff --git a/examples/manual_repo/hashed_bin_delegation.py b/examples/manual_repo/hashed_bin_delegation.py
index 94ce93cd..a16c72ee 100644
--- a/examples/manual_repo/hashed_bin_delegation.py
+++ b/examples/manual_repo/hashed_bin_delegation.py
@@ -15,7 +15,6 @@
 NOTE: Metadata files will be written to a 'tmp*'-directory in CWD.
 
 """
-
 import hashlib
 import os
 import tempfile
diff --git a/examples/repository/_simplerepo.py b/examples/repository/_simplerepo.py
index ece4d99f..17ce7a0f 100644
--- a/examples/repository/_simplerepo.py
+++ b/examples/repository/_simplerepo.py
@@ -8,7 +8,7 @@
 import logging
 from collections import defaultdict
 from datetime import datetime, timedelta
-from typing import Dict, List
+from typing import Dict, List, Union
 
 from securesystemslib import keys
 from securesystemslib.signer import Key, Signer, SSlibKey, SSlibSigner
@@ -20,10 +20,13 @@
     Metadata,
     MetaFile,
     Root,
+    RootVerificationResult,
+    Signed,
     Snapshot,
     TargetFile,
     Targets,
     Timestamp,
+    VerificationResult,
 )
 from tuf.repository import Repository
 
@@ -89,6 +92,25 @@ def targets_infos(self) -> Dict[str, MetaFile]:
     def snapshot_info(self) -> MetaFile:
         return self._snapshot_info
 
+    def _get_verification_result(
+        self, role: str, md: Metadata
+    ) -> Union[VerificationResult, RootVerificationResult]:
+        """Verify roles metadata using the existing repository metadata"""
+        if role == Root.type:
+            assert isinstance(md.signed, Root)
+            root = self.root()
+            previous = root if root.version > 0 else None
+            return md.signed.get_root_verification_result(
+                previous, md.signed_bytes, md.signatures
+            )
+        if role in [Timestamp.type, Snapshot.type, Targets.type]:
+            delegator: Signed = self.root()
+        else:
+            delegator = self.targets()
+        return delegator.get_verification_result(
+            role, md.signed_bytes, md.signatures
+        )
+
     def open(self, role: str) -> Metadata:
         """Return current Metadata for role from 'storage' (or create a new one)"""
 
@@ -112,6 +134,14 @@ def close(self, role: str, md: Metadata) -> None:
         for signer in self.signer_cache[role]:
             md.sign(signer, append=True)
 
+        # Double check that we only write verified metadata
+        vr = self._get_verification_result(role, md)
+        if not vr:
+            raise ValueError(f"Role {role} failed to verify")
+        keyids = [keyid[:7] for keyid in vr.signed]
+        verify_str = f"verified with keys [{', '.join(keyids)}]"
+        logger.debug("Role %s v%d: %s", role, md.signed.version, verify_str)
+
         # store new metadata version, update version caches
         self.role_cache[role].append(md)
         if role == "snapshot":
@@ -130,8 +160,6 @@ def add_target(self, path: str, content: str) -> None:
         with self.edit_targets() as targets:
             targets.targets[path] = TargetFile.from_data(path, data)
 
-        logger.debug("Targets v%d", targets.version)
-
         # update snapshot, timestamp
         self.do_snapshot()
         self.do_timestamp()
@@ -157,8 +185,6 @@ def submit_delegation(self, rolename: str, data: bytes) -> bool:
             logger.info("Failed to add delegation for %s: %s", rolename, e)
             return False
 
-        logger.debug("Targets v%d", targets.version)
-
         # update snapshot, timestamp
         self.do_snapshot()
         self.do_timestamp()
@@ -177,8 +203,6 @@ def submit_role(self, role: str, data: bytes) -> bool:
                 if not targetpath.startswith(f"{role}/"):
                     raise ValueError(f"targets allowed under {role}/ only")
 
-            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}")
 
@@ -186,10 +210,19 @@ def submit_role(self, role: str, data: bytes) -> bool:
             logger.info("Failed to add new version for %s: %s", role, e)
             return False
 
+        # Check that we only write verified metadata
+        vr = self._get_verification_result(role, md)
+        if not vr:
+            logger.info("Role %s failed to verify", role)
+            return False
+
+        keyids = [keyid[:7] for keyid in vr.signed]
+        verify_str = f"verified with keys [{', '.join(keyids)}]"
+        logger.debug("Role %s v%d: %s", role, md.signed.version, verify_str)
+
         # Checks passed: Add new delegated role version
         self.role_cache[role].append(md)
         self._targets_infos[f"{role}.json"].version = md.signed.version
-        logger.debug("%s v%d", role, md.signed.version)
 
         # To keep it simple, target content is generated from targetpath
         for targetpath in md.signed.targets:
diff --git a/requirements/lint.txt b/requirements/lint.txt
index 4cae3d76..0085af78 100644
--- a/requirements/lint.txt
+++ b/requirements/lint.txt
@@ -6,9 +6,9 @@
 # Lint tools
 # (We are not so interested in the specific versions of the tools: the versions
 # are pinned to prevent unexpected linting failures when tools update)
-black==24.1.1
+black==23.12.1
 isort==5.13.2
 pylint==3.0.3
 mypy==1.8.0
-bandit==1.7.7
+bandit==1.7.6
 pydocstyle==6.3.0
diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py
index 205678a3..e64b2da9 100644
--- a/tuf/api/metadata.py
+++ b/tuf/api/metadata.py
@@ -44,7 +44,6 @@
     Iterator,
     List,
     Optional,
-    Set,
     Tuple,
     Type,
     TypeVar,
@@ -653,30 +652,68 @@ class VerificationResult:
     """Signature verification result for delegated role metadata.
 
     Attributes:
-        verified: True, if threshold of signatures is met.
-        signed: Set of delegated keyids, which validly signed.
-        unsigned: Set of delegated keyids, which did not validly sign.
+        threshold: Number of required signatures.
+        signed: dict of keyid to Key, containing keys that have signed.
+        unsigned: dict of keyid to Key, containing keys that have not signed.
+    """
+
+    threshold: int
+    signed: Dict[str, Key]
+    unsigned: Dict[str, Key]
+
+    def __bool__(self) -> bool:
+        return self.verified
 
+    @property
+    def verified(self) -> bool:
+        """True if threshold of signatures is met."""
+        return len(self.signed) >= self.threshold
+
+    @property
+    def missing(self) -> int:
+        """Number of additional signatures required to reach threshold."""
+        return max(0, self.threshold - len(self.signed))
+
+
+@dataclass
+class RootVerificationResult:
+    """Signature verification result for root metadata.
+
+    Root must be verified by itself and the previous root version. This
+    dataclass represents both results. For the edge case of first version
+    of root, these underlying results are identical.
+
+    Note that `signed` and `unsigned` correctness requires the underlying
+    VerificationResult keys to not conflict (no reusing the same keyid for
+    different keys).
+
+    Attributes:
+        first: First underlying VerificationResult
+        second: Second underlying VerificationResult
     """
 
-    verified: bool
-    signed: Set[str]
-    unsigned: Set[str]
+    first: VerificationResult
+    second: VerificationResult
 
     def __bool__(self) -> bool:
         return self.verified
 
-    def union(self, other: "VerificationResult") -> "VerificationResult":
-        """Combine two verification results.
+    @property
+    def verified(self) -> bool:
+        """True if threshold of signatures is met in both underlying VerificationResults."""
+        return self.first.verified and self.second.verified
+
+    @property
+    def signed(self) -> Dict[str, Key]:
+        """Dictionary of all signing keys that have signed, from both VerificationResults"""
+        # return a union of all signed (in python<3.9 this requires dict unpacking)
+        return {**self.first.signed, **self.second.signed}
 
-        Can be used to verify, if root metadata is signed by the threshold of
-        keys of previous root and the threshold of keys of itself.
-        """
-        return VerificationResult(
-            self.verified and other.verified,
-            self.signed | other.signed,
-            self.unsigned | other.unsigned,
-        )
+    @property
+    def unsigned(self) -> Dict[str, Key]:
+        """Dictionary of all signing keys that have not signed, from both VerificationResults"""
+        # return a union of all unsigned (in python<3.9 this requires dict unpacking)
+        return {**self.first.unsigned, **self.second.unsigned}
 
 
 class _DelegatorMixin(metaclass=abc.ABCMeta):
@@ -719,33 +756,30 @@ def get_verification_result(
         """
         role = self.get_delegated_role(delegated_role)
 
-        signed = set()
-        unsigned = set()
+        signed = {}
+        unsigned = {}
 
         for keyid in role.keyids:
             try:
                 key = self.get_key(keyid)
             except ValueError:
-                unsigned.add(keyid)
                 logger.info("No key for keyid %s", keyid)
                 continue
 
             if keyid not in signatures:
-                unsigned.add(keyid)
+                unsigned[keyid] = key
                 logger.info("No signature for keyid %s", keyid)
                 continue
 
             sig = signatures[keyid]
             try:
                 key.verify_signature(sig, payload)
-                signed.add(keyid)
+                signed[keyid] = key
             except sslib_exceptions.UnverifiedSignatureError:
-                unsigned.add(keyid)
+                unsigned[keyid] = key
                 logger.info("Key %s failed to verify %s", keyid, delegated_role)
 
-        return VerificationResult(
-            len(signed) >= role.threshold, signed, unsigned
-        )
+        return VerificationResult(role.threshold, signed, unsigned)
 
     def verify_delegate(
         self,
@@ -773,10 +807,9 @@ def verify_delegate(
             delegated_role, payload, signatures
         )
         if not result:
-            role = self.get_delegated_role(delegated_role)
             raise UnsignedMetadataError(
                 f"{delegated_role} was signed by {len(result.signed)}/"
-                f"{role.threshold} keys"
+                f"{result.threshold} keys"
             )
 
 
@@ -935,6 +968,46 @@ def get_key(self, keyid: str) -> Key:  # noqa: D102
 
         return self.keys[keyid]
 
+    def get_root_verification_result(
+        self,
+        previous: Optional["Root"],
+        payload: bytes,
+        signatures: Dict[str, Signature],
+    ) -> RootVerificationResult:
+        """Return signature threshold verification result for two root roles.
+
+        Verify root metadata with two roles (`self` and optionally `previous`).
+
+        If the repository has no root role versions yet, `previous` can be left
+        None. In all other cases, `previous` must be the previous version of
+        the Root.
+
+        NOTE: Unlike `verify_delegate()` this method does not raise, if the
+        root metadata is not fully verified.
+
+        Args:
+            previous: The previous `Root` to verify payload with, or None
+            payload: Signed payload bytes for root
+            signatures: Signatures over payload bytes
+
+        Raises:
+            ValueError: no delegation was found for ``root`` or given Root
+                versions are not sequential.
+        """
+
+        if previous is None:
+            previous = self
+        elif self.version != previous.version + 1:
+            versions = f"v{previous.version} and v{self.version}"
+            raise ValueError(
+                f"Expected sequential root versions, got {versions}."
+            )
+
+        return RootVerificationResult(
+            previous.get_verification_result(Root.type, payload, signatures),
+            self.get_verification_result(Root.type, payload, signatures),
+        )
+
 
 class BaseFile:
     """A base class of ``MetaFile`` and ``TargetFile``.
diff --git a/tuf/ngclient/_internal/requests_fetcher.py b/tuf/ngclient/_internal/requests_fetcher.py
index 4abd3914..f68fd368 100644
--- a/tuf/ngclient/_internal/requests_fetcher.py
+++ b/tuf/ngclient/_internal/requests_fetcher.py
@@ -29,13 +29,12 @@ class RequestsFetcher(FetcherInterface):
 
     Attributes:
         socket_timeout: Timeout in seconds, used for both initial connection
-            delay and the maximum delay between bytes received.
+            delay and the maximum delay between bytes received. Default is
+            4 seconds.
         chunk_size: Chunk size in bytes used when downloading.
     """
 
-    def __init__(
-        self, socket_timeout: int = 30, chunk_size: int = 400000
-    ) -> None:
+    def __init__(self) -> None:
         # http://docs.python-requests.org/en/master/user/advanced/#session-objects:
         #
         # "The Session object allows you to persist certain parameters across
@@ -53,8 +52,8 @@ def __init__(
         self._sessions: Dict[Tuple[str, str], requests.Session] = {}
 
         # Default settings
-        self.socket_timeout: int = socket_timeout  # seconds
-        self.chunk_size: int = chunk_size  # bytes
+        self.socket_timeout: int = 4  # seconds
+        self.chunk_size: int = 400000  # bytes
 
     def _fetch(self, url: str) -> Iterator[bytes]:
         """Fetch the contents of HTTP/HTTPS url from a remote server.


Test patch:
diff --git a/requirements/test.txt b/requirements/test.txt
index db9c1350..cd98e361 100644
--- a/requirements/test.txt
+++ b/requirements/test.txt
@@ -4,4 +4,4 @@
 -r pinned.txt
 
 # coverage measurement
-coverage==7.4.1
+coverage==7.4.0
diff --git a/tests/test_api.py b/tests/test_api.py
index 517ff5bd..bf0b9606 100755
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -13,7 +13,7 @@
 import sys
 import tempfile
 import unittest
-from copy import copy
+from copy import copy, deepcopy
 from datetime import datetime, timedelta
 from typing import Any, ClassVar, Dict, Optional
 
@@ -41,6 +41,7 @@
     Metadata,
     MetaFile,
     Root,
+    RootVerificationResult,
     Signature,
     Snapshot,
     SuccinctRoles,
@@ -55,7 +56,7 @@
 logger = logging.getLogger(__name__)
 
 
-# pylint: disable=too-many-public-methods
+# pylint: disable=too-many-public-methods,too-many-statements
 class TestMetadata(unittest.TestCase):
     """Tests for public API of all classes in 'tuf/api/metadata.py'."""
 
@@ -471,95 +472,205 @@ def test_signed_verify_delegate(self) -> None:
             Snapshot.type, snapshot_md.signed_bytes, snapshot_md.signatures
         )
 
+    def test_verification_result(self) -> None:
+        vr = VerificationResult(3, {"a": None}, {"b": None})
+        self.assertEqual(vr.missing, 2)
+        self.assertFalse(vr.verified)
+        self.assertFalse(vr)
+
+        # Add a signature
+        vr.signed["c"] = None
+        self.assertEqual(vr.missing, 1)
+        self.assertFalse(vr.verified)
+        self.assertFalse(vr)
+
+        # Add last missing signature
+        vr.signed["d"] = None
+        self.assertEqual(vr.missing, 0)
+        self.assertTrue(vr.verified)
+        self.assertTrue(vr)
+
+        # Add one more signature
+        vr.signed["e"] = None
+        self.assertEqual(vr.missing, 0)
+        self.assertTrue(vr.verified)
+        self.assertTrue(vr)
+
+    def test_root_verification_result(self) -> None:
+        vr1 = VerificationResult(3, {"a": None}, {"b": None})
+        vr2 = VerificationResult(1, {"c": None}, {"b": None})
+
+        vr = RootVerificationResult(vr1, vr2)
+        self.assertEqual(vr.signed, {"a": None, "c": None})
+        self.assertEqual(vr.unsigned, {"b": None})
+        self.assertFalse(vr.verified)
+        self.assertFalse(vr)
+
+        vr1.signed["c"] = None
+        vr1.signed["f"] = None
+        self.assertEqual(vr.signed, {"a": None, "c": None, "f": None})
+        self.assertEqual(vr.unsigned, {"b": None})
+        self.assertTrue(vr.verified)
+        self.assertTrue(vr)
+
     def test_signed_get_verification_result(self) -> None:
         # Setup: Load test metadata and keys
         root_path = os.path.join(self.repo_dir, "metadata", "root.json")
         root = Metadata[Root].from_file(root_path)
-        initial_root_keyids = root.signed.roles[Root.type].keyids
-        self.assertEqual(len(initial_root_keyids), 1)
-        key1_id = initial_root_keyids[0]
-        key2 = self.keystore[Timestamp.type]
-        key2_id = key2["keyid"]
+
+        key1_id = root.signed.roles[Root.type].keyids[0]
+        key1 = root.signed.get_key(key1_id)
+
+        key2_id = root.signed.roles[Timestamp.type].keyids[0]
+        key2 = root.signed.get_key(key2_id)
+        priv_key2 = self.keystore[Timestamp.type]
+
         key3_id = "123456789abcdefg"
-        key4 = self.keystore[Snapshot.type]
-        key4_id = key4["keyid"]
+        priv_key4 = self.keystore[Snapshot.type]
+        key4_id = priv_key4["keyid"]
 
         # Test: 1 authorized key, 1 valid signature
         result = root.signed.get_verification_result(
             Root.type, root.signed_bytes, root.signatures
         )
-        self.assertTrue(result.verified)
-        self.assertEqual(result.signed, {key1_id})
-        self.assertEqual(result.unsigned, set())
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1})
+        self.assertEqual(result.unsigned, {})
 
         # Test: 2 authorized keys, 1 invalid signature
         # Adding a key, i.e. metadata change, invalidates existing signature
-        root.signed.add_key(
-            SSlibKey.from_securesystemslib_key(key2),
-            Root.type,
-        )
+        root.signed.add_key(key2, Root.type)
         result = root.signed.get_verification_result(
             Root.type, root.signed_bytes, root.signatures
         )
-        self.assertFalse(result.verified)
-        self.assertEqual(result.signed, set())
-        self.assertEqual(result.unsigned, {key1_id, key2_id})
+        self.assertFalse(result)
+        self.assertEqual(result.signed, {})
+        self.assertEqual(result.unsigned, {key1_id: key1, key2_id: key2})
 
         # Test: 3 authorized keys, 1 invalid signature, 1 key missing key data
-        # Adding a keyid w/o key, fails verification the same as no signature
-        # or an invalid signature for that key
+        # Adding a keyid w/o key, fails verification but this key is not listed
+        # in unsigned
         root.signed.roles[Root.type].keyids.append(key3_id)
         result = root.signed.get_verification_result(
             Root.type, root.signed_bytes, root.signatures
         )
-        self.assertFalse(result.verified)
-        self.assertEqual(result.signed, set())
-        self.assertEqual(result.unsigned, {key1_id, key2_id, key3_id})
+        self.assertFalse(result)
+        self.assertEqual(result.signed, {})
+        self.assertEqual(result.unsigned, {key1_id: key1, key2_id: key2})
 
         # Test: 3 authorized keys, 1 valid signature, 1 invalid signature, 1
         # key missing key data
-        root.sign(SSlibSigner(key2), append=True)
+        root.sign(SSlibSigner(priv_key2), append=True)
         result = root.signed.get_verification_result(
             Root.type, root.signed_bytes, root.signatures
         )
-        self.assertTrue(result.verified)
-        self.assertEqual(result.signed, {key2_id})
-        self.assertEqual(result.unsigned, {key1_id, key3_id})
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key2_id: key2})
+        self.assertEqual(result.unsigned, {key1_id: key1})
 
         # Test: 3 authorized keys, 1 valid signature, 1 invalid signature, 1
         # key missing key data, 1 ignored unrelated signature
-        root.sign(SSlibSigner(key4), append=True)
+        root.sign(SSlibSigner(priv_key4), append=True)
         self.assertEqual(
             set(root.signatures.keys()), {key1_id, key2_id, key4_id}
         )
-        self.assertTrue(result.verified)
-        self.assertEqual(result.signed, {key2_id})
-        self.assertEqual(result.unsigned, {key1_id, key3_id})
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key2_id: key2})
+        self.assertEqual(result.unsigned, {key1_id: key1})
 
         # See test_signed_verify_delegate for more related tests ...
 
-    def test_signed_verification_result_union(self) -> None:
-        # Test all possible "unions" (AND) of "verified" field
-        data = [
-            (True, True, True),
-            (True, False, False),
-            (False, True, False),
-            (False, False, False),
-        ]
+    def test_root_get_root_verification_result(self) -> None:
+        # Setup: Load test metadata and keys
+        root_path = os.path.join(self.repo_dir, "metadata", "root.json")
+        root = Metadata[Root].from_file(root_path)
+
+        key1_id = root.signed.roles[Root.type].keyids[0]
+        key1 = root.signed.get_key(key1_id)
+
+        key2_id = root.signed.roles[Timestamp.type].keyids[0]
+        key2 = root.signed.get_key(key2_id)
+        priv_key2 = self.keystore[Timestamp.type]
 
-        for a_part, b_part, ab_part in data:
-            self.assertEqual(
-                VerificationResult(a_part, set(), set()).union(
-                    VerificationResult(b_part, set(), set())
-                ),
-                VerificationResult(ab_part, set(), set()),
+        priv_key4 = self.keystore[Snapshot.type]
+
+        # Test: Verify with no previous root version
+        result = root.signed.get_root_verification_result(
+            None, root.signed_bytes, root.signatures
+        )
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1})
+        self.assertEqual(result.unsigned, {})
+
+        # Test: Verify with other root that is not version N-1
+        prev_root: Metadata[Root] = deepcopy(root)
+        with self.assertRaises(ValueError):
+            result = root.signed.get_root_verification_result(
+                prev_root.signed, root.signed_bytes, root.signatures
             )
 
-        # Test exemplary union (|) of "signed" and "unsigned" fields
-        a = VerificationResult(True, {"1"}, {"2"})
-        b = VerificationResult(True, {"3"}, {"4"})
-        ab = VerificationResult(True, {"1", "3"}, {"2", "4"})
-        self.assertEqual(a.union(b), ab)
+        # Test: Verify with previous root
+        prev_root.signed.version -= 1
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1})
+        self.assertEqual(result.unsigned, {})
+
+        # Test: Add a signer to previous root (threshold still 1)
+        prev_root.signed.add_key(key2, Root.type)
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1})
+        self.assertEqual(result.unsigned, {key2_id: key2})
+
+        # Test: Increase threshold in previous root
+        prev_root.signed.roles[Root.type].threshold += 1
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertFalse(result)
+        self.assertEqual(result.signed, {key1_id: key1})
+        self.assertEqual(result.unsigned, {key2_id: key2})
+
+        # Test: Sign root with both keys
+        root.sign(SSlibSigner(priv_key2), append=True)
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1, key2_id: key2})
+        self.assertEqual(result.unsigned, {})
+
+        # Test: Sign root with an unrelated key
+        root.sign(SSlibSigner(priv_key4), append=True)
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1, key2_id: key2})
+        self.assertEqual(result.unsigned, {})
+
+        # Test: Remove key1 from previous root
+        prev_root.signed.revoke_key(key1_id, Root.type)
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertFalse(result)
+        self.assertEqual(result.signed, {key1_id: key1, key2_id: key2})
+        self.assertEqual(result.unsigned, {})
+
+        # Test: Lower threshold in previous root
+        prev_root.signed.roles[Root.type].threshold -= 1
+        result = root.signed.get_root_verification_result(
+            prev_root.signed, root.signed_bytes, root.signatures
+        )
+        self.assertTrue(result)
+        self.assertEqual(result.signed, {key1_id: key1, key2_id: key2})
+        self.assertEqual(result.unsigned, {})
 
     def test_key_class(self) -> None:
         # Test if from_securesystemslib_key removes the private key from keyval
diff --git a/tests/test_metadata_serialization.py b/tests/test_metadata_serialization.py
index cc3bee37..04c53775 100644
--- a/tests/test_metadata_serialization.py
+++ b/tests/test_metadata_serialization.py
@@ -536,7 +536,7 @@ def test_delegation_serialization(self, test_case_data: str) -> None:
 
     invalid_targetfiles: utils.DataSet = {
         "no hashes": '{"length": 1}',
-        "no length": '{"hashes": {"sha256": "abc"}}',
+        "no length": '{"hashes": {"sha256": "abc"}}'
         # The remaining cases are the same as for invalid_hashes and
         # invalid_length datasets.
     }
diff --git a/tests/test_trusted_metadata_set.py b/tests/test_trusted_metadata_set.py
index dce680d4..5f6732aa 100644
--- a/tests/test_trusted_metadata_set.py
+++ b/tests/test_trusted_metadata_set.py
@@ -1,5 +1,4 @@
 """Unit tests for 'tuf/ngclient/_internal/trusted_metadata_set.py'."""
-
 import logging
 import os
 import sys
diff --git a/tests/test_updater_top_level_update.py b/tests/test_updater_top_level_update.py
index 2cd224ea..b5abb377 100644
--- a/tests/test_updater_top_level_update.py
+++ b/tests/test_updater_top_level_update.py
@@ -555,9 +555,9 @@ def test_new_targets_hash_mismatch(self) -> None:
         # Modify targets contents without updating
         # snapshot's targets hashes
         self.sim.targets.version += 1
-        self.sim.snapshot.meta["targets.json"].version = (
-            self.sim.targets.version
-        )
+        self.sim.snapshot.meta[
+            "targets.json"
+        ].version = self.sim.targets.version
         self.sim.snapshot.version += 1
         self.sim.update_timestamp()