Adopt TAP 3 multi-role delegations metadata format #57

lukpueh · 2019-10-30T16:35:57Z

Update section "4.5. File formats: targets.json and delegated target roles" to reflect metadata format introduced by TAP 3.

Note that TAP 3 is not implemented in the reference implementation, which claims to be spec 1.0.0 conformant!

lukpueh

Here are a few things I noted while adopting TAP3 metadata format. I'd appreciate thoughts (cc @JustinCappos, @mnm678, @SantiagoTorres).

lukpueh · 2019-10-30T16:36:40Z

tuf-spec.md

@@ -790,7 +790,10 @@ repo](https://github.com/theupdateframework/specification/issues).
         "version" : VERSION,
         "expires" : EXPIRES,
         "targets" : TARGETS,
-         ("delegations" : DELEGATIONS)
+         ("keys_for_delegations" : {


Wouldn't it be enough to just call this keys?

IMHO this field should be under delegations, but then delegations would have to remain a dictionary and we'd have to find another name for the list that contains all the delegations...

I think we should keep the keys separate for clarity.

Okay, keeping it separate works for me. And what about my suggestion to call it keys only? Because the keys field in root.json in reality also lists "keys for delegations", i.e. keys to delegate trust to other top-level roles, but is only called keys.

Calling it keys to match root.json makes sense.

lukpueh · 2019-10-30T16:37:59Z

tuf-spec.md

+          "paths" : [ PATHPATTERN, ... ]),
+          "terminating": TERMINATING,
+          "min_roles_in_agreement" : NUM_ROLES,
+          "roleinfo": [{


Prior to TAP 3 delegated roles were listed in the roles field. Should we keep that name?

I like roles, it makes it clear that there can be multiple roles

lukpueh · 2019-10-30T16:45:36Z

tuf-spec.md

+          ("path_hash_prefixes" : [ HEX_DIGEST, ... ] |
+          "paths" : [ PATHPATTERN, ... ]),
+          "terminating": TERMINATING,
+          "min_roles_in_agreement" : NUM_ROLES,


This was probably named min_roles_in_agreement to distinguish it from the signature threshold field. But I find it odd that one threshold field has such a specific name and the other one is generic.

Maybe the scope (delegation vs. role) is disambiguation enough and we can both call threshold?

maybe delegation_threshold?

The field is a property of the delegation object. Addressing it as delegation.delegation_threshold seems like unnecessary redundancy.

I agree. Let's just call it threshold.

That was the reasoning, yes (distinguishing it from the other threshold). If you think it'll not be confused, then it's nice to set it to something simple.

Thinking about it some more, it does make sense to distinguish it from the other threshold. Ideally, we'd have equally specific names for both, e.g. role.min_keys_in_agreement and delegation.min_roles_in_agreement or role.signature_threshold and delegation.role_threshold.

Is renaming the existing threshold field an option? Is it worthwhile? This ~~will be~~ is a breaking change anyway. :)

Until then I'll just leave it as threshold and min_roles_in_agreement.

lukpueh · 2019-10-30T16:52:05Z

tuf-spec.md

+        {
+          "name": DELEGATION_NAME,
+          ("path_hash_prefixes" : [ HEX_DIGEST, ... ] |
+          "paths" : [ PATHPATTERN, ... ]),


TAP 3 only mentions paths, but I guess we still want to support path_hash_prefixes?

yes, especially for PEP 458

lukpueh · 2019-11-05T14:30:27Z

Interestingly, the client workflow had already mentioned multi-role delegation (prior to this PR). b46dc5e adds some clarification about the role threshold (min_roles_in_agreement).

trishankatdatadog · 2019-11-05T14:48:22Z

Do we consider backwards-compatibility? If not, why not?

mnm678 · 2019-11-05T19:14:38Z

tuf-spec.md

@@ -790,7 +790,10 @@ repo](https://github.com/theupdateframework/specification/issues).
         "version" : VERSION,
         "expires" : EXPIRES,
         "targets" : TARGETS,
-         ("delegations" : DELEGATIONS)
+         ("keys_for_delegations" : {


I think we should keep the keys separate for clarity.

mnm678 · 2019-11-05T19:15:29Z

tuf-spec.md

+          ("path_hash_prefixes" : [ HEX_DIGEST, ... ] |
+          "paths" : [ PATHPATTERN, ... ]),
+          "terminating": TERMINATING,
+          "min_roles_in_agreement" : NUM_ROLES,


maybe delegation_threshold?

mnm678 · 2019-11-05T19:16:55Z

tuf-spec.md

+          "paths" : [ PATHPATTERN, ... ]),
+          "terminating": TERMINATING,
+          "min_roles_in_agreement" : NUM_ROLES,
+          "roleinfo": [{


I like roles, it makes it clear that there can be multiple roles

mnm678 · 2019-11-05T19:19:45Z

tuf-spec.md

-      recursively visit each role, and check that each has signed exactly the
-      same non-custom metadata (i.e., length and hashes) about the target (or
-      the lack of any such metadata).
+      recursively visit each role, and check that a defined threshold of


where does this threshold come from?

min_roles_in_agreement

mnm678 · 2019-11-05T19:23:19Z

Do we consider backwards-compatibility? If not, why not?

If yes, we would need to change the spec to version 2.0.0. However, we have previously stated that TAP 3 is part of TUF 1.0.0. I think for now we should include this change in 1.0.0.

lukpueh · 2019-11-06T10:40:08Z

Do we consider backwards-compatibility? If not, why not?

This PR is not the right place to ask these questions. This PR only restores consistency with backwards incompatible changes that have already been made. That is, claiming that the spec supports TAP 3 and TAP 4 (see #51). If 1.0.0 of the spec should not break backwards compatibility, then we must revert those claims.

But I agree with @mnm678 that we should get this (and TAP 4) into the spec now, and then make a formal 1.0.0 release. 1.0.0 will also help us to lead a more structured discussion about backwards compatibility (#33 has not yet been resolved!).

Independently, and I think that's the operational concern here (@trishankatdatadog?), we can discuss whether python-tuf should remain backwards compatible or not.

trishankatdatadog · 2019-11-06T16:37:13Z

@mnm678 @lukpueh Understood. I wasn't aware that the 1.0.0 spec had made these claims about the TAPs, and that the client has not matched these claims yet. Good observation, and let's discuss in our meeting next week.

lukpueh · 2019-11-07T16:21:07Z

I just pushed some renames (3c3b687) as discussed above. This discussion about min_roles_in_agreement is still open (see #57 (comment)).

Also, should we propagate the renames to the actual TAP 3 document?

lukpueh · 2019-11-07T16:28:36Z

Maybe now is also a good time to fix #19 and #32?

This extends the spec to clarify that when searching for the key that signed a delegated role, that key should only be found in the delegating target, and not any other role in the delegation chain. This incorporates language suggested by lukpueh in theupdateframework#58, and the definition of the keys field from [TAP 3](theupdateframework#57). Closes theupdateframework#58

The specification claims that it adheres to TAP 3 and TAP 4, which is not the case. TAP 3 requires changes to the existing metadata format (prepared in theupdateframework#57) and TAP 4 adds a new metadata file (map.json), which is not yet mentioned in the specification (issue described in theupdateframework#88). As agreed in the TUF community meeting on Dec 10, 2019, we remove TAP 3 and TAP 4 (adherence claims), for a formal 1.0.0 release and add TAP 4 in version 1.1.x and TAP 3 in version 2.x.x. (see meeting notes about "Versioning" https://groups.google.com/forum/#!msg/theupdateframework/oPTliXibT_4/0tJ_POLGAwAJ)

lukpueh · 2020-03-03T11:18:39Z

As discussed in the TUF community meeting on Dec 10, 2019 the changes of this PR require a major bump of the spec version (see meeting notes about "Versioning" ).

Update "4.5. File formats: targets.json and delegated target roles" to reflect metadata format introduced by TAP 3

Clarify in step 4.5.2.1 of the client workflow that in a multi-role delegation not each but rather a defined threshold of roles must agree on the target hashes and lengths (see `min_roles_in_agreement`).

`keys_for_delegations` --> `keys` (the keys field in root.json in reality also lists "keys for delegations", i.e. keys to delegate trust to other top-level roles, but is only called keys) `roleinfo` --> `roles` (keeping the name for delegated roles as it was before TAP3)

lukpueh · 2020-03-05T12:31:27Z

Note: We will merge this PR into draft, because it is a major-type change of the spec. Bumping version number and last modified date will be done when PRing draft into master.

mnm678

I think this looks good as the first pr for version 2.0.0. Could someone with write access merge this into draft (and possibly give me write access)? @trishankatdatadog @JustinCappos

lukpueh · 2020-05-27T15:07:00Z

Could someone with write access merge this into draft (and possibly give me write access)?
Done.

Thanks for the review!

lukpueh commented Oct 30, 2019

View reviewed changes

lukpueh requested a review from mnm678 October 30, 2019 16:54

mnm678 previously approved these changes Nov 5, 2019

View reviewed changes

lukpueh force-pushed the adopt-tap3-metadata branch from 3c3b687 to 558ab11 Compare November 8, 2019 09:56

lukpueh mentioned this pull request Feb 4, 2020

Add specification versioning- and release management instructions and checks #87

Merged

lukpueh mentioned this pull request Mar 3, 2020

Remove TAP 3 + 4 adherence claim #93

Merged

lukpueh added 3 commits March 5, 2020 13:15

Adopt TAP 3 multi-role delegations metadata format

45645a4

Update "4.5. File formats: targets.json and delegated target roles" to reflect metadata format introduced by TAP 3

Clarify TAP3 multi-role delegation client workflow

febda3d

Clarify in step 4.5.2.1 of the client workflow that in a multi-role delegation not each but rather a defined threshold of roles must agree on the target hashes and lengths (see `min_roles_in_agreement`).

lukpueh dismissed mnm678’s stale review via 3962417 March 5, 2020 12:21

lukpueh force-pushed the adopt-tap3-metadata branch from 558ab11 to 3962417 Compare March 5, 2020 12:21

lukpueh changed the base branch from master to draft March 5, 2020 12:22

lukpueh closed this Mar 5, 2020

lukpueh reopened this Mar 5, 2020

lukpueh mentioned this pull request Apr 10, 2020

Add TAP for keyid flexibility theupdateframework/taps#112

Merged

mnm678 approved these changes May 27, 2020

View reviewed changes

lukpueh merged commit 4bae5d8 into theupdateframework:draft May 27, 2020

lukpueh mentioned this pull request Oct 21, 2020

Define and document review expectations #130

Closed

lukpueh mentioned this pull request Dec 11, 2020

Is multi-role delegation in 1.0.0 or not? #140

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt TAP 3 multi-role delegations metadata format #57

Adopt TAP 3 multi-role delegations metadata format #57

lukpueh commented Oct 30, 2019

lukpueh left a comment

lukpueh Oct 30, 2019

lukpueh Oct 30, 2019

mnm678 Nov 5, 2019

lukpueh Nov 6, 2019

mnm678 Nov 6, 2019

lukpueh Oct 30, 2019

mnm678 Nov 5, 2019

lukpueh Oct 30, 2019

mnm678 Nov 5, 2019

lukpueh Nov 6, 2019

mnm678 Nov 6, 2019

awwad Nov 6, 2019

lukpueh Nov 7, 2019 •

edited

Loading

lukpueh Oct 30, 2019

trishankatdatadog Nov 5, 2019

lukpueh commented Nov 5, 2019

trishankatdatadog commented Nov 5, 2019

mnm678 Nov 5, 2019

mnm678 Nov 5, 2019

mnm678 Nov 5, 2019

mnm678 Nov 5, 2019

lukpueh Nov 6, 2019

mnm678 commented Nov 5, 2019

lukpueh commented Nov 6, 2019

trishankatdatadog commented Nov 6, 2019

lukpueh commented Nov 7, 2019

lukpueh commented Nov 7, 2019

lukpueh commented Mar 3, 2020

lukpueh commented Mar 5, 2020

mnm678 left a comment

lukpueh commented May 27, 2020

Adopt TAP 3 multi-role delegations metadata format #57

Adopt TAP 3 multi-role delegations metadata format #57

Conversation

lukpueh commented Oct 30, 2019

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh Nov 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Nov 5, 2019

trishankatdatadog commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnm678 commented Nov 5, 2019

lukpueh commented Nov 6, 2019

trishankatdatadog commented Nov 6, 2019

lukpueh commented Nov 7, 2019

lukpueh commented Nov 7, 2019

lukpueh commented Mar 3, 2020

lukpueh commented Mar 5, 2020

mnm678 left a comment

Choose a reason for hiding this comment

lukpueh commented May 27, 2020

lukpueh Nov 7, 2019 •

edited

Loading