fix: timestamp.json meta can has optional fields #778

flavio · 2024-07-01T14:57:10Z

Issue #, if available:

Fixes issue #771

Description of changes:

According to the TUF specification, the meta attribute of
timestamp.json must follow the same specification of METAFILES.
That means it has optional LENGTH and HASHES.

See this section of the TUF specification.

I've handled a missing LENGTH and HASHES in the timestamp.json file using the same logic used by the library when loading targets.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

flavio · 2024-07-01T14:59:19Z

JFYI: TUF's reference implementation (written in python) has LENGTH and HASHES optional also for timestamp.json

jku

Seems correct to me.

The only note I have is that I believe TimestampMeta and SnapshotMeta are now 100% equivalent (as they should be): Having two separate types seems reasonable but maybe there should be only one struct implementation and then TypeAliases for the actual types?

flavio · 2024-07-02T08:00:34Z

I agree with you @jku, I just wanted to go soft on the initial PR to not scare the maintainers 😄

But this change is definitely on the table if the maintainers are fine with that.

jpculp · 2024-07-02T17:55:51Z

This change makes sense to me, although it has triggered a clippy warning. Unless someone disagrees, I think it might be fine to just add a clippy exception to load_snapshot for now and we can break up the function in a later PR.

bcressey · 2024-07-02T23:21:53Z

tough/src/cache.rs

+            self.max_snapshot_size()?
+                .unwrap_or(self.limits.max_timestamp_size),


I would prefer adding a separate max_snapshot_size limit to the limits struct. It can share the default with max_timestamp_size (1 MiB) but it's a different metadata file and clients may want to treat it differently.

Yes, good point. I've fixed that with 4474051

I will squash the commit once the review process is done

bcressey · 2024-07-02T23:35:30Z

tough/src/schema/mod.rs

+    /// The integer length in bytes of the snapshot.json file. It is OPTIONAL and
+    /// can be omitted to reduce the snapshot metadata file size. In that case the client MUST use a
+    /// custom download limit for the listed metadata.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub length: Option<u64>,


After this change, TimestampMeta looks identical to SnapshotMeta. If we're taking an API break for this, seems like we should just converge on a single MetaFile struct, to better convey the idea that this is actually the same type according to the spec.

I agree, do you want me to attempt to perform this change now or would you prefer a dedicated PR later?

I think this should probably be in the same PR. Perhaps a separate commit for now and then we squash at the end of the review process?

I've just pushed a commit that replaces SnapshotMeta and TimestampMeta with Metafiles.

I'm open to suggestions for a better name 😅

flavio · 2024-07-11T06:33:44Z

The CI is failing because clippy complains about the load_snapshot function having too many lines.

Should I add a directive to ignore this warning? The function seems ok to me...

jpculp · 2024-07-11T17:07:05Z

The CI is failing because clippy complains about the load_snapshot function having too many lines.

I would probably just add a clippy exception for now and we can make an issue to split up the function in a separate PR.

flavio · 2024-07-12T08:07:31Z

I would probably just add a clippy exception for now and we can make an issue to split up the function in a separate PR.

Done. BTW, the function length limit is 100, the function is currently 101 lines long 😓

bcressey

Looks good, just a couple minor quibbles - including the inevitable naming discussion 😅

bcressey · 2024-07-16T18:43:13Z

tough/src/schema/mod.rs

@@ -292,7 +292,7 @@ pub struct Snapshot {
 ///    },
 /// ```
 #[derive(Debug, Clone, Deserialize, Serialize, Eq, PartialEq)]
-pub struct SnapshotMeta {
+pub struct Metafiles {


I think this should be Metafile since there's only one. I was thrown at first by the Hashes struct immediately after this one, but that's different because it can include multiple hashes, even though only the sha256 one is used.

In the enclosing structs, the HashMap<String, Metafile> collection implies the "plural" - there can be 0-N metafiles - so the type name doesn't need to reflect it. If you really wanted a closer correspondence to the spec, you could declare a newtype:

struct Metafiles(HashMap<String, Metafile>)

And then Snapshot and Timestamp could say this instead:

pub meta: Metafiles,

But I don't think that's necessary.

@bcressey: sorry, should I go ahead and rename Metafiles to Metafile?

If you want I can also go ahead with the struct Metafiles thing you suggested.

sorry, should I go ahead and rename Metafiles to Metafile?

@flavio: yes please rename Metafiles to Metafile. I wouldn't bother with the second part (newtype) unless you're excited about it 😀

I’m currently on vacation. I’ll look at that on the 1st of August

I'm back from holidays. I've done the rename inside of 0274aa5

I tried to introduce the struct Metafiles, but the change required to implement get and insert methods on it. I don't know if we really want all this extra code.

I'll leave you to mark this comment as solved if you're fine with the change

tough/src/lib.rs

flavio · 2024-08-05T16:32:24Z

Just a reminder, once everybody is happy about the change I'll squash all commits into a single one.

bcressey · 2024-08-08T16:33:46Z

Just a reminder, once everybody is happy about the change I'll squash all commits into a single one.

Looks good to me! Thanks!

According to the TUF specification, the `meta` attribute of `timestamp.json` must follow the same specification of `METAFILES`. That means it has optional `LENGTH` and `HASHES`. See [this](https://theupdateframework.github.io/specification/latest/#file-formats-timestamp) section of the TUF specification. Fixes issue awslabs#771 Signed-off-by: Flavio Castelli <[email protected]>

flavio · 2024-08-08T16:40:52Z

Great, I've squashed all the commits into a single one

flavio · 2024-08-30T12:05:34Z

Hello, do you have any ETA about when this fix will be made available through an official release?

Thanks!

loosebazooka · 2024-09-03T17:04:05Z

Hi @jpculp. Sorry, just pinging this after the long weekend, hoping it doesn't get lost. I understand that sigstore work isn't really your concern but, it would be helpful for us to get a release on this, as we're planning some updates to our TUF metadata that break without it. If there's an ETA, I'd love to try to schedule our work around it.

anfredette · 2024-09-03T22:30:45Z

This change broke bpfman today. Is there a schedule for a release containing this fix? Thanks, Andre

jpculp · 2024-09-03T22:42:45Z

@flavio / @loosebazooka, sorry for the delay! We were taking a look at some other contributions first, but I suspect they will end up in a separate release. I can't commit to a date yet, but I'd like to get this out sooner rather than later. Thank you for your patience!

@anfredette, can you elaborate on the breakage with bpfman?

anfredette · 2024-09-04T00:06:28Z

Thanks for your response, @jpculp! We just started getting the following error from sigstore::trust::sigstore::SigstoreTrustRoot::new():

Failed to parse timestamp metadata: missing field `length` at line 14 column 4

loosebazooka · 2024-09-04T01:30:49Z

Oh my bad I guess they already moved the sigstore metadata.

flavio · 2024-09-04T07:02:57Z

On top of having this fix published, we also need #769 to be merged.

Currently sigstore on the main branch is affected by this issue.
The only way to workaround it is to use a patched version of tough, which is something we cannot do when publishing a new release on crates.io. There we cannot have dependencies pulled from git 😢

Sigstore's TUF repository format changed in way that breaks the tough crate. This commit updates to a forked release of the tough crate that includes fixes for these issues: * awslabs/tough#778 - the fix for the TUF repo change * awslabs/tough#769 - another long standing issue Signed-off-by: Flavio Castelli <[email protected]>

mgsharm · 2024-09-05T00:21:01Z

Hi @flavio / @loosebazooka, this fix is available today through the official release. Thank you for your patience.

flavio · 2024-09-05T06:52:07Z

@mgsharm thanks! now we have to sort out #769

We will need another tough release with whatever fix of the linked issue to have sigstore work :(

flavio mentioned this pull request Jul 1, 2024

client: hashes and length should be optional in (timestamp, snapshot) METAFILEs #771

Open

jku approved these changes Jul 2, 2024

View reviewed changes

jpculp requested review from jpculp and rpkelly July 2, 2024 17:56

bcressey requested changes Jul 2, 2024

View reviewed changes

jpculp approved these changes Jul 16, 2024

View reviewed changes

bcressey self-requested a review July 16, 2024 18:35

bcressey reviewed Jul 16, 2024

View reviewed changes

flavio force-pushed the timestamp.json-optional-fields-inside-of-meta branch from db0b9e6 to ff2625e Compare July 17, 2024 14:34

bcressey approved these changes Aug 8, 2024

View reviewed changes

flavio force-pushed the timestamp.json-optional-fields-inside-of-meta branch from 0274aa5 to f2a8cce Compare August 8, 2024 16:40

jpculp approved these changes Aug 9, 2024

View reviewed changes

arnaldo2792 approved these changes Aug 12, 2024

View reviewed changes

jpculp merged commit 29289de into awslabs:develop Aug 12, 2024
7 checks passed

jku mentioned this pull request Aug 13, 2024

root-signing metadata is incompatible with current sigstore-rs sigstore/root-signing#1251

Open

flavio deleted the timestamp.json-optional-fields-inside-of-meta branch August 13, 2024 14:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: timestamp.json meta can has optional fields #778

fix: timestamp.json meta can has optional fields #778

flavio commented Jul 1, 2024 •

edited

Loading

flavio commented Jul 1, 2024

jku left a comment •

edited

Loading

flavio commented Jul 2, 2024

jpculp commented Jul 2, 2024

bcressey Jul 2, 2024

flavio Jul 3, 2024

bcressey Jul 2, 2024

flavio Jul 3, 2024

jpculp Jul 9, 2024

flavio Jul 10, 2024

flavio commented Jul 11, 2024

jpculp commented Jul 11, 2024

flavio commented Jul 12, 2024

bcressey left a comment

bcressey Jul 16, 2024

flavio Jul 17, 2024

bcressey Jul 19, 2024

flavio Jul 19, 2024

flavio Aug 5, 2024

flavio commented Aug 5, 2024

bcressey commented Aug 8, 2024

flavio commented Aug 8, 2024

flavio commented Aug 30, 2024

loosebazooka commented Sep 3, 2024

anfredette commented Sep 3, 2024

jpculp commented Sep 3, 2024

anfredette commented Sep 4, 2024

loosebazooka commented Sep 4, 2024

flavio commented Sep 4, 2024

mgsharm commented Sep 5, 2024

flavio commented Sep 5, 2024

		self.max_snapshot_size()?
		.unwrap_or(self.limits.max_timestamp_size),

fix: timestamp.json meta can has optional fields #778

fix: timestamp.json meta can has optional fields #778

Conversation

flavio commented Jul 1, 2024 • edited Loading

flavio commented Jul 1, 2024

jku left a comment • edited Loading

Choose a reason for hiding this comment

flavio commented Jul 2, 2024

jpculp commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavio commented Jul 11, 2024

jpculp commented Jul 11, 2024

flavio commented Jul 12, 2024

bcressey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flavio commented Aug 5, 2024

bcressey commented Aug 8, 2024

flavio commented Aug 8, 2024

flavio commented Aug 30, 2024

loosebazooka commented Sep 3, 2024

anfredette commented Sep 3, 2024

jpculp commented Sep 3, 2024

anfredette commented Sep 4, 2024

loosebazooka commented Sep 4, 2024

flavio commented Sep 4, 2024

mgsharm commented Sep 5, 2024

flavio commented Sep 5, 2024

flavio commented Jul 1, 2024 •

edited

Loading

jku left a comment •

edited

Loading