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

Tap14 updates #158

Merged
merged 17 commits into from
Oct 11, 2022
Merged

Tap14 updates #158

merged 17 commits into from
Oct 11, 2022

Conversation

mnm678
Copy link
Contributor

@mnm678 mnm678 commented Jul 26, 2022

I updated TAP 14 based on some implementation questions from @abs007.

This replaces references to the now-rejected TAP 5 with TAP 13 and adds a new optional file to support a range of filesystem types.

@mnm678 mnm678 requested review from joshuagl and lukpueh August 2, 2022 15:42
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo one type and a request for clarification (see inline)

tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
@lukpueh
Copy link
Member

lukpueh commented Aug 3, 2022

I have an non-PR-related question about the following sentence in the TAP:

This allows them to differentiate between metadata that is expired due to deprecation and metadata that is expired due to an attack.

What does "expired due to an attack" mean?

znewman01
znewman01 previously approved these changes Aug 3, 2022
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
@mnm678 mnm678 requested review from znewman01 and joshuagl and removed request for znewman01 September 20, 2022 20:37
joshuagl
joshuagl previously approved these changes Sep 21, 2022
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
Copy link

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, sorry to be annoying. I just had a bunch of worries come up

tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Show resolved Hide resolved
tap14.md Show resolved Hide resolved
Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Marina! My biggest concern is on why we need a signed supported-versions.json somewhere for repositories that don't support listing of what appear to files in what appear to be folders. Could we please consider simpler mechanisms?

tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
@znewman01
Copy link

My biggest concern is on why we need a signed supported-versions.json somewhere for repositories that don't support listing of what appear to files in what appear to be folders. Could we please consider simpler mechanisms?

@trishankatdatadog I think that's most repositories. For instance, go-tuf only has built-in support for HTTP remotes, and IIRC python-tuf is the same, but HTTP doesn't have a (standard/uniform) way to list the contents of a directory. We ran into this when attempting to build out TAP-14 support in python-tuf.

We're open to alternatives suggestions, of course. The first thought was to stick an index.txt file in each folder, but if there's a lot of root metadata files that gets annoying really quickly. So then we decided to filter it down to just a list of folders containing supported versions, but v1.x.y is a funky special-case (do we call it 1/ or /) so we figured being explicit was better.

Signing is something that feels appropriate to me as a "better safe than sorry" thing. I think we could get away with not requiring it, but it feels nicer to me to have the roots sign off on something as consequential as a major version transition.

Co-authored-by: Trishank Karthik Kuppusamy <[email protected]>
Signed-off-by: Marina Moore <[email protected]>
@JustinCappos
Copy link
Member

JustinCappos commented Sep 30, 2022 via email

@mnm678
Copy link
Contributor Author

mnm678 commented Sep 30, 2022

I wrote up the pros and cons in a google doc.

In summary, I agree with Trishank that this information can be included in existing TUF metadata.

Edit: I updated this pr to show what this would look like

@mnm678
Copy link
Contributor Author

mnm678 commented Sep 30, 2022

Can we get consensus on this approach from TAP maintainers? @JustinCappos and @trishankatdatadog have already looked. @lukpueh and @joshuagl wdyt?

joshuagl
joshuagl previously approved these changes Oct 3, 2022
Copy link
Member

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a field to root metadata feels like a more appropriate path forwards. 👍

tap14.md Outdated Show resolved Hide resolved
@lukpueh
Copy link
Member

lukpueh commented Oct 5, 2022

Unrelated observation about this paragraph:

For example, during the course of an update a client could use 4.0.0_parse_root, 4.0.0_parse_snapshot, 4.0.0_parse_timestamp, 4.0.0_parse_targets, 4.0.0_parse_delegation, and 3.0.0_parse_delegation.

If the order of function calls should represent the course of an update, *_parse_timestamp should be before *_parse_snapshot.

@lukpueh
Copy link
Member

lukpueh commented Oct 5, 2022

Unrelated observation about this paragraph:

If the change adds a new feature that is backwards compatible, for example in TAP 4 and TAP 10 [...]

TAP 10 claims that it is backwards incompatible.

@lukpueh
Copy link
Member

lukpueh commented Oct 5, 2022

Ping unrelated observation from #158 (comment), cc @mnm678

tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
Consistently use "supported_versions" (with underscore) as name
for newly added field.

Signed-off-by: Lukas Puehringer <[email protected]>
In a real TUF client update timestamp parsing happens before
snapshot parsing.

Signed-off-by: Lukas Puehringer <[email protected]>
TAP 10 is wrongly used as example for a backwards compatible TAP,
at least according to the "Backwards Compatibility" section in
TAP 10:
https://github.com/theupdateframework/taps/blob/master/tap10.md#backwards-compatibility

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh
lukpueh previously approved these changes Oct 6, 2022
Copy link
Member

@lukpueh lukpueh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following a chat with @mnm678 and @JustinCappos, I just pushed a couple of minimal fixes for the concerns I raised. @joshuagl, can you green-light again?

tap14.md Outdated Show resolved Hide resolved
tap14.md Outdated Show resolved Hide resolved
@mnm678
Copy link
Contributor Author

mnm678 commented Oct 7, 2022

Based on discussion with @JustinCappos and @lukpueh I simplified the root metadata update mechanism in this TAP. The new mechanism allows the root version numbers to diverge between specification versions without compromising the security of an upgrade. I will update the PoC shortly to reflect this change.

Copy link
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hash / supported_versions change to the text. Looks ready to me.

Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM to merge as draft for now, please do address my comments as we discussed earlier:

  • The unclear benefits of becomes_obsolete at the expense of added complexity (e.g., delegatees could accidentally create conflicts)
  • If we keep becomes_obsolete, clarify whether it uses the same datetime format as expires
  • Clarify that root rotation is done as before this TAP for the current known MAJOR spec version before looking at (the latest) supported_versions
  • Clarify whether a root metadata file refer to MAJOR versions lower than or equal to the current spec_version in that file (and metadata version directory)
  • There are only 3 cases for checking version numbers: <, =, or >. The current text is ambiguous about the < case. Please clarify.

@mnm678 mnm678 merged commit 465bcfe into theupdateframework:master Oct 11, 2022
@mnm678 mnm678 deleted the tap14-updates branch March 1, 2023 15:16
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

Successfully merging this pull request may close these issues.

7 participants