Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion of TAP 15: Succinct hash bin delegations #132

Open
joshuagl opened this issue Dec 16, 2020 · 10 comments
Open

Discussion of TAP 15: Succinct hash bin delegations #132

joshuagl opened this issue Dec 16, 2020 · 10 comments
Milestone

Comments

@joshuagl
Copy link
Member

TAP 15: Succinct hash bin delegations is available at https://github.com/theupdateframework/taps/blob/master/tap15.md

Pull requests related to TAP 15: #120

Outstanding items for TAP 15:

  • Can we make it more clear in the object format that a delegations object needs either name or succinct_hash_delegations, and path_hash_prefixes is optional. If the delegations object includes a succinct_hash_delegations field then any path_hash_prefixes field is ignored.
    • would be nice to do so in a way that suggests a relationship between succinct_hash_delegations and path_hash_prefixes
  • Resolve on the use of name vs bin_name_prefix (probably through some experimenting on the PoC) see Add candidate TAP for succinct hash bin delegations #120 (comment)
@mnm678
Copy link
Contributor

mnm678 commented Feb 4, 2021

* Can we make it more clear in the object format that a delegations object needs _either_ `name` or `succinct_hash_delegations`, and `path_hash_prefixes` is optional. If the delegations object includes a `succinct_hash_delegations` field then any `path_hash_prefixes` field is ignored.
  
  * would be nice to do so in a way that suggests a relationship between `succinct_hash_delegations` and `path_hash_prefixes`

Looking back at this with fresh eyes, it might make more sense to include all three of these fields in the same definition to make the relationships more clear. A delegation may use either succinct_hash_delegations or (name and optional path_hash_prefixes). Which would look something like "succinct_hash_delegations" : ... | "role_name" : {"name": ROLENAME, ("path_hash_prefixes": [ HEX_DIGEST, ...])}.

Unfortunately, the above definition would be a backwards-incompatible change, so it may be better to leave it as is for now (as a change that does not affect non-succinct delegations), and set this as a goal for 2.0.0.

* Resolve on the use of `name` vs `bin_name_prefix` (probably through some experimenting on the PoC) see [#120 (comment)](https://github.com/theupdateframework/taps/pull/120#discussion_r476656697)

This relates somewhat to the format of the delegations object. With the definition I proposed above, bin_name_prefix within the succinct_hash_delegations definition is more clear, but if we are leaving the current definition of the name field, we can discuss this further.

@mnm678 mnm678 added this to the TUF 1.2.0 milestone Apr 29, 2021
@mnm678
Copy link
Contributor

mnm678 commented Jun 23, 2021

The discussion in theupdateframework/specification#156 may be relevant for finding a clear way to state the various options for required fields in this TAP.

@lukpueh
Copy link
Member

lukpueh commented Apr 19, 2022

Some quick questions:

  1. Is there a reason to do non-succinct hash delegation?
  2. Is there a use-case for the path_hash_prefixes-field, other than using it for non-succinct hash delegation?
  3. If the answer to 1. and 2. is "no", does it make sense to deprecate the path_hash_prefixes field and any documentation that describes non-succinct hash delegation (e.g. in python-tuf examples) once TAP 15 is accepted in the spec?

cc @JustinCappos, @trishankatdatadog

@jku
Copy link
Member

jku commented May 2, 2022

I'll add some more:

  • is there a real use case for multiple succinct delegations in one delegating metadata?
  • is there a real use case for a succinct delegation with other types of delegations in one delegating metadata?

I think Martins work in theupdateframework/python-tuf#1948 is showing how supporting those two cases makes this significantly more complex to implement -- and implementation simplicity is something this project could really have more of.

If the two cases above are not required (or can reasonably be implemented by multiple levels of delegating metadata), then we could consider a format where there are two separate ways to delegate:

# Current method (that could still support path_hash_prefixes -- I'm just just leaving it out for clarity)
{ "keys" : {KEYID : KEY, ... },
   "roles" : [{
       "name": ROLENAME,
       "keyids" : [ KEYID, ... ] ,
       "threshold" : THRESHOLD,
       "paths" : [ PATHPATTERN, ... ],
       "terminating": TERMINATING,
   }, ... ]
 }

# new succinct delegation
{ "keys" : {KEYID : KEY, ... },
   "succinct_roles" : {
       "keyids" : [ KEYID, ... ] ,
       "threshold" : THRESHOLD,
       "prefix_bit_length", BIT_LENGTH,
       "name_prefix", NAME_PREFIX,
   }
}

I think this could be much easier to implement correctly (and is easier to define explicitly in the spec).

@mnm678
Copy link
Contributor

mnm678 commented May 2, 2022

Some quick questions:

1. Is there a reason to do non-succinct hash delegation?

non-succinct hash delegations provide more flexibility (to define bin sizes, different keys for different bins), but I'm not sure this flexibility is needed in practice.

2. Is there a use-case for the [`path_hash_prefixes`](https://theupdateframework.github.io/specification/v1.0.29/index.html#path_hash_prefixes)-field, other than using it for non-succinct hash delegation?

No

3. If the answer to 1. and 2. is "no", does it make sense to deprecate the `path_hash_prefixes` field and any documentation that describes non-succinct hash delegation ([e.g. in python-tuf examples](https://github.com/theupdateframework/python-tuf/blob/develop/examples/repo_example/hashed_bin_delegation.py)) once TAP 15 is accepted in the spec?

In the short term I'd opt to keep them for backwards compatibility, but we could drop them in the next major release of the spec. I'll add something to the TAP about eventually deprecating path_hash_prefixes

@trishankatdatadog
Copy link
Member

I never liked the absolutely rigid and ambiguous way we define delegations by going into roles first. It makes multi-role delegations and succinct hash delegations absolutely awkward. At the risk of another PR where we fix this larger, more general issue, what does everyone else think here?

@trishankatdatadog
Copy link
Member

trishankatdatadog commented May 2, 2022

One way to fix the issue is by using an unambiguous data structure that makes each type of delegation absolutely sparkling clear and distinct, so there's no way to mix conflicting fields from different types. I haven't fully thought this through, but this might be possible even with JSON (which has no data structures).

Obviously, this means breaking backwards-compatibility, but if we're going to fix things, let's do it right.

@mnm678
Copy link
Contributor

mnm678 commented May 2, 2022

@trishankatdatadog we could achieve that by separating roles from delegations as follows (I named the delegations "mappings" as this is all in a field called "delegations". We could rethink the names):

{ "keys" : {KEYID : KEY, ... },
   "roles" : [{
       "name": ROLENAME,
       "keyids" : [ KEYID, ... ] ,
       "threshold" : THRESHOLD,
   }, ... ],
   "mappings": [{ #rename this
       "roles": [ROLENAME, ...],
       "paths": [ PATHPATTERN, ... ],
       "role_threshold": THRESHOLD
   }, ...],
   "succinct_mapping": [{ #rename this
       "keyids" : [ KEYID, ... ] ,
       "threshold" : THRESHOLD,
       "prefix_bit_length", BIT_LENGTH,
       "name_prefix", NAME_PREFIX,
   }, ...]
 }

I left out path_hash_prefixes, but it could be added here as well.

This change is breaking for all existing clients, and so would be much harder to achieve without something like TAP 14, but it does simplify TAP 3 (which is actually included with the role_threshold field in my example).

@trishankatdatadog
Copy link
Member

@trishankatdatadog we could achieve that by separating roles from delegations as follows (I named the delegations "mappings" as this is all in a field called "delegations". We could rethink the names):

This is actually a pretty good start, thanks! I have a few reservations (such as how to resolve conflicts between mappings and succinct_mappings, but I don't think there's anything we can't fix).

This change is breaking for all existing clients, and so would be much harder to achieve without something like TAP 14, but it does simplify TAP 3 (which is actually included with the role_threshold field in my example).

True, it does have its pros and cons. Hrm. This is something we need to discuss with all major stakeholders (such as AWS, Datadog, Google, Sigstore, Uptane, etc).

Aside: is there a way we could get each major stakeholder represented in our monthly community meeting? Maybe something like a steering committee would encourage people to show up.

@JustinCappos
Copy link
Member

JustinCappos commented May 3, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants