Metadata API: validate root role names by MVrachev · Pull Request #1630 · theupdateframework/python-tuf

MVrachev · 2021-10-22T18:23:24Z

Description of the changes being introduced by the pull request:

Validate that root role names are 4 and that they are exactly
"root", "snapshot", "targets" and "timestamp" as described in
the spec:
https://theupdateframework.github.io/specification/latest/#root-role

Additionally, fix the valid_roots dataset, so each of the cases contains
the top metadata role names inside the roles dictionary.

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

MVrachev · 2021-10-23T15:50:39Z

The CI failures are fixed in #1631.

coveralls · 2021-10-25T10:27:10Z

Pull Request Test Coverage Report for Build 1381179026

3 of 3(100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.002%) to 97.43%

Totals
Change from base Build 1380118579:	0.002%
Covered Lines:	3946
Relevant Lines:	4030

💛 - Coveralls

lukpueh

Nice patch, @MVrachev, and thanks for considering my comments from earlier! I have two more comments/questions. Other than that this lgtm.

lukpueh · 2021-10-25T11:32:16Z

+ roles = {role_name: Role([], 1) for role_name in TOP_LEVEL_ROLE_NAMES}
+ root = Root(1, SPEC_VER, self.safe_expiry, {}, roles, True)
+
+ for role in TOP_LEVEL_ROLE_NAMES:


Note that sets are unordered. If the original order, i.e. ["root", "timestamp", "snapshot", "targets"], needs to be guaranteed, you can't use TOP_LEVEL_ROLE_NAMES here.

Good note. I don't think the order matters here, am I right @jku?
Also, I decided to use a set, instead of a list for TOP_LEVEL_ROLE_NAMES because of this check:
if set(roles) != TOP_LEVEL_ROLE_NAMES: in tuf/api/metadata.py in Root.init() will result as false even if roles have the same content as TOP_LEVEL_ROLE_NAMES. The reason is that one is a list and the other is a set.

I think using a set makes sense. If it turns out that there are many use cases that require the same particular order of the top-level role names, we can still add another global that is a list, or make this one an ordered set.

lukpueh · 2021-10-25T11:40:29Z

 self.keys = keys
+ if set(roles) != TOP_LEVEL_ROLE_NAMES:
+ raise ValueError("Role names must be the top-level metadata roles")
+


It might be helpful to include the received, i.e. roles and expected, i.e. TOP_LEVEL_ROLE_NAMES value in the error message. I can't remember, do we have a general recommendation for error messages in that regard?

I'd avoid any complexity. We're preparing for one of two options:
deserialization: error message won't be useful
construction of new Root: this is code that we hopefully write once in a repository component and that's it
complex error message would have very limited value in both cases

This constant can be used across tuf without defining it each time. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Validate that root role names are 4 and that they are exactly "root", "snapshot", "targets" and "timestamp" as described in the spec: https://theupdateframework.github.io/specification/latest/#root-role Additionally, fix the valid_roots dataset, so each of the cases contains the top metadata role names inside the roles dictionary. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

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

lukpueh

LGTM!

lukpueh · 2021-10-26T07:41:23Z

+ roles = {role_name: Role([], 1) for role_name in TOP_LEVEL_ROLE_NAMES}
+ root = Root(1, SPEC_VER, self.safe_expiry, {}, roles, True)
+
+ for role in TOP_LEVEL_ROLE_NAMES:


I think using a set makes sense. If it turns out that there are many use cases that require the same particular order of the top-level role names, we can still add another global that is a list, or make this one an ordered set.

jku

LGTM

coveralls · 2024-12-23T15:35:11Z

Pull Request Test Coverage Report for Build 1380621788

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

2 of 2(100.0%) changed or added relevant lines in 1 file are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+1.1%) to 98.478%

Files with Coverage Reduction	New Missed Lines	%
tuf/api/metadata.py	6	97.97%

Totals
Change from base Build 1380118579:	1.1%
Covered Lines:	3785
Relevant Lines:	3811

💛 - Coveralls

MVrachev force-pushed the validate-role branch from 9cb7def to aafa75aCompare October 23, 2021 15:45

lukpueh reviewed Oct 25, 2021
View reviewed changes

Comment threadtuf/api/metadata.py Outdated

lukpueh reviewed Oct 25, 2021
View reviewed changes

Comment threadtuf/api/metadata.py Outdated

MVrachev force-pushed the validate-role branch from aafa75a to c43ef49Compare October 25, 2021 10:22

MVrachev requested a review from lukpueh October 25, 2021 10:23

lukpueh reviewed Oct 25, 2021
View reviewed changes

jku reviewed Oct 25, 2021
View reviewed changes

Comment threadtuf/api/metadata.py Outdated

jku reviewed Oct 25, 2021
View reviewed changes

Comment threadtests/test_metadata_serialization.py Outdated

Martin Vrachev added 3 commits October 25, 2021 15:32

Add TOP_LEVEL_ROLE_NAMES constant
59b5498
This constant can be used across tuf without defining it each time. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Use TOP_LEVEL_ROLE_NAMES across TUF
4158272
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev force-pushed the validate-role branch from c43ef49 to 4158272Compare October 25, 2021 13:04

MVrachev requested review from jku and lukpueh October 25, 2021 13:13

lukpueh approved these changes Oct 26, 2021
View reviewed changes

jku approved these changes Oct 26, 2021
View reviewed changes

joshuagl mentioned this pull request Oct 27, 2021
Metadata API: prevent Delegation role names to be one of top level metadata roles #1558
Closed

jku merged commit 1d115b5 into theupdateframework:developOct 27, 2021

MVrachev deleted the validate-role branch November 5, 2021 15:50

MVrachev mentioned this pull request Nov 17, 2021
Add input validation to simple metadata api #1140
Closed

lukpueh mentioned this pull request Dec 13, 2021
Prepare v0.20.0 #1723
Closed

Uh oh!

Conversation

MVrachev commented Oct 22, 2021

Uh oh!

MVrachev commented Oct 23, 2021

Uh oh!

Uh oh!

Uh oh!

coveralls commented Oct 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1381179026

💛 - Coveralls

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

lukpuehOct 25, 2021

Choose a reason for hiding this comment

Uh oh!

MVrachevOct 25, 2021

Choose a reason for hiding this comment

Uh oh!

lukpuehOct 26, 2021

Choose a reason for hiding this comment

Uh oh!

jkuOct 26, 2021

Choose a reason for hiding this comment

Uh oh!

lukpuehOct 25, 2021

Choose a reason for hiding this comment

Uh oh!

jkuOct 25, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

lukpuehOct 26, 2021

Choose a reason for hiding this comment

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 23, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1380621788

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Oct 25, 2021•
edited
Loading

jkuOct 25, 2021•
edited
Loading

coveralls commented Dec 23, 2024•
edited
Loading