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

ITE-5: Replace signature wrapper with SSL signing spec #13

Merged
merged 24 commits into from
Apr 20, 2021

Conversation

MarkLodato
Copy link
Contributor

This is an initial draft of ITE-5 that defines a new signature scheme. The reasoning is explained in the body. I have already discussed with @SantiagoTorres, who will sponsor this ITE.

Note: This is in Markdown because that's the format that I know and it's much simpler to edit in. We'll convert to Asciidoc before submitting. I'm sending this out now to get initial feedback on the idea.

Several sections are still empty. They need to be fleshed out (or removed) before the ITE is accepted.

This is in Markdown because that's the format that I know and it's much
simpler to edit in. I'll convert to Asciidoc later.

Several sections are still empty. They need to be fleshed out before the
ITE is accepted.
ITE/5/README.md Outdated Show resolved Hide resolved
@adityasaky
Copy link
Member

Hi Mark, thank you for putting this up as an ITE! It makes it easier for the community to discuss and evaluate proposals.

ITE-5 came up today in an in-toto + TUF discussion on our signature wrapper and our current plan to spin it off into a standalone spec. While this doesn't yet mean this ITE is approved, it received positive feedback. The primary concern is with supporting two formats - we have some legacy clients that will need to continue using the old format, and that'll have to be phased out slowly. We aren't certain we have the bandwidth to maintain implementations for two formats. But apart from that, from the perspective of adopting these into the specification, there are positive signs!

@MarkLodato
Copy link
Contributor Author

Great, thank you for the update! If there is anything I can do to help, please let me know.

(Note: I'll be on leave for the next several weeks but my colleagues will be covering for me.)

@trishankatdatadog
Copy link
Member

Now that I think about it: it's good that payload type is part of each signature, but what happens when sig A claims that the serialized body is type X, but sig B claims that it is type Y? Should payload type be a fixed part of key distribution? Should all keys/sigs be required to have the same payload type for the serialized body?

@MarkLodato
Copy link
Contributor Author

Hi Trishank, there is only a single payloadType at the top level so all signatures must use the same type. Is there something in the doc that would imply otherwise? I'm happy to make edits to clarify.

Personally, I'd recommend removing multiple signature support altogether. It is a rarely used feature that adds increased complexity and attack surface. Example: go-jose had a vulnerability where it would return "verified" if any of the signatures passed. If multiple signatures are needed, each signer can produce a separate link file. You can always verify that the payloads are identical, if needed.

I didn't go so far as to suggest that in this doc, but perhaps I should?

@trishankatdatadog
Copy link
Member

Hi Trishank, there is only a single payloadType at the top level so all signatures must use the same type. Is there something in the doc that would imply otherwise? I'm happy to make edits to clarify.

Personally, I'd recommend removing multiple signature support altogether. It is a rarely used feature that adds increased complexity and attack surface. Example: go-jose had a vulnerability where it would return "verified" if any of the signatures passed. If multiple signatures are needed, each signer can produce a separate link file. You can always verify that the payloads are identical, if needed.

I didn't go so far as to suggest that in this doc, but perhaps I should?

in-toto produces signatures per functionary separately using different link file per signature. However, TUF shares the same signature format as in-toto, and will be affected by this change. In TUF, we use threshold signatures to require that m out of n roles have signed off on the same metadata payload. TUF does not intrinsically suffer from the same vulnerability as go-jose.

What do others think? @JustinCappos @SantiagoTorres

@davidstrauss
Copy link

davidstrauss commented Oct 20, 2020

We would value the ability to validate signatures in the PHP-TUF client without a cJSON implementation (or any JSON parsing at all, ideally).

Perhaps off-topic for this issue but related to signature modularity, it would also be nice to support FIDO2-style signatures, which could offer a uniquely easy and inexpensive way to enforce offline, hardware-isolated signing.

The specifics of the signature wrapper were moved to their own
repository/effort, in an attempt to better sync goals between TUF and
in-toto team members.

In turn, we will refer to that spec here, but mention why we are
adopting such a spec in ITE-5.
@SantiagoTorres
Copy link
Member

Hi everyone. I'm shaking the dust off this ITE. I rephrased things around to check the specifics on signing-spec, rather than here. I also tried to elaborate in our in-toto-specific motivations. Does this look like what you had in mind?

@adityasaky @MarkLodato @trishankatdatadog @JustinCappos

ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
@SantiagoTorres SantiagoTorres changed the title ITE-5: Signature scheme that avoids canonicalization ITE-5: Replace signature wrapper with SSL signing spec Nov 24, 2020
Copy link
Member

@adityasaky adityasaky left a comment

Choose a reason for hiding this comment

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

Minor nits, but this looks good to me! Can we get rid of the hypothetical attack and keep it in signing spec?

ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
Co-authored-by: Aditya Sirish <8928778+adityasaky@users.noreply.github.com>
@SantiagoTorres
Copy link
Member

thank you @adityasaky !

ITE/5/README.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

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

Very minor edits suggested for clarity. Apologies if this is too close to bike shedding.

ITE/5/README.md Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
Thank you @joshuagl!

Co-authored-by: Joshua Lock <jlock@vmware.com>
@MarkLodato
Copy link
Contributor Author

It would be helpful to merge the PR so that we can link to ITE-5 without referencing this PR. It seems like there hasn't been any updates or comments in many months. Is anything more needed? Note that it's still Draft Status.

@SantiagoTorres
Copy link
Member

+1, I think there are two small comments and we could merge...

Copy link
Contributor Author

@MarkLodato MarkLodato left a comment

Choose a reason for hiding this comment

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

Oops! I didn't realize there were unresolved comments. I went through and addressed them as well as made several more fixes. I think it's in OK state now, modulo the comments below.

ITE/5/README.adoc Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
ITE/5/README.adoc Outdated Show resolved Hide resolved
These notebooks are no longer relevant to the ITE, now that the details
have moved to the signing-spec repo. The reference implementation in
this ITE now refers to in-toto itself, and there is not one yet.
Since the message already has its own type indicator, it's better to
call them the same thing.

Also prefix the URL path with "schemas/".
This ITE is purely about signing-spec.
@MarkLodato
Copy link
Contributor Author

OK, I think all the comments should be resolved now. Please take a look at the latest wording, particularly the psuedocode. Thanks!

README.md Outdated Show resolved Hide resolved
@SantiagoTorres
Copy link
Member

Merging now 👍 I'll announce this on the ML soon™

@SantiagoTorres SantiagoTorres merged commit 1b010bc into in-toto:master Apr 20, 2021
@MarkLodato MarkLodato deleted the ite-5 branch April 24, 2021 13:08
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.

None yet

8 participants