Add support for succinct roles (TAP 15)

[MVrachev]

Related to the previous pr: #1948 and issue #1909

This pr includes 11 breaking changes:

In Delegations class from tuf/api/metadata.py :

1. roles argument is made optional
2. unrecognized_fields argument becomes the 4-th rather than the 3-rd
   as it used to be

In Root class from tuf/api/metadata.py:

1. The "role" and "key" arguments in "Root.add_key()" are in reverse
   order - "key" becomes first and "role" second.
2. "Root.remove_key()" has been renamed to "Root.remove_key()"
3. The "role" and "keyid" arguments in "Root.revoke_key()" are in
   reverse order - "keyid" becomes first and "role" second.

In Targets class from tuf/api/metadata.py:

1. The "role" and "key" arguments in "Targets.add_key()" are in reverse
   order - "key" becomes first and "role" second
2. "Targets.remove_key()" has been renamed to "Targets.revoke_key()"
3. The "role" and "keyid" arguments in "Targets.revoke_key()" are in
   reverse order - "keyid" becomes first and "role" second.
4. In both methods "Targets.add_key()" and "Targets.revoke_key()" the
   "role" argument becomes an optional with a default value of None

Those changes are made in an effort to make those two methods logical
for both cases when standard roles and succinct_roles are used.

Description of the changes being introduced by the pull request:

TAP 15 was created on June 23-rd 2020 and was last modified on July 6-th 2020.
Since then the TAP has been put to draft status meaning it needs a prototype implementation before it's accepted as a future specification change.
Given that this TAP underlines an efficient way of handling succinct hash bin delegations, meaning it would be really useful when python-tuf is integrated into Warehouse, it's logical that we should not only create a prototype but directly work to integrate it in python-tuf.

The outcome of the TAP 15 implementation is

1. Support reading, updating, and saving target metadata files that are using succinct_roles.
2. Support in ngclient for calculating the delegated role name on a given target path.
3. An exemplary document inside examples/ showcasing how succinct_roles can be utilized in practice.

This pr covers points 1 and 2.
Point 3 will be done in another pr.

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]

I know the code is a lot, but a big chunk of it is testing.

[coveralls]

Pull Request Test Coverage Report for Build 2514955908

• 135 of 137 (98.54%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.3%) to 98.012%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tuf/api/metadata.py	126	128	98.44%

Totals
Change from base Build 2507249080:	-0.3%
Covered Lines:	1295
Relevant Lines:	1313

---

💛 - Coveralls

[jku]

Overall this looks like a good direction I think and code does not seem to have issues (bugs). I have left some individual code comments (no bugs or anything, just opinions). These are the concerns:

• this adds a large amount of new API, 10 new methods or something like that: let's focus on whether all of that is actually used and needed, whether it needs to be public API and whether there are other options to adding the API (ie the add_key() case). I've left some comments
• this is also an API change: I think it's one we want to do but let's take changes seriously

I've not checked if the test coverage is "complete": let's figure out the API size first.

[jku]

Documenting some possible options WRT add_key() and remove_key():

• as in PR: add new methods for succinct case (so users need to remember to call different Targets method depending on what the value of Targets.delegations.succinct_delegations is)
• just document that role argument to the *_key() functions is unused whenever succinct delegations are used and make the existing functions work in succinct case
• Modify *_key() API so that instead of self, role: str, key: Key the arguments are self, key: Key, role: Optional[str] = None

[lukpueh]

This is really great, @MVrachev. I left a few comments inline, but nothing big a deal.

I also have a few more naming suggestions for SuccinctRoles methods, to consolidate with the rest of the API...

get_bin_name --> get_role(_for_index)
get_all_bin_names --> get_roles
find_bin --> get_role_for_target
is_bin --> is delegated_role

... and for Delegations:

get_all_delegations -> get_roles_for_target

What do you think?

[MVrachev]

get_bin_name --> get_role(_for_index)
get_all_bin_names --> get_roles
find_bin --> get_role_for_target
is_bin --> is delegated_role

I understand where you come from and why those changes make sense for you.
I am worried about using the word role inside the SuccinctRole method names and especially the phrase delegated role because I don't want to confuse our users with Delegations.roles or the DelegatedRole class instances.
That's why I opt-in for a more concrete naming scheme including bin instead of role.
Also, after your docstring suggestion, it seems a lot clearer what a bin actually is.

After I update the pr with your docstring suggestions would you have a look again and tell me if you think those changes are still worth it?

[MVrachev]

I have addressed all of your comments @jku and @lukpueh.
For Targets.*_key() methods I decided to change the order of the arguments and make role an optional.
One downside in this (besides it's a breaking change) is that those methods no longer have the same signature as Root.*_key() methods.

Also, I have mentioned in the commit messages about the breaking API changes and also updated the pr description with information about them.

The pr is smaller by a big margin and it's ready for another review.

[lukpueh]

get_bin_name --> get_role(_for_index)
get_all_bin_names --> get_roles
find_bin --> get_role_for_target
is_bin --> is delegated_role

I understand where you come from and why those changes make sense for you. I am worried about using the word role inside the SuccinctRole method names and especially the phrase delegated role because I don't want to confuse our users with Delegations.roles or the DelegatedRole class instances. That's why I opt-in for a more concrete naming scheme including bin instead of role. Also, after your docstring suggestion, it seems a lot clearer what a bin actually is.

After I update the pr with your docstring suggestions would you have a look again and tell me if you think those changes are still worth it?

My idea was to point out exactly that connection, i.e. that a bin is a delegated targets role. But I won't insist. I'm fine with the names you picked as well.

What about my other suggestion get_all_delegations -> get_roles_for_target? The idea behind this one is that the function does not really return delegations but delegated roles (or actually their names, and their terminating property)

I'm a bit pedantic about this, because roles and delegations are confused so often.

[MVrachev]

My idea was to point out exactly that connection, i.e. that a bin is a delegated targets role. But I won't insist. I'm fine with the names you picked as well.

What about my other suggestion get_all_delegations -> get_roles_for_target? The idea behind this one is that the function does not really return delegations but delegated roles (or actually their names, and their terminating property)

I'm a bit pedantic about this, because roles and delegations are confused so often.

Hmm... more and more when I think about it probably you have some point here.
I keep considering the SuccinctRoles instances as one delegated role which is not what is implied even from the class title... SuccinctRoles instance just represents a group of delegated roles instead of using a dictionary. Probably mentally it's easier to think SuccinctRoles is just one role as keyids and threshold is shared through all delegated roles represented by that instance.
Probably it will be useful in using the same delegated role phrase for SuccinctRoles roles as well instead of promoting a new bin term.

Will rename the methods as you have suggested here:

get_all_bin_names --> get_roles
find_bin --> get_role_for_target
is_bin --> is delegated_role

About get_all_delegations -> get_roles_for_target.
Yea, I missed it honestly as there were so many renames. I agree with that suggestion. Will do it.

[MVrachev]

I followed @lukpueh advice and did the following renames:

• get_all_bin_names --> get_roles
• find_bin --> get_role_for_target
• is_bin --> is delegated_role
• get_all_delegations -> get_roles_for_target

EDIT: I forced pushed once more to fix the commit message mentioning the old methods name.

[jku]

Looks good. The only thing that sticks out to me is the key API.

- def add_key(self, role: str, key: Key) -> None:
+ def add_key(self, key: Key, role: Optional[str] = None) -> None:

this looks ok to me -- it's easy to notice with static checks and we could even add a specific error message if isinstance(role, Key):...

- def remove_key(self, role: str, keyid: str) -> None:
+ def remove_key(self, keyid: str, role: Optional[str] = None) -> None:

... but I hadn't thought about this one all the way through: both argument types are str here. It really is the most annoying kind of API change, very easy to mess up.

[MVrachev]

Looks good. The only thing that sticks out to me is the key API.

- def add_key(self, role: str, key: Key) -> None:
+ def add_key(self, key: Key, role: Optional[str] = None) -> None:

this looks ok to me -- it's easy to notice with static checks and we could even add a specific error message if isinstance(role, Key):...

Maybe this could be useful. Preventing users from mistaking the arguments order.

- def remove_key(self, role: str, keyid: str) -> None:
+ def remove_key(self, keyid: str, role: Optional[str] = None) -> None:

... but I hadn't thought about this one all the way through: both argument types are str here. It really is the most annoying kind of API change, very easy to mess up.

Yea... if only the types were different. I was thinking if we can use the insider knowledge we have about keyid.
For example that it will have a length longer than X number of symbols, but then what stops a user from having very long delegated role names? Nothing...

I hope we can make this API change as still there aren't so many users, especially in the 1.0.0 version of python-tuf and calls like targets.add_key(None, key_obj) and ``targets.add_key(None, keyid)` are not really nice...

[jku]

I was thinking if we can use the insider knowledge we have about keyid.

I don't think we have any insider knowledge about it -- theoretically it's a hash but

• we've explicitly avoided using that in python-tuf as it seems like a mistake: it should be just a unique id
• even if we know keyid is a hash, nothing prevents rolenames from also being hashes (in fact I could imagine it would be a good way to handle rolenames in some cases)

[MVrachev]

@lukpueh any thoughts?

[jku]

I think there's still several possibilities here that wouldn't require surprisingly breaking applications:

• keep current API (maybe allow None for rolename), just document it well enough. I think this is completely within reason. It's a wart in the API but that happens.
• @overload magic so that we could have both remove_key(rolename, keyid) and remove_key(keyid) -- the code will not be very pretty but should work.
• just rename the API: remove_key() stays as is for a few releases and we have a new method with different API -- the lovely linux naming tradition would give us remove_key2() 😬
• probably something else still?

[lukpueh]

I think there's still several possibilities here that wouldn't require surprisingly breaking applications:

• keep current API (maybe allow None for rolename), just document it well enough. I think this is completely within reason. It's a wart in the API but that happens.

This would be an okay solution.

• @overload magic so that we could have both remove_key(rolename, keyid) and remove_key(keyid) -- the code will not be very pretty but should work.

I'm not sure about this one. It is definitely unidiomatic (~> surprising). And the code to do this would likely stain the otherwise elegant metadata module.

• just rename the API: remove_key() stays as is for a few releases and we have a new method with different API -- the lovely linux naming tradition would give us remove_key2() 😬

I like this suggestion best, at least the first part. But I'd rather not have a remove_key2 in our API. And I also don't think we need to support the old method for a few releases, if we make this release a breaking one.

So what about replacing remove_key right away with a name that also makes sense and that we like. Some ideas: revoke_key, deauthorize, unauthorize, something with delegation in its name, etc...

Some consistency considerations:

• Will the new name work as counterpart to add_key or do we need to rename that too?
• What about Root's {add, remove}_key methods. Should we adopt a rename there too? And the argument order? This would make it a bigger cut, but API consistency seems important.

• probably something else still?

Couldn't think of anything else.

[jku]

Yes, agreed that providing the old deprecated method isn't terribly important if using the new method is basically a copy-paste fix

[lukpueh]

Great tests, @MVrachev. I just reviewed:

docs/api/tuf.api.metadata.supporting.rst
docs/repository-library-design.md
examples/repo_example/hashed_bin_delegation.py
tests/test_api.py
tests/test_metadata_eq_.py

I'll review the rest tomorrow.

[MVrachev]

I think revoke_key and the counterpart add_key sounds okay.

If we decide to do a rename to remove_key then does this mean that we are okay with its counterpart - add_key and its changes after I add a check from the type if isinstance(role, Key) as Jussi has suggested preventing mistakes?

[MVrachev]

Yes, you are right that probably some summary of my changes will be good.
All of them were addressing Lukas comments, so those comments can give you some idea what I changed + I renamed the Root/Targets.remove_key() to Root/Targets.revoke_key() as we had discussed.

What I did ca be summarized like this:

1. Removed the zero_padding test in favor of a more complicated test_get_roles_in_succinct_roles test.
2. Moved a comment about bin names inside test_is_delegated_role_in_succinct_roles a few lines below.
3. Updated the test about __eq__ implementation for SuccinctRoles after the merge of Tests: simplify and shorten test_metadata_eq_.py #2017.
4. Some typo fixes.
5. Renamed RepositorySimulator. add_succinct_roles_to_delegations() to RepositorySimulator.add_succinct_roles() and added a better docstring.
6. Added a helper RepositorySimulator._get_delegator() which removes code dublication in add_target, add_delegation and add_succinct_roles in RepositorySimulator.
7. Updated test_succinct_roles_graph_traversal with many suggestions by @lukas from this comment Add support for succinct roles (TAP 15) #2010 (comment)
8. Renamed Root/Targets.remove_key() to Root/Targets.revoke_key() plus updated the pr description and commit description mentioning those API changes.

[MVrachev]

I have updated the pr by making Root.*_key() API methods with consistent naming and argument order as the ones in Targets.*_key().

[jku]

I think I'm happy with this -- left some nitpicks but they're not critical. The main question I have is documentation:

• The documentation currently does not make it clear that this feature is not a TUF specification feature: maybe it should?

As TODO items (that might exist already):

• we should improve the succinct delegation docs and update the example script -- this is something we should do before we release this
• for completeness, actual download tests should contain succinct delegations: it may make sense to refactor the download tests at that point

[MVrachev]

Updated the pr with the following changes:

• added a note that SuccinctRoles is not supported by the spec
• addressed all other changes mentioned by @jku

[MVrachev]

Accepted @jku suggested how to document that succinct_roles is not in the spec yet and moved that documentation next to the succinct_roles argument description in Delegations.

[jku]

Yes, looks good to me. Thanks everyone! The rough plan here is:

• Merge this PR: succinct delegations are then usable on "develop" branch
• Get any remaining polish done on TAP-15 and move it to "accepted" state Move TAP 15 to Accepted taps#148
• release python-tuf with TAP-15 support (likely means v2.0.0)
• Maybe TAP-15 gets integrated into spec at some point?

@lukpueh, would you like to do a final review of the PR as the system thinks your review is pending? Checking "merge without waiting for requirements to be met" makes me feel criminal...

@MVrachev would you mind having a last look to identify any issues we may need to file for future work?

• Updating the example code is one (if it does not exist already)
• Maybe also one for better parameterised target download tests that use succinct bins and normal bins -- there is a PR for the latter but I think we can do better

[MVrachev]

@MVrachev would you mind having a last look to identify any issues we may need to file for future work?

• Updating the example code is one (if it does not exist already)
• Maybe also one for better parameterised target download tests that use succinct bins and normal bins -- there is a PR for the latter but I think we can do better

We have an issue #1909 where we keep track of that stuff.
Only three items are left on the list and I am not sure there is a need for separate issues.

[lukpueh]

LGTM! 💯