Root and Targets key API changes

Here is the list of all breaking API changes:
1) The "role" and "key" arguments in "Root.add_key()" are in reverse
order - "key" becomes first and "role" second.
2) "Root.remove_key()" has been renamed to "Root.revoke_key()".
3) The "role" and "keyid" arguments in "Root.revoke_key()" are in
reverse order - "keyid" becomes first and "role" second.
4) The "role" and "key" arguments in "Targets.add_key()" are in reverse
order - "key" becomes first and "role" second.
5) "Targets.remove_key()" has been renamed to "Targets.revoke_key()".
6) The "role" and "keyid" arguments in "Targets.revoke_key()" are in
reverse order - "keyid" becomes first and "role" second.
7) In both methods "Targets.add_key()" and "Targets.revoke_key()" the
"role" argument becomes an optional with a default value of None.

Those changes are made in an effort to make those methods logical
for both cases when standard roles and succinct_roles are used.
The "Root" API change was done in order to preserve naming and argument
order consistency with "Targets" API.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Author: Martin Vrachev

docs/repository-library-design.md

@@ -63,7 +63,7 @@ are found, in the python-tuf library):
 ```python
 with repository.edit(“targets”) as targets:
     # adds a key for role1 (as an example, arbitrary edits are allowed)
-    targets.add_key(“role1”, key)
+    targets.add_key(key, “role1”)
 ```
 
 This code loads current targets metadata for editing, adds the key to a role,

examples/repo_example/basic_repo.py

@@ -157,7 +157,7 @@ def _in(days: float) -> datetime:
 for name in ["targets", "snapshot", "timestamp", "root"]:
     keys[name] = generate_ed25519_key()
     roles["root"].signed.add_key(
-        name, Key.from_securesystemslib_key(keys[name])
+        Key.from_securesystemslib_key(keys[name]), name
     )
 
 # NOTE: We only need the public part to populate root, so it is possible to use
@@ -173,7 +173,7 @@ def _in(days: float) -> datetime:
 # required signature threshold.
 another_root_key = generate_ed25519_key()
 roles["root"].signed.add_key(
-    "root", Key.from_securesystemslib_key(another_root_key)
+    Key.from_securesystemslib_key(another_root_key), "root"
 )
 roles["root"].signed.roles["root"].threshold = 2
 
@@ -343,9 +343,9 @@ def _in(days: float) -> datetime:
 # remains in place, it can be used to count towards the old and new threshold.
 new_root_key = generate_ed25519_key()
 
-roles["root"].signed.remove_key("root", keys["root"]["keyid"])
+roles["root"].signed.revoke_key(keys["root"]["keyid"], "root")
 roles["root"].signed.add_key(
-    "root", Key.from_securesystemslib_key(new_root_key)
+    Key.from_securesystemslib_key(new_root_key), "root"
 )
 roles["root"].signed.version += 1
 

tests/generated_data/generate_md.py

@@ -89,10 +89,10 @@ def generate_all_files(
     md_snapshot = Metadata(Snapshot(expires=EXPIRY))
     md_targets = Metadata(Targets(expires=EXPIRY))
 
-    md_root.signed.add_key("root", keys["ed25519_0"])
-    md_root.signed.add_key("timestamp", keys["ed25519_1"])
-    md_root.signed.add_key("snapshot", keys["ed25519_2"])
-    md_root.signed.add_key("targets", keys["ed25519_3"])
+    md_root.signed.add_key(keys["ed25519_0"], "root")
+    md_root.signed.add_key(keys["ed25519_1"], "timestamp")
+    md_root.signed.add_key(keys["ed25519_2"], "snapshot")
+    md_root.signed.add_key(keys["ed25519_3"], "targets")
 
     for i, md in enumerate([md_root, md_timestamp, md_snapshot, md_targets]):
         assert isinstance(md, Metadata)

tests/repository_simulator.py

@@ -169,7 +169,7 @@ def rotate_keys(self, role: str) -> None:
         self.signers[role].clear()
         for _ in range(0, self.root.roles[role].threshold):
             key, signer = self.create_key()
-            self.root.add_key(role, key)
+            self.root.add_key(key, role)
             self.add_signer(role, signer)
 
     def _initialize(self) -> None:
@@ -182,7 +182,7 @@ def _initialize(self) -> None:
 
         for role in TOP_LEVEL_ROLE_NAMES:
             key, signer = self.create_key()
-            self.md_root.signed.add_key(role, key)
+            self.md_root.signed.add_key(key, role)
             self.add_signer(role, signer)
 
         self.publish_root()
@@ -370,7 +370,7 @@ def add_delegation(
 
         # By default add one new key for the role
         key, signer = self.create_key()
-        delegator.add_key(role.name, key)
+        delegator.add_key(key, role.name)
         self.add_signer(role.name, signer)
 
         # Add metadata for the role

tests/test_api.py

@@ -361,7 +361,7 @@ def test_metadata_verify_delegate(self) -> None:
 
         # 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(Snapshot.type, root.signed.keys[ts_keyid])
+        root.signed.add_key(root.signed.keys[ts_keyid], Snapshot.type)
         snapshot.signatures[ts_keyid] = Signature(ts_keyid, "ff" * 64)
 
         # verify succeeds if threshold is reached even if some signatures
@@ -390,7 +390,7 @@ def test_key_class(self) -> None:
         with self.assertRaises(ValueError):
             Key.from_securesystemslib_key(sslib_key)
 
-    def test_root_add_key_and_remove_key(self) -> None:
+    def test_root_add_key_and_revoke_key(self) -> None:
         root_path = os.path.join(self.repo_dir, "metadata", "root.json")
         root = Metadata[Root].from_file(root_path)
 
@@ -410,8 +410,12 @@ def test_root_add_key_and_remove_key(self) -> None:
         self.assertNotIn(keyid, root.signed.roles[Root.type].keyids)
         self.assertNotIn(keyid, root.signed.keys)
 
+        # Assert that add_key with old argument order will raise an error
+        with self.assertRaises(ValueError):
+            root.signed.add_key(Root.type, key_metadata)  # type: ignore
+
         # Add new root key
-        root.signed.add_key(Root.type, key_metadata)
+        root.signed.add_key(key_metadata, Root.type)
 
         # Assert that key is added
         self.assertIn(keyid, root.signed.roles[Root.type].keyids)
@@ -423,30 +427,30 @@ def test_root_add_key_and_remove_key(self) -> None:
 
         # Try adding the same key again and assert its ignored.
         pre_add_keyid = root.signed.roles[Root.type].keyids.copy()
-        root.signed.add_key(Root.type, key_metadata)
+        root.signed.add_key(key_metadata, Root.type)
         self.assertEqual(pre_add_keyid, root.signed.roles[Root.type].keyids)
 
         # Add the same key to targets role as well
-        root.signed.add_key(Targets.type, key_metadata)
+        root.signed.add_key(key_metadata, Targets.type)
 
         # Add the same key to a nonexistent role.
         with self.assertRaises(ValueError):
-            root.signed.add_key("nosuchrole", key_metadata)
+            root.signed.add_key(key_metadata, "nosuchrole")
 
         # Remove the key from root role (targets role still uses it)
-        root.signed.remove_key(Root.type, keyid)
+        root.signed.revoke_key(keyid, Root.type)
         self.assertNotIn(keyid, root.signed.roles[Root.type].keyids)
         self.assertIn(keyid, root.signed.keys)
 
         # Remove the key from targets as well
-        root.signed.remove_key(Targets.type, keyid)
+        root.signed.revoke_key(keyid, Targets.type)
         self.assertNotIn(keyid, root.signed.roles[Targets.type].keyids)
         self.assertNotIn(keyid, root.signed.keys)
 
         with self.assertRaises(ValueError):
-            root.signed.remove_key(Root.type, "nosuchkey")
+            root.signed.revoke_key("nosuchkey", Root.type)
         with self.assertRaises(ValueError):
-            root.signed.remove_key("nosuchrole", keyid)
+            root.signed.revoke_key(keyid, "nosuchrole")
 
     def test_is_target_in_pathpattern(self) -> None:
         # pylint: disable=protected-access
@@ -507,9 +511,13 @@ def test_targets_key_api(self) -> None:
         }
         key = Key.from_dict("id2", key_dict)
 
+        # Assert that add_key with old argument order will raise an error
+        with self.assertRaises(ValueError):
+            targets.add_key("role1", key)  # type: ignore
+
         # Assert that delegated role "role1" does not contain the new key
         self.assertNotIn(key.keyid, targets.delegations.roles["role1"].keyids)
-        targets.add_key("role1", key)
+        targets.add_key(key, "role1")
 
         # Assert that the new key is added to the delegated role "role1"
         self.assertIn(key.keyid, targets.delegations.roles["role1"].keyids)
@@ -519,46 +527,89 @@ def test_targets_key_api(self) -> None:
 
         # Try adding the same key again and assert its ignored.
         past_keyid = targets.delegations.roles["role1"].keyids.copy()
-        targets.add_key("role1", key)
+        targets.add_key(key, "role1")
         self.assertEqual(past_keyid, targets.delegations.roles["role1"].keyids)
 
         # Try adding a key to a delegated role that doesn't exists
         with self.assertRaises(ValueError):
-            targets.add_key("nosuchrole", key)
+            targets.add_key(key, "nosuchrole")
 
         # Add the same key to "role2" as well
-        targets.add_key("role2", key)
+        targets.add_key(key, "role2")
 
         # Remove the key from "role1" role ("role2" still uses it)
-        targets.remove_key("role1", key.keyid)
+        targets.revoke_key(key.keyid, "role1")
 
         # Assert that delegated role "role1" doesn't contain the key.
         self.assertNotIn(key.keyid, targets.delegations.roles["role1"].keyids)
         self.assertIn(key.keyid, targets.delegations.roles["role2"].keyids)
 
         # Remove the key from "role2" as well
-        targets.remove_key("role2", key.keyid)
+        targets.revoke_key(key.keyid, "role2")
         self.assertNotIn(key.keyid, targets.delegations.roles["role2"].keyids)
 
         # Try remove key not used by "role1"
         with self.assertRaises(ValueError):
-            targets.remove_key("role1", key.keyid)
+            targets.revoke_key(key.keyid, "role1")
 
         # Try removing a key from delegated role that doesn't exists
         with self.assertRaises(ValueError):
-            targets.remove_key("nosuchrole", key.keyid)
+            targets.revoke_key(key.keyid, "nosuchrole")
 
         # Remove delegations as a whole
         targets.delegations = None
-        # Test that calling add_key and remove_key throws an error
+        # Test that calling add_key and revoke_key throws an error
         # and that delegations is still None after each of the api calls
         with self.assertRaises(ValueError):
-            targets.add_key("role1", key)
+            targets.add_key(key, "role1")
         self.assertTrue(targets.delegations is None)
         with self.assertRaises(ValueError):
-            targets.remove_key("role1", key.keyid)
+            targets.revoke_key(key.keyid, "role1")
         self.assertTrue(targets.delegations is None)
 
+    def test_targets_key_api_with_succinct_roles(self) -> None:
+        targets_path = os.path.join(self.repo_dir, "metadata", "targets.json")
+        targets: Targets = Metadata[Targets].from_file(targets_path).signed
+        key_dict = {
+            "keytype": "ed25519",
+            "keyval": {
+                "public": "edcd0a32a07dce33f7c7873aaffbff36d20ea30787574ead335eefd337e4dacd"
+            },
+            "scheme": "ed25519",
+        }
+        key = Key.from_dict("id2", key_dict)
+
+        # Remove delegated roles.
+        assert targets.delegations is not None
+        assert targets.delegations.roles is not None
+        targets.delegations.roles = None
+        targets.delegations.keys = {}
+
+        # Add succinct_roles information.
+        targets.delegations.succinct_roles = SuccinctRoles([], 1, 8, "foo")
+        self.assertEqual(len(targets.delegations.keys), 0)
+        self.assertEqual(len(targets.delegations.succinct_roles.keyids), 0)
+
+        # Add a key to succinct_roles and verify it's saved.
+        targets.add_key(key)
+        self.assertIn(key.keyid, targets.delegations.keys)
+        self.assertIn(key.keyid, targets.delegations.succinct_roles.keyids)
+        self.assertEqual(len(targets.delegations.keys), 1)
+
+        # Try adding the same key again and verify that noting is added.
+        targets.add_key(key)
+        self.assertEqual(len(targets.delegations.keys), 1)
+
+        # Remove the key and verify it's not stored anymore.
+        targets.revoke_key(key.keyid)
+        self.assertNotIn(key.keyid, targets.delegations.keys)
+        self.assertNotIn(key.keyid, targets.delegations.succinct_roles.keyids)
+        self.assertEqual(len(targets.delegations.keys), 0)
+
+        # Try removing it again.
+        with self.assertRaises(ValueError):
+            targets.revoke_key(key.keyid)
+
     def test_length_and_hash_validation(self) -> None:
 
         # Test metadata files' hash and length verification.

tests/test_updater_key_rotations.py

@@ -184,7 +184,7 @@ def test_root_rotation(self, root_versions: List[MdVersion]) -> None:
 
             self.sim.root.roles[Root.type].threshold = rootver.threshold
             for i in rootver.keys:
-                self.sim.root.add_key(Root.type, self.keys[i])
+                self.sim.root.add_key(self.keys[i], Root.type)
             for i in rootver.sigs:
                 self.sim.add_signer(Root.type, self.signers[i])
             self.sim.root.version += 1
@@ -254,7 +254,7 @@ def test_non_root_rotations(self, md_version: MdVersion) -> None:
 
             self.sim.root.roles[role].threshold = md_version.threshold
             for i in md_version.keys:
-                self.sim.root.add_key(role, self.keys[i])
+                self.sim.root.add_key(self.keys[i], role)
 
             for i in md_version.sigs:
                 self.sim.add_signer(role, self.signers[i])

tuf/api/metadata.py

@@ -945,28 +945,33 @@ def to_dict(self) -> Dict[str, Any]:
         )
         return root_dict
 
-    def add_key(self, role: str, key: Key) -> None:
+    def add_key(self, key: Key, role: str) -> None:
         """Adds new signing key for delegated role ``role``.
 
         Args:
-            role: Name of the role, for which ``key`` is added.
             key: Signing key to be added for ``role``.
+            role: Name of the role, for which ``key`` is added.
 
         Raises:
-            ValueError: If ``role`` doesn't exist.
+            ValueError: If the argument order is wrong or if ``role`` doesn't
+                exist.
         """
+        # Verify that our users are not using the old argument order.
+        if isinstance(role, Key):
+            raise ValueError("Role must be a string, not a Key instance")
+
         if role not in self.roles:
             raise ValueError(f"Role {role} doesn't exist")
         if key.keyid not in self.roles[role].keyids:
             self.roles[role].keyids.append(key.keyid)
         self.keys[key.keyid] = key
 
-    def remove_key(self, role: str, keyid: str) -> None:
-        """Removes key from ``role`` and updates the key store.
+    def revoke_key(self, keyid: str, role: str) -> None:
+        """Revoke key from ``role`` and updates the key store.
 
         Args:
-            role: Name of the role, for which a signing key is removed.
             keyid: Identifier of the key to be removed for ``role``.
+            role: Name of the role, for which a signing key is removed.
 
         Raises:
             ValueError: If ``role`` doesn't exist or if ``role`` doesn't include
@@ -1972,17 +1977,23 @@ def to_dict(self) -> Dict[str, Any]:
             targets_dict["delegations"] = self.delegations.to_dict()
         return targets_dict
 
-    def add_key(self, role: str, key: Key) -> None:
+    def add_key(self, key: Key, role: Optional[str] = None) -> None:
         """Adds new signing key for delegated role ``role``.
 
+        If succinct_roles is used then the ``role`` argument is not required.
+
         Args:
-            role: Name of the role, for which ``key`` is added.
             key: Signing key to be added for ``role``.
+            role: Name of the role, for which ``key`` is added.
 
         Raises:
-            ValueError: If there are no delegated roles or if ``role`` is not
-                delegated by this Target.
+            ValueError: If the argument order is wrong or if there are no
+                delegated roles or if ``role`` is not delegated by this Target.
         """
+        # Verify that our users are not using the old argument order.
+        if isinstance(role, Key):
+            raise ValueError("Role must be a string, not a Key instance")
+
         if self.delegations is None:
             raise ValueError(f"Delegated role {role} doesn't exist")
 
@@ -1991,19 +2002,27 @@ def add_key(self, role: str, key: Key) -> None:
                 raise ValueError(f"Delegated role {role} doesn't exist")
             if key.keyid not in self.delegations.roles[role].keyids:
                 self.delegations.roles[role].keyids.append(key.keyid)
-            self.delegations.keys[key.keyid] = key
 
-    def remove_key(self, role: str, keyid: str) -> None:
-        """Removes key from delegated role ``role`` and updates the delegations
+        elif self.delegations.succinct_roles is not None:
+            if key.keyid not in self.delegations.succinct_roles.keyids:
+                self.delegations.succinct_roles.keyids.append(key.keyid)
+
+        self.delegations.keys[key.keyid] = key
+
+    def revoke_key(self, keyid: str, role: Optional[str] = None) -> None:
+        """Revokes key from delegated role ``role`` and updates the delegations
         key store.
 
+        If succinct_roles is used then the ``role`` argument is not required.
+
         Args:
-            role: Name of the role, for which a signing key is removed.
             keyid: Identifier of the key to be removed for ``role``.
+            role: Name of the role, for which a signing key is removed.
 
         Raises:
             ValueError: If there are no delegated roles or if ``role`` is not
-                delegated by this ``Target`` or if key is not used by ``role``.
+                delegated by this ``Target`` or if key is not used by ``role``
+                or if key with id ``keyid`` is not used by succinct roles.
         """
         if self.delegations is None:
             raise ValueError(f"Delegated role {role} doesn't exist")
@@ -2019,4 +2038,12 @@ def remove_key(self, role: str, keyid: str) -> None:
                 if keyid in keyinfo.keyids:
                     return
 
-            del self.delegations.keys[keyid]
+        elif self.delegations.succinct_roles is not None:
+            if keyid not in self.delegations.succinct_roles.keyids:
+                raise ValueError(
+                    f"Key with id {keyid} is not used by succinct_roles"
+                )
+
+            self.delegations.succinct_roles.keyids.remove(keyid)
+
+        del self.delegations.keys[keyid]