Metadata API: Role keyids is not ordered

[jku]

We made Role.keyids a set because the keyids are supposed to be unique -- this still makes sense

However, the data should also preserve order (when deserialized and serialized) and currently it does not. This is fairly serious since writing signed data potentially modifies the data (making the signature invalid).

• We should have tests that deserialize/serialize metadata with more than one item in each container (like roles keyids) to notice issues like this -- this should be easy now that the serialization tests exist
• We should fix the issue with Role.keyids -- my assumption is that the issue is Set not being ordered but I'm not 100% on that

[JustinCappos]

Good catch. I wonder if there are multiple valid ways to get the same set from serialized data. I'm not sure yet what all attacks this could enable, but it is worth thinking about. @marina Moore ***@***.***>

…

On Fri, Jul 2, 2021 at 9:16 PM Jussi Kukkonen ***@***.***> wrote: We made Role.keyids a set because the keyids are supposed to be unique -- this still makes sense However, the data should also preserve order (when deserialized and serialized) and currently it does not. This is fairly serious since writing signed data potentially modifies the data (making the signature invalid). - We should have tests that sign and verify metadata with more than one item in each container (like roles keyids) to notice issues like this - We should fix the issue with Role.keyids -- my assumption is that the issue is Set not being ordered but I'm not 100% on that — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1478>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRODYUOSNRXYEYFDJSVPDTVW33VANCNFSM47WWV76A> .

[jku]

The serialization test case is indeed easy: this fails test_role_serialization() almost always:

"many keyids": '{"keyids": ["a", "b", "c", "d", "e"], "threshold": 1}',

So I think the easiest solution is:

• revert the decision to use Set for Role.keyids -- python does not have an OrderedSet and using a Dict for this looks really weird
• instead make the attribute a private _keyids: List[str] and add a public property keyids: List[str] (with getter/setter)
• enforce uniqueness in the setter, and use the setter in deserialiation

[sechkova]

Here is the commit making signatures deterministic by sorting the set of signing keys in case this may help: 846604a

But in this case keys would have to be the same sorted set on serialization and deserialization so it may not be the best solution.

[jku]

I'm sure sorted() is reliable in its sort but the return value is not a set (it's a list) -- so we won't get the benefits we were hoping for by using a set (namely the API safety in that adding non-unique keyids would be very difficult).

Edit: or maybe you mean we could use set but always serialize using sorted? That might work...