-
Notifications
You must be signed in to change notification settings - Fork 54
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
Replace strict JSON requirement with a flexible requirement #102
Replace strict JSON requirement with a flexible requirement #102
Conversation
…that file extensions may change if a different data format is used.
Great PR, Marina, thank you! In fact, I think we should go further and remove strict tie-in with files and filesystems altogether... |
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, @mnm678!
I suggest to change "targets.json" to "targets role" in L335, so that we don't mention json before we say that any format is okay.
Also, the whole spec is pretty json-heavy. So I wouldn't say that only the examples here use json but also all of the definitions. Can we simply say that TUF doesn't have to be json, when all the metadata definitions are in json? I'm not sure if the json format can be unambiguously translated into any other format.
In particular we need to change the way we talk about canonicalization that is in L501:
SIGNATURE is a hex-encoded signature of the canonical JSON form of ROLE.
and in L574-L575:
The KEYID of a key is the hexdigest of the SHA-256 hash of the canonical JSON form of the key.
I agree, but there are a lot of references to filesystems and filepaths throughout the spec, so it might be better to do that is a separate pr. |
I worry that if we say the definitions are in json people will understand that as a json requirement. A translation may not be unambiguous, but I think it should be possible to include all the required in a different data format. I removed the mentions of json in metadata definitions, so hopefully that helps. |
Okay. Maybe we can extend the first sentence in 4.1. to say something like *"... as long as all fields in this specification are included and TUF clients are able to interpret them without ambiguity."?
Thanks! I think we should point out that the digest needs to be calculated in a deterministic manner by using the canonical form of whatever metadata format used, e.g. canonical 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.
Thanks for working on this @mnm678! The only feedback I have @lukpueh has already stated succinctly - we should make it clear that the digest needs to be calculated deterministically.
There was some feedback in one of the issues I reviewed for the mailing list post summarising concerns with Canonical JSON which suggested a wire format which does not require a canonicalisation step before checking the signatures would reduce the need to handle untrusted data.
It may be desirable to have the wire format match what is verified (explicitly not requiring canonicalization/encoding before checking the signature). This would mean implementers do not need to include the JSON parser in the TCB, as it will only parse trusted (verified) metadata.
Would it be worth including this recommendation in the specification? Or perhaps this would belong in the proposed secondary literature #91
Co-authored-by: Joshua Lock <[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 updates, @mnm678. Do you think this should be part of a patch release, or rather of a minor release?
Co-authored-by: lukpueh <[email protected]>
@lukpueh I was thinking a patch release as this doesn't add/change functionality. If the text looks good I can update the version, date, etc |
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.
This LGTM, thanks Marina. I noticed one minor nit, apologies for not spotting it last time.
Co-authored-by: Joshua Lock <[email protected]>
@lukpueh I'm having trouble getting travis-ci to run on this pr. Any idea what could be happening? |
Not sure what the problem was. But it looks like GitHub somehow had missed the notification from Travis. Just re-ran the build. Now it passes. Merging ... |
Addresses #101 by replacing the JSON requirement with a statement that any data format may be used.
This pr keeps the canonical JSON examples in the specification, but adds a clarification about filenames to support non-json metadata.
This change is backwards compatible, so I propose it as a patch to the current version of the specification.