new API: test containers for zero or more elements #1511

MVrachev · 2021-07-28T12:15:19Z

Fixes #1485

Description of the changes being introduced by the pull request:

Test metadata (de)serialization with input data containing containers with zero or more elements.

Here is the status for the different use cases:

Root:

many keys: added
many roles: added

Root role keyids:

many keids: already added in Metadata API: preserve Role.keyids order #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:

many keys: added
many keyids: added

Delegation role:

many paths: already tested
many path_hash_path_prefixes: already tested

Delegation roles:

zero roles: added
multiple roles: added

Targets targets:

zero items: already tested
multiple items: added

Additional tests are added for containers with 0 or more elements for some specific cases.
Those tests are needed to cover use cases when syntactically as
standalone objects the metadata classes and their helper classes defined
in tuf/api/metadata.py are valid even if they cannot be verified.

An example where an object is valid, but cannot be verified is
if we have a Role instance with an empty list of "keyids".
This instance is valid and can be created, but cannot be verified
because there is a requirement that the threshold should be above
1, meaning that there should be at least 1 element inside the "keyids"
list to complete successful threshold verification.

The situation is the same for the rest of the tests I am adding to this
commit:

Root object without keys
Root object without roles
DelegationRole object with empty "keyids"
DelegationRole object with an empty list of "paths"
DelegationRole object with an empty list of "path_hash_prefixes"
all of these objects can be instantiated, but cannot complete successfully
threshold verification.

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

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

jku · 2021-08-05T09:53:24Z

According to the spec having an empty container for any of these cases ... is not allowed

is this really true? 😮

jku

some potential test cases

timestamp meta with multiple items
timestamp meta with no items
snapshot meta with multiple items
delegation with multiple roles
targets targets with multiple items
targets roles with multiple items

or do I misunderstand the scope of the PR?

tests/test_metadata_serialization.py

MVrachev · 2021-08-06T18:11:17Z

@jku I had to rebase, but 92dc844 is unchanged.

The new commits are 75b2c21 and 033e069.

MVrachev · 2021-08-07T19:01:14Z

According to the spec having an empty container for any of these cases ... is not allowed

is this really true? 😮

Root keys are not allowed to be empty, because it's required that at least one element is inside it - the key used to sign the root metadata file itself.
Root roles are not allowed to be empty, because it's required that there should be information for at least one role - the root role.
Root role keyids are not allowed to be empty, because it's required that threshold should be at least 1 (see Metadata API: Add simple threshold validation #1450) and as a consequence, there should be at least a threshold number of elements in keyids
Delegation keys and Delegation roles
For those two it's a little harder and probably my statement is wrong.
This use case is valid:

"delegations": {
     "keys": {}
     "roles": {}

for which by the way we are not testing. I included a test.
The problem comes if Delegation keys are empty, then means there shouldn't be any roles.
So, maybe my assumption is not correct.

But still, where do you think we should test for that?

DelegationRole keyids cannot be empty because every role needs at least one key to be verified because the threshold is >= 1.

I am wrong about those two:

DelegationRole paths and DelegationRole path_hash_prefixes
I am already testing for them.

Do you think I should modify my commit message with more info as I did here?

jku · 2021-08-09T08:09:05Z

I think you may be mixing "is this a syntactically valid metadata" with "can this metadata file be used successfully in every part of the update process".

I mean when you say things like:

Root role keyids are not allowed to be empty, because it's required that threshold should be at least 1

from the perspective of the file format and the containing metadata, keyids and threshold are not related. The spec does not say metadata files can't exist with a role that has less keys than the corresponding threshold. Yes, we happen to know that those keys then can't possibly verify that roles metadata when needed but the metadata that contains this role is still valid.

jku · 2021-08-09T08:19:32Z

For Root roles specifically there is a spec mention (with a weird reference to "key list" but the intent seems clear: roles dictionary must contain these 4 or 5 items):

A role for each of "root", "snapshot", "timestamp", and "targets" MUST be specified in the key list. The role of "mirror" is OPTIONAL.

This we should test for in Root construction time I guess.

jku · 2021-08-17T07:50:37Z

tuf/api/metadata.py

@@ -878,20 +878,24 @@ def __init__(
 version: int,
 spec_version: str,
 expires: datetime,
- meta: Dict[str, MetaFile],


can you explain this?

Yes. I noticed we don't do validation on Timestamp.meta.
According to the spec, the value of Timestamp.meta called "METAFILES" is:

METAFILES is the same as described for the snapshot.json file. In the case of the timestamp.json file, this MUST only include a description of the snapshot.json file.

That's why in order to include validation for meta no matter if we create objects with Timestamp.from_dict()
or with the constructor like Timestamp() I moved the meta validation and object creation inside __init__.

I don't agree with this change.

it's not in line with all other constructors that do take actual objects and not json-like dicts

It's going to make the API for creating a new Timestamp from scratch worse (caller feeds in json-like dicts instead of well defined objects).

It makes annotations useless: we should get rid of all Any that we can, not add more

I'm fine with you not doing this validation at all right now: we could just wait for the snapshot_meta discussion to reach a conclusion first

Now with the new discussion about snapshot_meta I agree that we shouldn't hurry and add validation.
Will remove the commit.

MVrachev · 2021-08-19T13:48:17Z

I had to rebase on top of develop.
Additionally I:

added a new commit with tests for those valid cases, but which we cannot verify in a trusted set.
squashed to similar commits together
updated my commits and pr descriptions
had another look through the spec to see if we are missing something and it seems everything is okay.

After a discussion with @jku about his words

I think you may be mixing "is this a syntactically valid metadata" with "can this metadata file be used successfully in every part of the update process".
I mean when you say things like:

Root role keyids are not allowed to be empty, because it's required that threshold should be at least 1

from the perspective of the file format and the containing metadata, keyids and threshold are not related. The spec does not say metadata files can't exist with a role that has less keys than the corresponding threshold. Yes, we happen to know that those keys then can't possibly verify that roles metadata when needed but the metadata that contains this role is still valid.

I realized he is right and standalone objects in those cases can exist and are valid, but cannot be verified.
That's why I created a new commit explaining that.

For Root roles specifically there is a spec mention (with a weird reference to "key list" but the intent seems clear: roles dictionary must contain these 4 or 5 items):
A role for each of "root", "snapshot", "timestamp", and "targets" MUST be specified in the key list. The role of "mirror" is OPTIONAL.
This we should test for in Root construction time I guess.

There is a separate issue for that #1516.

jku

LGTM (apart from the Timestamp constructor api change which I disagree with), thanks.

some of the targets test data are getting quite big ... but personally I still prefer parsing this sort of multiple lines of json in my head: at least every case is understandable without external context -- opinions may vary though

jku · 2021-08-20T08:48:59Z

tuf/api/metadata.py

@@ -878,20 +878,24 @@ def __init__(
 version: int,
 spec_version: str,
 expires: datetime,
- meta: Dict[str, MetaFile],


I don't agree with this change.

it's not in line with all other constructors that do take actual objects and not json-like dicts

It's going to make the API for creating a new Timestamp from scratch worse (caller feeds in json-like dicts instead of well defined objects).

It makes annotations useless: we should get rid of all Any that we can, not add more

I'm fine with you not doing this validation at all right now: we could just wait for the snapshot_meta discussion to reach a conclusion first

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>

Those tests are needed to cover use cases when syntatcticly as standalone objects the metadata classes and their helper classes defined in tuf/api/metadata.py are valid even if they cannot be verified. An example where an object is valid, but cannot be verified is if we have a Role instance with an empty list of "keyids". This instance is valid and can be created, but cannot be verified because there is a requirement that the threshold should be above 1, meaning that there should be at least 1 element inside the "keyids" list to complete successful threshold verification. The situation is the same for the rest of the tests I am adding to this commit: - Root object without keys - Root object without roles - DelegationRole object with empty "keyids" - DelegationRole object with an empty list of "paths" - DelegationRole object with an empty list of "path_hash_prefixes" all of these objects can be instantiated, but cannot complete successfully threshold verification. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev · 2021-08-20T14:15:03Z

@jku updated the pr by dropping the timestamp meta validation commit and removing multiple metafiles in timestamp test.

Move the Delegation class serialization tests from "test_api.py" to test_metadata_serialization.py module focused on serialization testing. Additionally, a test for empty keys and roles will be added in my upcomming pr theupdateframework#1511. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

jku

Thanks!

Move the Delegation class serialization tests from "test_api.py" to test_metadata_serialization.py module focused on serialization testing. Additionally, a test for empty keys and roles will be added in my upcomming pr theupdateframework#1511. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev force-pushed the test-containers branch from 458fe73 to 41c661e Compare July 28, 2021 13:32

jku reviewed Aug 5, 2021

View reviewed changes

tests/test_metadata_serialization.py Outdated Show resolved Hide resolved

tests/test_metadata_serialization.py Outdated Show resolved Hide resolved

tests/test_metadata_serialization.py Outdated Show resolved Hide resolved

MVrachev force-pushed the test-containers branch from 426d1ae to 033e069 Compare August 6, 2021 18:10

MVrachev force-pushed the test-containers branch from 033e069 to a894495 Compare August 7, 2021 18:58

jku reviewed Aug 17, 2021

View reviewed changes

MVrachev force-pushed the test-containers branch 2 times, most recently from 66ea522 to 294f0eb Compare August 19, 2021 13:42

jku requested changes Aug 20, 2021

View reviewed changes

MVrachev force-pushed the test-containers branch from 294f0eb to ae3a671 Compare August 20, 2021 14:09

MVrachev added 2 commits August 20, 2021 17:12

MVrachev force-pushed the test-containers branch from ae3a671 to 4c3fd95 Compare August 20, 2021 14:12

MVrachev requested a review from jku August 25, 2021 11:48

jku approved these changes Aug 25, 2021

View reviewed changes

jku merged commit 66aac38 into theupdateframework:develop Aug 25, 2021

MVrachev deleted the test-containers branch August 25, 2021 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new API: test containers for zero or more elements #1511

new API: test containers for zero or more elements #1511

MVrachev commented Jul 28, 2021 •

edited

Loading

jku commented Aug 5, 2021 •

edited

Loading

jku left a comment

MVrachev commented Aug 6, 2021

MVrachev commented Aug 7, 2021 •

edited

Loading

jku commented Aug 9, 2021 •

edited

Loading

jku commented Aug 9, 2021 •

edited

Loading

jku Aug 17, 2021

MVrachev Aug 19, 2021

jku Aug 20, 2021

MVrachev Aug 20, 2021

MVrachev commented Aug 19, 2021 •

edited

Loading

jku left a comment

jku Aug 20, 2021

MVrachev commented Aug 20, 2021

jku left a comment

new API: test containers for zero or more elements #1511

new API: test containers for zero or more elements #1511

Conversation

MVrachev commented Jul 28, 2021 • edited Loading

jku commented Aug 5, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Aug 6, 2021

MVrachev commented Aug 7, 2021 • edited Loading

jku commented Aug 9, 2021 • edited Loading

jku commented Aug 9, 2021 • edited Loading

jku Aug 17, 2021

Choose a reason for hiding this comment

MVrachev Aug 19, 2021

Choose a reason for hiding this comment

jku Aug 20, 2021

Choose a reason for hiding this comment

MVrachev Aug 20, 2021

Choose a reason for hiding this comment

MVrachev commented Aug 19, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

jku Aug 20, 2021

Choose a reason for hiding this comment

MVrachev commented Aug 20, 2021

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Jul 28, 2021 •

edited

Loading

jku commented Aug 5, 2021 •

edited

Loading

MVrachev commented Aug 7, 2021 •

edited

Loading

jku commented Aug 9, 2021 •

edited

Loading

jku commented Aug 9, 2021 •

edited

Loading

MVrachev commented Aug 19, 2021 •

edited

Loading