Skip to content
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

Snapshot and Timestamp default content #2307

Open
jku opened this issue Feb 23, 2023 · 6 comments
Open

Snapshot and Timestamp default content #2307

jku opened this issue Feb 23, 2023 · 6 comments

Comments

@jku
Copy link
Member

jku commented Feb 23, 2023

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
Copy link
Member Author

jku commented Feb 24, 2023

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
Copy link
Member

lukpueh commented Feb 27, 2023

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
Copy link
Member Author

jku commented Feb 27, 2023

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
Copy link
Member

lukpueh commented Feb 27, 2023

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
Copy link
Member Author

jku commented Feb 27, 2023

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

@jku
Copy link
Member Author

jku commented Feb 28, 2023

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.

  • Repository creates timestamp/snapshot versions in timestamp()/snapshot(): a version is only created when one is needed.
  • This works, except for version 1
  • timestamp and snapshot should only be created in these methods (because in real applications signing keys are not available at other times)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants