Enforce role name uniqueness and add Targets key helpers #1537

MVrachev · 2021-08-24T15:02:11Z

Description of the changes being introduced by the pull request:

This pr contains three types of changes:

Enforce Delegation 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, because role names are the unique identifier of a role and
their order is important to the way they are traversed afterward.

Add missing key helpers in Delegations

Root class has the functionality to add and remove keys for delegated
metadata (add_key()/remove_key()) but the other delegator Targets does
not.
It should provide the same/similar functionality.

Some cleaning

Moves a couple of Delegations serialization tests inside test_metadata_serialization.py
Removed some unused imports.

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

tuf/api/metadata.py

jku · 2021-08-26T10:13:58Z

It seems logical to me that those functions will be part of the
"Delegations" class instead of "Targets" as those helper functions are
related to delegated roles.

This is true. The argument against is that

root and targets are both delegators with same key functionality... but now you'd access the functionality differently in those two
users of the library already have to learn about Metadata and Targets. If we push API top-level functionality into these helpers it means users have to also learn all the little helper classes. In some cases this is unavoidable (e.g. Key) but Delegations feels like an implementation detail of Targets that maybe no-one (except us) really needs to know about?

MVrachev · 2021-08-26T12:31:01Z

It seems logical to me that those functions will be part of the
"Delegations" class instead of "Targets" as those helper functions are
related to delegated roles.

This is true. The argument against is that

root and targets are both delegators with same key functionality... but now you'd access the functionality differently in those two

users of the library already have to learn about Metadata and Targets. If we push API top-level functionality into these helpers it means users have to also learn all the little helper classes. In some cases this is unavoidable (e.g. Key) but Delegations feels like an implementation detail of Targets that maybe no-one (except us) really needs to know about?

Hmm yes, you are right about that. We don't want to make our users know each of the helper classes.
Then, if the key helper functions are used in Targets, then at least shouldn't we use more clarifying names for them?
If you call Targets.add_key() then where are we adding this key? Yes, there is function documentation, but wonder if
names like add_delegated_key() and remove_delegated_key() (which are both a lot longer :( ) will be better?

tests/test_metadata_serialization.py

MVrachev · 2021-09-01T12:28:38Z

Inspired by #1553 commit "tests: Improve add_key/remove_key API tests" I added additional commit testing
the same thing as this commit does.
Also, I had to rebase.

jku

Then, if the key helper functions are used in Targets, then at least shouldn't we use more clarifying names for them?
If you call Targets.add_key() then where are we adding this key? Yes, there is function documentation, but wonder if
names like add_delegated_key() and remove_delegated_key() (which are both a lot longer :( ) will be better?

I don't think Targets.add_key() is a bad name. If the reader is confused about delegators and delegated roles we likely aren't going to fix that with function names: they could misread your suggestion the same way "oh Targets.add_delegated_key()! this is the function I use to add the key that delegates from root to targets"

tuf/api/metadata.py

MVrachev · 2021-09-02T09:38:47Z

Then, if the key helper functions are used in Targets, then at least shouldn't we use more clarifying names for them?
If you call Targets.add_key() then where are we adding this key? Yes, there is function documentation, but wonder if
names like add_delegated_key() and remove_delegated_key() (which are both a lot longer :( ) will be better?

I don't think Targets.add_key() is a bad name. If the reader is confused about delegators and delegated roles we likely aren't going to fix that with function names: they could misread your suggestion the same way "oh Targets.add_delegated_key()! this is the function I use to add the key that delegates from root to targets"

I can't say I am a fan of Targets.add_key() as an API name, but it seems there is no better option. I thought this name could be misleading for Targets compared to Root because in Root there is a top-level keyids dictionary and IMO this makes it clearer.
Will move the add_key API call to Targets.

coveralls · 2021-09-02T11:08:23Z

Pull Request Test Coverage Report for Build 1238606182

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

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

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

Totals
Change from base Build 1228471781:	0.01%
Covered Lines:	3809
Relevant Lines:	3846

💛 - Coveralls

MVrachev · 2021-09-02T11:36:36Z

I rebased on top of develop and had to resolve a couple of conflicts.
Additional changes I made are:

renamed the pr
moved the add_key and remove_key functions to Targets
added a check if delegations is None reminded by mypy that that is a possible case when calling the key helpers
created a new test function test_targets_key_api because the use cases became a lot

I was wondering what should we do if somebody calls targets.add_key() or targets.remove_key() and delegations is None.
It seemed logical to me that we can just ignore such calls.
What do the other think?

tests/test_api.py

tuf/api/metadata.py

MVrachev · 2021-09-03T11:13:44Z

We shouldn't silently ignore failures. I think an exception should be raised here. exceptions.py defines UnknownRoleError (as well as RoleAlreadyExistsError for the uniqueness check but a ValueError should be ok too).

Now as you said it makes sense to warn the user that nothing happened when calling add_key or remove_key.
Added a UnknownRole exception there.
Additionally, I swapped the ValueError exception with RoleAlreadyExistsError which seems more concrete for that case.

sechkova · 2021-09-03T12:11:52Z

Additionally, I swapped the ValueError exception with RoleAlreadyExistsError which seems more concrete for that case.

I'm afraid someone may have a different opinion on this one :D @jku

tuf/api/metadata.py

jku · 2021-09-08T08:41:18Z

I think in the future I'd suggest not combining issues like these two: If this had been two separate PRs it would have meant a bit more work for you (to rebase the second PR and to figure out when to put the second PR to review) but it would have meant reviewers need less context, can do faster reviews and the first PR would land faster.

jku · 2021-09-08T14:08:58Z

tuf/api/metadata.py

+ raise exceptions.RoleAlreadyExistsError(
+ f"Delegated role names must be unique! Got {new_role.name}"
+ )


This is code that gets called from serialization (which wraps it in a catch all try-except) so using custom exceptions seems not useful.

repository code that wants to add roles is now going to use targets.delegation.roles[role.name] = role so the exception won't be useful in that case either, as far as I can see...

We might want to provide some ways to easily order the dict at some point... but even then I don't see when a custom error would be useful

Okay, will change the exception to ValueError and will shorten the message.

MVrachev · 2021-09-13T14:18:06Z

I think in the future I'd suggest not combining issues like these two: If this had been two separate PRs it would have meant a bit more work for you (to rebase the second PR and to figure out when to put the second PR to review) but it would have meant reviewers need less context, can do faster reviews and the first PR would land faster.

Yes, I guess you are right. I thought I can combine fixing those two issues and that you will have the context given that
you suggested that here #1469 (comment) and both of the issues are submitted by you.

tuf/api/metadata.py

sechkova · 2021-09-15T14:14:39Z

tuf/api/metadata.py

+ key store.
+
+ Raises:
+ ValueError: If there are no delegated roles or if 'role' is not


How did we end up with two equivalent functions throwing ValueError and KeyError for the same thing. I am not sure about this.

Now as you highlighted it, it's not great we throw two different types of exceptions for Root.remove_key() - KeyError and Targets.remove_key() - ValueError.
IMO the correct exception here should be ValueError as the problems are in the given values -role doesn't exist or keyid doesn't point to a key used by that role.
@jku and @sechkova if you both agree I can change the Targets.remove_key() exception to be ValueError as well.

(EDIT: this is a response to your comment, see context in the discussion)

I suppose you mean change Targets.add_key() exception: sounds good to me. ValueError is no doubt the correct error (but it mostly matters for aesthetics: this is a programmer error not an error that will be handled)

btw: you don't need to say "If there are no delegated roles" in the docstring: that is an implementation detail the caller does not need to know: for them the error is "'role' is not delegated by this Targets"

I made an API change to Root.add_key() and Root.remove_key() so that they throw ValueError instead of KeyError on wrong given arguments.
The reason for this change is that ValueError in my opinion is the right exception that should be thrown when the value is not correct and I think it's important to be consistent when calling delegator.add_key or delegator.remove_keyno matter if the delegator isRootorTargets`.

jku

Looks good, left some style comments: probably none of them absolutely necessary.

tuf/api/metadata.py

jku · 2021-09-16T09:19:20Z

tuf/api/metadata.py

+ key store.
+
+ Raises:
+ ValueError: If there are no delegated roles or if 'role' is not


(EDIT: this is a response to your comment, see context in the discussion)

I suppose you mean change Targets.add_key() exception: sounds good to me. ValueError is no doubt the correct error (but it mostly matters for aesthetics: this is a programmer error not an error that will be handled)

btw: you don't need to say "If there are no delegated roles" in the docstring: that is an implementation detail the caller does not need to know: for them the error is "'role' is not delegated by this Targets"

tuf/api/metadata.py

jku

Looks ok, still have two comments on the errors though.

I'm fine with the API change (KeyError -> ValueError) even if I usually would be against changing API with no practical benefit: the errors in question are IMO clear programming mistakes so we don't expect there to exist code that might break if we change the errors, but more consistent errors should be useful to developers making the programming mistake.

tuf/api/metadata.py

jku · 2021-09-20T10:29:38Z

If you can rebase as well that would be great (the workflow jobs changed name and branch protection will now think you haven't passed the tests)

jku

I think this is a correct change, thanks.

Commit messages do not make clear that the Delegations API has a breaking change, we should get better at this as we can't expect the release notes writer to always make the connection... Anyway LGTM: we can mention API change in the merge commit maybe?

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 <mvrachev@vmware.com>

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>

We can remove the conditional imports from tests as now we support python versions 3.6+. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Root class has the functionality to add and remove keys for delegated metadata (add_key()/remove_key()) but the other delegator Targets does not. It should provide the same/similar functionality. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

This is an API change to the exceptions thrown in Root.add_key() and Root.remove_key(). The reason for that change is that in my opinion the correct exceptions in these cases should be "ValueError" instead of "KeyError" as the problems are in the given values - role doesn't exist or key is not used by a particular role. Additionally, document the thrown exceptions in "Root.add_key" and add a test which invokes that exception. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev · 2021-09-21T09:23:06Z

I think this is a correct change, thanks.

Commit messages do not make clear that the Delegations API has a breaking change, we should get better at this as we can't expect the release notes writer to always make the connection... Anyway LGTM: we can mention API change in the merge commit maybe?

Now it should be clearer and I am not sure what will be the value in adding a note about the API changes anymore.

MVrachev mentioned this pull request Aug 24, 2021

Metadata API: Missing key helper functions in Targets/Delegations #1469

Closed

MVrachev force-pushed the role-name-uniqueness branch from 6709675 to 355c3e1 Compare August 24, 2021 15:07

jku reviewed Aug 24, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the role-name-uniqueness branch from 355c3e1 to 90e4e5b Compare August 25, 2021 09:49

jku reviewed Aug 26, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the role-name-uniqueness branch from 90e4e5b to ba09c66 Compare August 26, 2021 17:22

MVrachev requested a review from jku August 26, 2021 17:22

sechkova reviewed Aug 27, 2021

View reviewed changes

tests/test_metadata_serialization.py Show resolved Hide resolved

MVrachev force-pushed the role-name-uniqueness branch from ba09c66 to cd466cf Compare August 27, 2021 12:56

MVrachev force-pushed the role-name-uniqueness branch from b760b07 to c07ab06 Compare September 1, 2021 12:38

jku reviewed Sep 2, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the role-name-uniqueness branch from c07ab06 to c37a63d Compare September 2, 2021 11:03

MVrachev force-pushed the role-name-uniqueness branch from c37a63d to b413b16 Compare September 2, 2021 11:31

MVrachev changed the title ~~Enforce role name uniqueness and add Delegations key helpers~~ Enforce role name uniqueness and add Targets key helpers Sep 2, 2021

sechkova reviewed Sep 3, 2021

View reviewed changes

tests/test_api.py Show resolved Hide resolved

sechkova reviewed Sep 3, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

sechkova reviewed Sep 3, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the role-name-uniqueness branch 3 times, most recently from fb22643 to 886c5d3 Compare September 3, 2021 11:11

jku requested changes Sep 7, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

jku linked an issue Sep 8, 2021 that may be closed by this pull request

Metadata API: Missing key helper functions in Targets/Delegations #1469

Closed

jku reviewed Sep 8, 2021

View reviewed changes

MVrachev force-pushed the role-name-uniqueness branch from 886c5d3 to 9f5327c Compare September 14, 2021 16:05

MVrachev requested a review from jku September 14, 2021 16:05

sechkova reviewed Sep 15, 2021

View reviewed changes

MVrachev force-pushed the role-name-uniqueness branch from 9f5327c to 496ab2b Compare September 15, 2021 17:24

jku reviewed Sep 16, 2021

View reviewed changes

MVrachev force-pushed the role-name-uniqueness branch 2 times, most recently from 7ba9dd4 to a25cab9 Compare September 16, 2021 12:59

MVrachev requested review from sechkova and jku September 16, 2021 13:03

jku requested changes Sep 20, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the role-name-uniqueness branch 2 times, most recently from 85fb461 to 0e7dac3 Compare September 20, 2021 11:59

MVrachev requested a review from jku September 20, 2021 12:15

MVrachev force-pushed the role-name-uniqueness branch from 0e7dac3 to 07a18ba Compare September 20, 2021 16:06

jku approved these changes Sep 21, 2021

View reviewed changes

MVrachev added 5 commits September 21, 2021 12:15

Remove some unused imports

1a5912a

We can remove the conditional imports from tests as now we support python versions 3.6+. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev force-pushed the role-name-uniqueness branch from 07a18ba to f00295f Compare September 21, 2021 09:20

MVrachev requested a review from jku September 21, 2021 09:23

jku merged commit cc1f95e into theupdateframework:develop Sep 22, 2021

MVrachev deleted the role-name-uniqueness branch October 13, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce role name uniqueness and add Targets key helpers #1537

Enforce role name uniqueness and add Targets key helpers #1537

MVrachev commented Aug 24, 2021 •

edited

Loading

jku commented Aug 26, 2021

MVrachev commented Aug 26, 2021

MVrachev commented Sep 1, 2021 •

edited

Loading

jku left a comment

MVrachev commented Sep 2, 2021 •

edited

Loading

coveralls commented Sep 2, 2021 •

edited

Loading

MVrachev commented Sep 2, 2021 •

edited

Loading

MVrachev commented Sep 3, 2021

sechkova commented Sep 3, 2021

jku commented Sep 8, 2021

jku Sep 8, 2021 •

edited

Loading

MVrachev Sep 14, 2021

MVrachev commented Sep 13, 2021

sechkova Sep 15, 2021

MVrachev Sep 15, 2021

jku Sep 16, 2021 •

edited

Loading

MVrachev Sep 16, 2021

jku left a comment

jku Sep 16, 2021 •

edited

Loading

jku left a comment

jku commented Sep 20, 2021

jku left a comment

MVrachev commented Sep 21, 2021

Enforce role name uniqueness and add Targets key helpers #1537

Enforce role name uniqueness and add Targets key helpers #1537

Conversation

MVrachev commented Aug 24, 2021 • edited Loading

jku commented Aug 26, 2021

MVrachev commented Aug 26, 2021

MVrachev commented Sep 1, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Sep 2, 2021 • edited Loading

coveralls commented Sep 2, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1238606182

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

MVrachev commented Sep 2, 2021 • edited Loading

MVrachev commented Sep 3, 2021

sechkova commented Sep 3, 2021

jku commented Sep 8, 2021

jku Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

MVrachev Sep 14, 2021

Choose a reason for hiding this comment

MVrachev commented Sep 13, 2021

sechkova Sep 15, 2021

Choose a reason for hiding this comment

MVrachev Sep 15, 2021

Choose a reason for hiding this comment

jku Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

MVrachev Sep 16, 2021

Choose a reason for hiding this comment

jku left a comment

Choose a reason for hiding this comment

jku Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

jku left a comment

Choose a reason for hiding this comment

jku commented Sep 20, 2021

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Sep 21, 2021

MVrachev commented Aug 24, 2021 •

edited

Loading

MVrachev commented Sep 1, 2021 •

edited

Loading

MVrachev commented Sep 2, 2021 •

edited

Loading

coveralls commented Sep 2, 2021 •

edited

Loading

MVrachev commented Sep 2, 2021 •

edited

Loading

jku Sep 8, 2021 •

edited

Loading

jku Sep 16, 2021 •

edited

Loading

jku Sep 16, 2021 •

edited

Loading