Metadata API: enforce role name uniqueness

The spec does not say anything about role name uniqueness in a
delegations object, but I believe we cannot safely allow multiple roles
with the same role name in the roles array of a delegations object.
If we did then the roles could have different keyids, and then we would
end up in a situation where metadata may be both a valid delegation
and an invalid delegation at the same time, depending on how the role
gets chosen and that does not seem like the intention of the design.
There is an issue open in the specification with number 167 about
that issue.

Regardless of the Metadata API, I think we should enforce role name
uniqueness.
I chose to change the data structure containing roles to
OrderedDict, where keys are role names and values are DelegatedRole
instances.
This made sense to me as role names are the unique identifier of a role
and their order is important to the way they are traversed afterward.

Note: we can't use OrderedDict as type annotation until we drop support
for Python 3.6:
https://docs.python.org/3/library/typing.html#typing.OrderedDict
That's why I used quotes around "OrderedDict" annotation, because I
can't import it.

Signed-off-by: Martin Vrachev <[email protected]>

tests/test_metadata_serialization.py

@@ -304,6 +304,22 @@ def test_invalid_delegated_role_serialization(self, test_case_data: str):
             DelegatedRole.from_dict(copy.copy(case_dict))
 
 
+    invalid_delegations: DataSet = {
+        "duplicate role names": '{"keys": {"keyid" : {"keytype": "rsa", "scheme": "rsassa-pss-sha256", "keyval": {"public": "foo"}}}, \
+            "roles": [ \
+                {"keyids": ["keyid"], "name": "a", "paths": ["fn1", "fn2"], "terminating": false, "threshold": 3}, \
+                {"keyids": ["keyid2"], "name": "a", "paths": ["fn3"], "terminating": false, "threshold": 2}    \
+                ] \
+            }',
+    }
+
+    @run_sub_tests_with_dataset(invalid_delegations)
+    def test_invalid_delegation_serialization(self, test_case_data: str):
+        case_dict = json.loads(test_case_data)
+        with self.assertRaises(ValueError):
+            Delegations.from_dict(copy.deepcopy(case_dict))
+
+
     valid_delegations: DataSet = {
         "all":
             '{"keys": { \

tuf/api/metadata.py

@@ -339,10 +339,7 @@ def verify_delegate(
                 raise ValueError(f"No delegation found for {delegated_role}")
 
             keys = self.signed.delegations.keys
-            roles = self.signed.delegations.roles
-            # Assume role names are unique in delegations.roles: #1426
-            # Find first role in roles with matching name (or None if no match)
-            role = next((r for r in roles if r.name == delegated_role), None)
+            role = self.signed.delegations.roles.get(delegated_role)
         else:
             raise TypeError("Call is valid only on delegator metadata")
 
@@ -1129,16 +1126,17 @@ class Delegations:
 
     Attributes:
         keys: Dictionary of keyids to Keys. Defines the keys used in 'roles'.
-        roles: List of DelegatedRoles that define which keys are required to
-            sign the metadata for a specific role. The roles order also
-            defines the order that role delegations are considered in.
+        roles: Ordered dictionary of role names to DelegatedRoles instances. It
+            defines which keys are required to sign the metadata for a specific
+            role. The roles order also defines the order that role delegations
+            are considered in.
         unrecognized_fields: Dictionary of all unrecognized fields.
     """
 
     def __init__(
         self,
         keys: Dict[str, Key],
-        roles: List[DelegatedRole],
+        roles: "OrderedDict[str, DelegatedRole]",
         unrecognized_fields: Optional[Mapping[str, Any]] = None,
     ) -> None:
         self.keys = keys
@@ -1153,17 +1151,19 @@ def from_dict(cls, delegations_dict: Dict[str, Any]) -> "Delegations":
         for keyid, key_dict in keys.items():
             keys_res[keyid] = Key.from_dict(keyid, key_dict)
         roles = delegations_dict.pop("roles")
-        roles_res = []
+        roles_res: "OrderedDict[str, DelegatedRole]" = OrderedDict()
         for role_dict in roles:
             new_role = DelegatedRole.from_dict(role_dict)
-            roles_res.append(new_role)
+            if new_role.name in roles_res:
+                raise ValueError(f"Duplicate role {new_role.name}")
+            roles_res[new_role.name] = new_role
         # All fields left in the delegations_dict are unrecognized.
         return cls(keys_res, roles_res, delegations_dict)
 
     def to_dict(self) -> Dict[str, Any]:
         """Returns the dict representation of self."""
         keys = {keyid: key.to_dict() for keyid, key in self.keys.items()}
-        roles = [role_obj.to_dict() for role_obj in self.roles]
+        roles = [role_obj.to_dict() for role_obj in self.roles.values()]
         return {
             "keys": keys,
             "roles": roles,

tuf/ngclient/updater.py

@@ -420,7 +420,7 @@ def _preorder_depth_first_walk(
                 child_roles_to_visit = []
                 # NOTE: This may be a slow operation if there are many
                 # delegated roles.
-                for child_role in role_metadata.delegations.roles:
+                for child_role in role_metadata.delegations.roles.values():
                     if child_role.is_delegated_path(target_filepath):
                         logger.debug("Adding child role %s", child_role.name)