-
Notifications
You must be signed in to change notification settings - Fork 20
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
TAP-17: Remove signature wrapper from TUF spec #138
Conversation
5e2b411
to
46fa241
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work on this @adityasaky! I think this will be a great simplification to the spec. My main concern is making sure that this will interact well with TAP 11, so making it clear that a specific json encoding isn't required, but an implementation could use ASN.1, etc if they do the proper checks.
Thanks for the review, @mnm678! I've resolved the easier comments. I'll take a look at TAP-11 in the next few days and see if this one conflicts with it. :) |
I've added an early draft of a new POUF that uses DSSE rather than the current format. I've also marked several sections pointing to POUF-1 for now, because they shouldn't be changing as part of the envelope. The way I see it, I think if/when the reference implementation moves over to DSSE, we can fill in these sections and mark the first POUF as obsolete? Please correct me if I'm misunderstanding how POUFs work. :) |
56e7502
to
d75d24d
Compare
I've updated POUF-1 to use DSSE, though there are some TODOs we need to fill in before this can be merged. I've also updated TAP-17 to remove the signature wrapper from TUF entirely, and instead lean entirely on POUFs for that information. |
Thanks a ton, @mnm678, for helping me draft this. 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[removed since I misunderstood some things]
POUFs/reference-POUF/pouf1.md
Outdated
* Title: Reference Implementation Using Canonical JSON | ||
* Version: 2 | ||
* Last-Modified: 06-May-2020 | ||
* Title: Reference Implementation Using Canonical JSON and DSSE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're making a backwards incompatible change to the format, it'd be really nice if we could address the whole canonical json is not json issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, would we still need canonicalization if we used this envelope format? I don’t think the update flow would ever need to re-serialize the metadata after verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @erickt, I missed this. Canonicalization shouldn't be necessary when using DSSE. One of its goals is to avoid canonicalization, therefore ensuring possibly untrusted inputs are not parsed. When the implementation makes the switch and stops relying on canonicalization, I suspect theupdateframework/specification#92 should be addressed, but perhaps someone more qualified than me can weigh in on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this TAP @adityasaky !
In principle, I am on-board with the TAP. I'd like to see a few changes before we merge it as draft, though:
- drop edits to the POUF until such a time as the reference implementation is updated or make this a separate POUF as suggested in TAP-17: Remove signature wrapper from TUF spec #138 (comment)
- Some discussion in the TAP itself about updating an implementation between wire formats and requiring TAP 14
tap17.md
Outdated
|
||
# Backwards Compatibility | ||
|
||
No backwards incompatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section would benefit from some more content. It's strictly true that it's not backwards incompatible for the specification, but the specification has a symbiotic relationship with the reference implementation. A change of the signature wrapper in the reference implementation absolutely is backwards incompatible, as the POUF update indicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we are quite interested in switching to DSSE, and we’d appreciate any guidance on making the transition. Our situation is complicated by us having to support products that don’t support DSSE need to use TUF to update to the latest version.
I’m guessing we will have to either use a tombstone repo which updates us to some new version that points at the new DSSE repo, or we start publishing the metadata in both formats, where the metadata files have different file extensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed some changes discussing a transition period (on a per implementation basis) during which metadata is published in multiple formats. Also links to TAP-14.
a4e3d07
to
7634177
Compare
I believe I've addressed everything other than the backward compatibility comment, @joshuagl. Thank you for the review! On that front: perhaps we can discuss it in the context of it being a backward incompatible change for anyone switching from the current wrapper to any other wrapper, DSSE or otherwise, and therefore they must provide a transition period and clear communication of the breaking change? This is what we have in ITE-5. See: https://github.com/in-toto/ITE/blob/master/ITE/5/README.adoc#backwards-compatibility |
ITE-5, which describes essentially the same change for in-toto, was accepted today! |
7634177
to
e619ca1
Compare
With the changes to backwards compatibility (#138 (comment)), I wonder if we can merge this TAP as a draft? Should it presented at the community meeting next week first? |
fce6db6
to
09173ca
Compare
09173ca
to
47c0677
Compare
It's probably ready to merge, but we can present it at the community meeting to be sure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this TAP @adityasaky! It LGTM to merge as a draft. I think questions to answer are:
- Should the spec recommend/suggest/mention a format which implements the proposed properties? i.e., DSSE
- what should the pedagogical examples look like after the spec is updated per this TAP? do we continue to use JSON for file format examples?
- should we include guidance on payload type, particularly given this is a motivation for the TAP?
- what recommendations should the spec make about capturing implementation details in a POUF, especially payload type (how to identify the implementation)?
I've captured these questions in a discussion thread #144
with multiple implementations and derivatives. Indeed, every POUF that describes an | ||
implementation MUST choose a unique payload type, ensuring that there is no confusion | ||
about which implementation generated some piece of metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the POUF format be updated to include capturing the unique payload type for an implementation? Should we provide any guidance on forming payload type?
The spec should probably more strongly recommend capturing the implementation details in a POUF, especially the payload type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the POUF format be updated to include capturing the unique payload type for an implementation?
This is probably a good idea. @mnm678 what do you think?
TAP-11 lists only the minimum fields a POUF must contain, so in theory it can be extended without changing the TAP, but it's probably worth adding a field. I also wonder if it's worth adding something to formally identify the implementations a POUF describes. POUF-1 identifies the python implementation, but does it also describe the Go implementation, for example? Should it link to them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide any guidance on forming payload type?
I'm not sure we can, since we're only saying the envelope should support an authenticated payload type, while each envelope format may enforce its own standard. Guidance here may run afoul of some signature wrapper that is otherwise compliant? I think the selection of a unique payload type that conforms to a particular wrapper's specification is sufficient, as long as it's recorded in the corresponding POUF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's worth referring to a survey of wrappers to understand their guidelines for payload types. We may be able to specify some aspects like "include information about encoding" without running into trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the POUF format be updated to include capturing the unique payload type for an implementation?
This is probably a good idea. @mnm678 what do you think?
TAP-11 lists only the minimum fields a POUF must contain, so in theory it can be extended without changing the TAP, but it's probably worth adding a field. I also wonder if it's worth adding something to formally identify the implementations a POUF describes. POUF-1 identifies the python implementation, but does it also describe the Go implementation, for example? Should it link to them?
I agree, I think we could add payload type as a subsection of Formats in the POUF definition.
I would say the implementations should/could link to the POUF rather than the other way around. The implementation implements TUF plus some POUF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say the implementations should/could link to the POUF rather than the other way around. The implementation implements TUF plus some POUF.
Makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, I think we can file an issue against TAP 11 to discuss payload type once we merge this as draft.
47c0677
to
281284d
Compare
Signed-off-by: Aditya Sirish <[email protected]>
281284d
to
be62a29
Compare
Three TAP editor approvals, merging this as a draft. Thanks for the TAP @adityasaky ! |
TAP-17 proposes dropping the signature wrapper specification (section 4.2 of the TUF spec), and instead presenting some properties that a signature wrapper must have for use in a TUF implementation.
The proposed POUF-1 update changes the signature wrapper from the current one to DSSE v1.0. This is now in #143
Original message:
This TAP proposes replacing the existing signature wrapper of TUF with DSSE. in-toto, TUF's sister project, has had a similar effort. That proposal, ITE-5 was authored by @SantiagoTorres and @MarkLodato.