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

TAP-17: Remove signature wrapper from TUF spec #138

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

adityasaky
Copy link
Contributor

@adityasaky adityasaky commented Apr 30, 2021

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.

@adityasaky adityasaky force-pushed the add-tap-17 branch 2 times, most recently from 5e2b411 to 46fa241 Compare April 30, 2021 18:43
Copy link
Contributor

@mnm678 mnm678 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 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.

tap17.md Outdated Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
tap17.md Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
@adityasaky
Copy link
Contributor Author

adityasaky commented May 6, 2021

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. :)

@adityasaky
Copy link
Contributor Author

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. :)

POUFs/pouf2.md Outdated Show resolved Hide resolved
@adityasaky adityasaky force-pushed the add-tap-17 branch 2 times, most recently from 56e7502 to d75d24d Compare June 22, 2021 17:51
@adityasaky adityasaky changed the title TAP-17: Replace signature envelope with SSL signing-spec Jun 22, 2021
@adityasaky
Copy link
Contributor Author

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.

@adityasaky
Copy link
Contributor Author

Thanks a ton, @mnm678, for helping me draft this. 😄

POUFs/reference-POUF/pouf1.md Outdated Show resolved Hide resolved
tap17.md Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
tap17.md Show resolved Hide resolved
Copy link

@erickt erickt left a 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 Show resolved Hide resolved
* Title: Reference Implementation Using Canonical JSON
* Version: 2
* Last-Modified: 06-May-2020
* Title: Reference Implementation Using Canonical JSON and DSSE
Copy link

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.

Copy link

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.

Copy link
Contributor Author

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.

@adityasaky adityasaky changed the title TAP-17 / POUF-1: Remove signature wrapper from TUF spec, and update reference implementation to use DSSE Aug 25, 2021
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.

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:

POUFs/reference-POUF/pouf1.md Outdated Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
tap17.md Show resolved Hide resolved
POUFs/reference-POUF/pouf1.md Outdated Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
tap17.md Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
tap17.md Outdated Show resolved Hide resolved
tap17.md Outdated

# Backwards Compatibility

No backwards incompatibility.
Copy link
Member

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.

Copy link

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@erickt I wrote a TAP about managing TUF versions that describes a version of publishing metadata using multiple formats that you might be interested in. I'm hoping we can get that or something similar working before adding this and other breaking changes to the spec.

Copy link
Contributor Author

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.

tap17.md Show resolved Hide resolved
@adityasaky
Copy link
Contributor Author

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

@adityasaky adityasaky changed the title TAP-X / POUF-1: Remove signature wrapper from TUF spec, and update reference implementation to use DSSE Sep 10, 2021
@adityasaky
Copy link
Contributor Author

ITE-5, which describes essentially the same change for in-toto, was accepted today!

@adityasaky
Copy link
Contributor Author

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?

@adityasaky adityasaky force-pushed the add-tap-17 branch 2 times, most recently from fce6db6 to 09173ca Compare November 11, 2021 20:49
tap17.md Show resolved Hide resolved
@adityasaky adityasaky changed the title TAP-X: Remove signature wrapper from TUF spec, and update reference implementation to use DSSE Nov 11, 2021
@mnm678
Copy link
Contributor

mnm678 commented Nov 12, 2021

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?

It's probably ready to merge, but we can present it at the community meeting to be sure.

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.

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

Comment on lines +48 to +50
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.
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Signed-off-by: Aditya Sirish <aditya@saky.in>
@joshuagl
Copy link
Member

Three TAP editor approvals, merging this as a draft. Thanks for the TAP @adityasaky !

@joshuagl joshuagl merged commit 1041122 into theupdateframework:master Nov 18, 2021
@adityasaky adityasaky deleted the add-tap-17 branch November 18, 2021 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants