-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix inconsistencies in spec #69
Conversation
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.
some minor nits. Thank you for the pass!
in-toto-spec.md
Outdated
in-toto can be found in the in-toto website. | ||
|
||
### 4.2 File formats: general principles | ||
not required to use JSON. |
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 say this is a different flavor of CJSON?
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.
Made a minor edit to indicate multiple CJSON flavours exist.
described in sections 4.3 and 4.4). KEYID is the identifier of the key signing | ||
the ROLE dictionary. SIGNATURE is a signature of the canonical JSON form of | ||
ROLE. | ||
#### 4.2.1 Key formats |
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 wonder if we want to mention that implementations may serialize to disk using whatever format for private keys they would like to use (I don't think this is clear, and tools like -golang and -rs opted for this to simplify the impl).
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.
Added a note.
8385b49
to
b0056d7
Compare
Signed-off-by: Aditya Sirish <[email protected]>
b0056d7
to
51c788e
Compare
Signed-off-by: Aditya Sirish <[email protected]>
Signed-off-by: Aditya Sirish <[email protected]>
Signed-off-by: Aditya Sirish <[email protected]>
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 the patch, @adityasaky! I pointed out a couple of issues, most notably about key formats. The rest looks pretty good to me.
I'd like to give the section about artifact rule processing another thorough pass and see if it really aligns with the reference implementation. But we can do this in a separate PR.
KEYTYPE is a string denoting a public key signature system. SCHEME is a string | ||
denoting a corresponding signature scheme. KEYVAL is a dictionary containing the | ||
public portion of the key. The following table summarizes the expected entries | ||
for each signing algorithm supported by the reference implementation. |
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.
Is the table meant to be exhaustive? I think the reference implementation supports more schemes than listed (see secure-systems-lab/securesystemslib#308). It also supports keys that don't have the format described above (see secure-systems-lab/securesystemslib#450).
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'd prefer the specification to not specify this at all, tbh. Perhaps we should adopt POUFs for these matters. Do you reckon we describe the fields and leave it at that? Remove key schemes altogether and point them to implementation documentation for specifics?
cc @lukpueh Signed-off-by: Aditya Sirish <[email protected]>
Thanks for the updates, @adityasaky! We can beautify JSON snippets in a separate PR. |
No description provided.