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

Add initial draft of specification based on ITE-5 #2

Merged
merged 7 commits into from
Dec 1, 2020
Merged

Add initial draft of specification based on ITE-5 #2

merged 7 commits into from
Dec 1, 2020

Conversation

adityasaky
Copy link
Member

This is an initial draft of the specification based on the recommendations by @MarkLodato in in-toto/ITE#13.

Relevant to the discussions in #1 too.

@adityasaky adityasaky marked this pull request as draft October 19, 2020 21:09
Signed-off-by: Aditya Sirish <aditya@saky.in>
@adityasaky
Copy link
Member Author

I've copied over ITE-5 and related resources here verbatim, it serves as a good starting point for us. Any thoughts on the best way to proceed?

cc @SantiagoTorres @mnm678 @lukpueh @joshuagl @trishankatdatadog

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Show resolved Hide resolved
- Parse SERIALIZED_BODY according to PAYLOAD_TYPE. Reject if the parsing
fails.

Either standard or URL-safe base64 encodings are allowed. Signers may use
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a good reason to allow for this flexibility? Why not just use the URL-safe one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the late response. I've been in contact with different integrators and some of them use the non-url-safe version of b64. I wonder if this is something we can revisit on future iterations though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reason for supporting both is that it makes it easier to produce signatures, at the small cost in complexity for consuming. In particular, protocol buffers produce standard encoding when converted to JSON, so it would be inconvenient if only URL-safe were supported. Since there will likely be only a handful of libraries to consume signatures, I thought this shift in complexity might be warranted.

specification.md Outdated Show resolved Hide resolved
@adityasaky
Copy link
Member Author

Thanks for the review, @trishankatdatadog, I'm going to try and resolve them shortly.

cc @mnm678 @lukpueh @SantiagoTorres @trishankatdatadog:

How do you all feel about merging this PR in as version 0.1? The idea is to then update the ITE-5 PR to become a proposal to use signing-spec over the current document format.

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

How do you all feel about merging this PR in as version 0.1? The idea is to then update the ITE-5 PR to become a proposal to use signing-spec over the current document format.

Provided my comments are reviewed, I have no strong objections.

@SantiagoTorres
Copy link
Collaborator

I feel that we could get away with merging provided we solve the URI element to backwards-compatible json. I propose something like:

https://github.com/secure-systems-lab/backwards-compatible-json

Or such.

Signed-off-by: Aditya Sirish <aditya@saky.in>
Signed-off-by: Aditya Sirish <aditya@saky.in>
@adityasaky adityasaky marked this pull request as ready for review November 25, 2020 03:12
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.

I'm fine with merging this as is modulo some minor nits below. Note that I have not reviewed the two *.ipynb files, has anyone else?

specification.md Outdated Show resolved Hide resolved
specification.md Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated
```

The PAYLOAD_TYPE is a URI indicating how to interpret SERIALIZED_BODY. It
encompasses the content type (JSON, CBOR, etc.), the purpose, and the schema
Copy link
Member

Choose a reason for hiding this comment

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

Should we recommend a canonical content type ? In one of our first discussions about this proposal we agreed that producing the signature over the canonical form has advantages, i.e. it makes signatures reproducible.

Copy link
Collaborator

@trishankatdatadog trishankatdatadog Nov 25, 2020

Choose a reason for hiding this comment

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

💯

I'd like to start off this draft as backwards-compatible as possible. We cannot simply break existing clients unless we put this in the next major versions of tuf and in-toto...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added Canonical-JSON to the list of options, rather than replace JSON. I wonder if the spec is best served by not taking a stance requiring canonicalization, while we as implementers for in-toto / TUF can use it to get benefits like reproducible signatures. As for backwards compatibility, the document has workflows for it which use canonical JSON for the current document formats. For new documents, the spec is not taking a stance against canonical JSON, it's merely not requiring it. Does that work? :)

@lukpueh
Copy link
Member

lukpueh commented Nov 25, 2020

So far we don't say anything about public key formats. I was hoping to resolve the issues that we have been brainstorming in secure-systems-lab/securesystemslib#251 and define supported key formats as part of this specification. Or should they go in a separate document, or be implementation specific?

@trishankatdatadog
Copy link
Collaborator

So far we don't say anything about public key formats. I was hoping to resolve the issues that we have been brainstorming in secure-systems-lab/securesystemslib#251 and define supported key formats as part of this specification. Or should they go in a separate document, or be implementation specific?

Great point. Let's put it in draft 2? 🙂

Copy link
Collaborator

@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 keeping this moving along @adityasaky, this looks reasonable to me as a starting point.

specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated Show resolved Hide resolved
specification.md Outdated
```

The PAYLOAD_TYPE is a URI indicating how to interpret SERIALIZED_BODY. It
encompasses the content type (JSON, CBOR, etc.), the purpose, and the schema
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
encompasses the content type (JSON, CBOR, etc.), the purpose, and the schema
encompasses the content type (Canonical-JSON, CBOR, etc.), the purpose, and the schema

I don't think we want to suggest using this signing scheme over non-canonicalised JSON?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added Canonical JSON to the list rather than replace JSON and I think the in-toto and TUF teams can continue using canonical JSON for things like reproducible signatures. The spec doesn't mandate canonicalization here, but leaves it up to the implementers. Does that sound good? :)

FYI, Lukas brought up the same point here: #2 (comment)

- fix references
- fix sentences
- remove empty sections
- fix header section

Signed-off-by: Aditya Sirish <aditya@saky.in>
Signed-off-by: Aditya Sirish <aditya@saky.in>
Signed-off-by: Aditya Sirish <aditya@saky.in>
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.

Thanks for addressing the comments, @adityasaky!

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