theupdateframework / python-tuf

Python reference implementation of The Update Framework (TUF)
https://theupdateframework.com/
Apache License 2.0
1.63k stars 272 forks source link

Snapshot and Timestamp default content #2307

Open jku opened 1 year ago

jku commented 1 year ago

Snapshot and Timestamp constructors try to be clever:

self.meta = meta if meta is not None else {"targets.json": MetaFile(1)}

and

self.snapshot_meta = snapshot_meta or MetaFile(1)

So they set the metafile content without knowing what it really should be

this is annoying as Repository.snapshot() and Repository.timestamp() now think a snapshot and timestamp are not needed -- even though none have been generated yet.

I can work around this but especially for snapshot the default value seems wrong: empty dict would be more correct -- the meta dict should be filled by a conscious decision not by a default value that might be right.

For timestamp there might not be a correct value though. I think MetaFile(0) would be best (although it requires loosening the check for valid MetaFiles): it's at least clear that timestamp doesn't have a snapshot version yet

jku commented 1 year ago

for reference my workaround looks like this when I create online roles:

if role == "timestamp":
    md = Metadata(Timestamp())
    # workaround https://github.com/theupdateframework/python-tuf/issues/2307
    md.signed.snapshot_meta.version = 0
else:
    md = Metadata(Snapshot())
    # workaround https://github.com/theupdateframework/python-tuf/issues/2307
    md.signed.meta.clear()
lukpueh commented 1 year ago

I lean a bit against semi-valid default values. That would be a novelty in the Metadata API, right? I can see how your proposal is convenient for your particular use case, but I'm not sure if that alone should drive the design.

What about None as default? That seems to be more encouraging to make a conscious decision about the values.

jku commented 1 year ago

What about None as default

Can you explain more about how that is different from a 0?

Is the idea that the value is something that prevents serialization from succeeding -- that creating a Timestamp with None is fine but a real MetaFile needs to be inserted before serialization?

That works for me.

lukpueh commented 1 year ago

My concern was more abstract. Our code could very well prevent serialization of MetaFile(0) too. But even before that it might "look" more like valid metadata than None does, to a user who's not perfectly familiar with all subtleties of the TUF spec.

jku commented 1 year ago

I guess I was assuming 0 is clearly "not a real version number", but you are probably correct that's not obvious.

jku commented 1 year ago

I can see how your proposal is convenient for your particular use case

I notice I didn't respond to this. I might be to blame for the design but this is not my use case: this is python-tuf Repository class failing to operate because of python-tuf Metadata API default values.

What about None as default? That seems to be more encouraging to make a conscious decision about the values.

I tried and I dislike it: now all code that handles Timestamp.snapshot_meta.version has to check for None. This would mean 30+ checks in python-tuf alone.

I think I will make a branch with version=0 as default (with a check in to_dict that prevents serialization with value 0) to see what it looks like.