-
Notifications
You must be signed in to change notification settings - Fork 104
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: require length and hashes for target metadata #345
Conversation
Signed-off-by: Asra Ali <asraa@google.com>
@trishankatdatadog @mnm678 @rdimitrov could you please review this change? it's breaking consumption of sigstore metadata from |
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.
As per the spec, both length and hashes should be optional - ref. I wonder though if having the field present with zero can end up being a problem in case there's some client logic that triggers if the field is present without checking the value too.
I don't think that's the correct reference. That's for a metapath length like snapshotted metadata. Target files https://theupdateframework.github.io/specification/latest/#target-files do not mention length being optional. |
But, I guess note that target metadata and metapath metadata have shared |
Yeah, sorry, I was about to edit my comment. I think the problem is that we use the same |
I'll update this PR to be a larger fix then to separate these two metas. I'll add a test for a 0 length target and an optionally omitted lenghth on snapshot meta hopefully. |
Sounds great! 👍 |
Signed-off-by: Asra Ali <asraa@google.com>
updated! with regression test to make sure that target meta always includes a 0. Made as little API changes as possible (see renames of structs) |
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.
Thank you, this fix looks correct. TARGETS
should always have length
and hashes
, with an optional custom
field. METAFILES
should always have version
, with optional length
and hashes
fields.
In python-tuf's metadata API we have have a different class to represent each file type: TargetFile
and MetaFile
. The names of the types feel a little less identifiable here, as this is already a break change (right?) could we rename TargetFileMeta
to TargetFile
?
The only breaking change here is that I'd rather keep the wording |
Oh, I'd misunderstood what a |
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, @asraa! 💯 I also appreciate that you made sure it's not introducing any breaking changes 👍
Signed-off-by: Asra Ali <asraa@google.com>
thanks! all set with @joshua's comment on |
Signed-off-by: Asra Ali asraa@google.com
Please fill in the fields below to submit a pull request. The more information that is provided, the better.
Fixes #
Release Notes:
Rust's
tough
requires length in the JSON, and go-tuf omits if empty. always marshal a length, even if0
. Some files need to be signed as empty because there is explicitly no content there.cc @raulcabello
Types of changes:
Description of the changes being introduced by the pull request:
Please verify and check that the pull request fulfills the following requirements: