fix: require length and hashes for target metadata

[asraa]

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:

* breaking change: `data.FileMeta` no longer contains a `Custom` field

Rust's tough requires length in the JSON, and go-tuf omits if empty. always marshal a length, even if 0. Some files need to be signed as empty because there is explicitly no content there.

cc @raulcabello

Types of changes:

• [ x ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:

Please verify and check that the pull request fulfills the following requirements:

• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[asraa]

@trishankatdatadog @mnm678 @rdimitrov could you please review this change? it's breaking consumption of sigstore metadata from tuftool because a target file had length 0.

[rdimitrov]

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.

[asraa]

As per the spec, both length and hashes should be optional - ref.

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.

[asraa]

But, I guess note that target metadata and metapath metadata have shared FileMeta in go-tuf....

[rdimitrov]

As per the spec, both length and hashes should be optional - ref.

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.

Yeah, sorry, I was about to edit my comment. I think the problem is that we use the same FileMeta object to describe both metafiles and targets. As you said, it's optional for things like snapshot meta (which is what I had in mind initially), but shouldn't be the same for targets.

[asraa]

As you said, it's optional for things like snapshot meta (which is what I had in mind initially), but shouldn't be the same for targets.

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.

[rdimitrov]

As you said, it's optional for things like snapshot meta (which is what I had in mind initially), but shouldn't be the same for targets.

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! 👍

[asraa]

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)

[joshuagl]

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?

[asraa]

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 FileMeta (the base FileMeta type) no longer contains Custom: added this to the release note. All other names are preserved! I introduced MetapathFileMeta, analogous to TargetFileMeta, which SnapshotFileMeta and TimestampFileMeta alias to.

I'd rather keep the wording XXXXFileMeta since that's how (Targets/Snapshot/Timestamp)FileMeta are all worded in go-tuf. Should I make MetapathFileMeta private and only expose (Targets/Snapshot/Timestamp)FileMeta? I'd rather not break API and remove (Snapshot/Timestamp)FileMeta in favor or something like MetaFile

[joshuagl]

Oh, I'd misunderstood what a TargetFileMeta was. Thanks for explaining. I think making MetapathFileMeta private would be a good change. Otherwise, this LGTM.

[rdimitrov]

Thanks, @asraa! 💯 I also appreciate that you made sure it's not introducing any breaking changes 👍

[asraa]

thanks! all set with @joshua's comment on metapathFileMeta privatized.