Metadata API: preserve Role.keyids order#1481
Merged
jku merged 1 commit intotheupdateframework:developfrom Jul 15, 2021
Merged
Conversation
497a6e5 to
db55ccf
Compare
Member
|
@MVrachev can you please comment on Teodoras suggestion (comment was made in the issue)? I think the idea was that we could keep using the set if we make sure that on serialization we always sort the the output. This has some corner cases as well (like if your using the API to process keyids somehow, the order isn't guaranteed) but if that's the only issue then maybe that's a better way? |
Collaborator
Author
|
Yes, I started answering, but it seems I haven't sent my response. |
We made Role.keyids a set because the keyids are supposed to be unique and 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). The simplest solution (as proposed by Teodora) is to sort the set during serialization and that would ensure the order of the items. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
db55ccf to
df9f3df
Compare
Collaborator
Author
|
Yes, it looks a lot better. |
jku
approved these changes
Jul 14, 2021
Member
jku
left a comment
There was a problem hiding this comment.
I'm trying to think if there's any case where in the API we would need the ordered keyids... but I don't think we ever do. So this looks right to me, thanks!
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Jul 28, 2021
Test metadata dataset containing containers of zero or more elements can be serialized into objects. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested According to the spec having an empty container for any of these cases: - Root keys - Root roles - Root role keyids - Delegation keys - DelegationRole keyids - DelegationRole paths - DelegationRole path_hash_prefixes is not allowed, but for the purpose of interactive object construction we don't block those use-cases. Still, we don't want to add tests, because we don't want to advertise this behavior. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Jul 28, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested According to the spec having an empty container for any of these cases: - Root keys - Root roles - Root role keyids - Delegation keys - DelegationRole keyids - DelegationRole paths - DelegationRole path_hash_prefixes is not allowed, but for the purpose of interactive object construction we don't block those use-cases. We don't want to add tests, because we don't want to advertise this behavior. In the future, we are going to add validation that those cases don't occur which will be called when serializing the object back to bytes/dictionary/file. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
3 tasks
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Jul 28, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested According to the spec having an empty container for any of these cases: - Root keys - Root roles - Root role keyids - Delegation keys - DelegationRole keyids - DelegationRole paths - DelegationRole path_hash_prefixes is not allowed, but for the purpose of interactive object construction we don't block those use-cases. We don't want to add tests, because we don't want to advertise this behavior. In the future, we are going to add validation that those cases don't occur which will be called when serializing the object back to bytes/dictionary/file. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Aug 6, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested According to the spec having an empty container for any of these cases: - Root keys - Root roles - Root role keyids - Delegation keys - DelegationRole keyids - DelegationRole paths - DelegationRole path_hash_prefixes is not allowed, but for the purpose of interactive object construction we don't block those use-cases. We don't want to add tests, because we don't want to advertise this behavior. In the future, we are going to add validation that those cases don't occur which will be called when serializing the object back to bytes/dictionary/file. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Aug 19, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Timestamp meta: - zero elements: already tested - many elements: added Snapshot meta: - zero items: added - many items: added Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested Delegation roles: - zero roles: added - multiple roles: added Targets targets: - zero items: already tested - multiple items: added Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Aug 20, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Timestamp meta: - zero elements: already tested - many elements: added Snapshot meta: - zero items: added - many items: added Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested Delegation roles: - zero roles: added - multiple roles: added Targets targets: - zero items: already tested - multiple items: added Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Aug 20, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Timestamp meta: - zero elements: already tested - many elements: added Snapshot meta: - zero items: added - many items: added Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested Delegation roles: - zero roles: added - multiple roles: added Targets targets: - zero items: already tested - multiple items: added Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
pushed a commit
to MVrachev/tuf
that referenced
this pull request
Aug 25, 2021
Test metadata (de)serialization with input data containing containers with zero or more elements. Here is the status for the different use cases: Root keys: - many keys: added Root roles: - many roles: added Root role keyids: - many keids: already added in theupdateframework#1481 MetaFile hashes: - many hashes: already tested - zero hashes: added. Testing as invalid test case. Timestamp meta: - zero elements: already tested - many elements: added Snapshot meta: - zero items: added - many items: added Delegation keys: - many keys: added Delegation role keyids: - many keyids: added Delegation role paths: - many paths: already tested Delegation role path_hash_prefixes: - many path_hash_path_prefixes: already tested Delegation roles: - zero roles: added - multiple roles: added Targets targets: - zero items: already tested - multiple items: added Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1478
We made Role.keyids a set because the keyids are supposed
to be unique and 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).
The simplest solution (as proposed by Jussi) is to make keyids
a property and there to enforce keyids uniqueness and preserve the
order by using a list instead of a set.
Description of the changes being introduced by the pull request:
Please verify and check that the pull request fulfills the following
requirements: