Add root metadata class to new TUF metadata model

[sechkova]

Fixes #1137

Description of the changes being introduced by the pull request:

• Added root metadata class to tuf.api.metadata module and implements serialization and update methods.
• Added root and targets metadata tests

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

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[joshuagl]

Great catch on the use of dictionaries in the test!

I think your proposed interface for Root.update() here might be inconsistent with the API in the other metadata classes. More details on that inline, what do you think?

[joshuagl]

Suggested change

	supports consistent snapshots,
	supports consistent snapshots.

[joshuagl]

I think we might have an API granularity mismatch here.

Snapshot.update() takes information for a single targets metadata file, to update/add to the listed files in the snapshot metadata file.

Targets.update() takes information for a single target file, to update/add the fileinfo for a single target file.

Whereas Root.update() is overwriting all of the keys and roles defined in the root metadata.
Perhaps keys and roles parameters should be optional and singular (key and role), taking a single keydict to add/update and a single roleinfo to add/update? That probably gets easier with classes for complex metadata fields (#1139).

I'd appreciate hearing from some library users (@trishankatdatadog, @woodruffw, @jku) and co-maintainers (@lukpueh) on this topic of API granularity.

[sechkova]

Now that you've said it, it seems you have a point. I think I like an api of the form:
Root.update('timestamp', key)
but what if you want to revoke a key and remove it, not only add/update one?
Should we then extend Root with Root.remove/delete(role, key) or implement this logic ... somewhere else?

How do you feel about consistent snapshot? Maybe we need a setter/getter pattern for toggling it independently? I am not sure ... a user of the class can access it anyway directly

[lukpueh]

Good questions! I think the update methods on Targets/Snapshot/Timestamp are supposed to facilitate the chain of (currently manual) updates that are necessary when the target files in the repository are updated.

Root is not part of that chain, so it may not need to be consistent with the other update functions. Regardless, something like Root.update/delete(role_name, key) IMHO seems to add more value than mere (multi-)setter logic.

Generally, I think we should do in these methods whatever is convenient for updating an individual role either manually or from within methods like Repository.update_targets(...), Repository.create_snapshot(...), Repository.rotate_keys(...) etc., i.e. the methods that actually do TUF.

This discussion seems related to #1134.

[joshuagl]

Generally, I think we should do in these methods whatever is convenient for updating an individual role either manually or from within methods like Repository.update_targets(...), Repository.create_snapshot(...), Repository.rotate_keys(...) etc., i.e. the methods that actually do TUF.

I think that sounds like a sensible approach.

I like the Root.update() and Root.remove() interfaces you've proposed here @sechkova. Do they fulfil the needs which led you to create this PR?

I think the best path forward for this PR would be to implement as much of Root functionality as you need to proceed. We can modify/extend the metadata API as we work on making use of it from higher-level pieces of the refactor.

[sechkova]

I worked on this PR in the context of the client meaning that I only need read access to 'signed' dictionaries.
Anyway I think we all agree on implementing a set of Root.update() and Root.remove() interfaces which won't cost me much and will be valuable for moving the metadata API forward.

[sechkova]

Added two root methods that should be enough for a key revocation:

root.add_key(role, keyid, key_metadata) - root.update seemed ambiguous since a new keyid is appended to the role's list of keyids
root.remove_key(role, keyid)

I think this is what we can do for now, until we decide how to represent the internal metadata fields. For the same reason I haven't added any errors/warnings on missing/already existing keyids but I will if you don't like the idea of silently ignoring these cases.

[joshuagl]

root.add_key(role, keyid, key_metadata) - root.update seemed ambiguous since a new keyid is appended to the role's list of keyids

update() would be consistent with Targets.update(), which either replaces an existing entry in the targets dict, or adds a new one when the filename (key) is new.

[sechkova]

root.add_key(role, keyid, key_metadata) - root.update seemed ambiguous since a new keyid is appended to the role's list of keyids

update() would be consistent with Targets.update(), which either replaces an existing entry in the targets dict, or adds a new one when the filename (key) is new.

I see your point about consistency but what would root.update('targets', keyid ...) mean with this metadata format:

 "targets": {
    "keyids": [
     "135c2f50e57ff11e744d234a62cebad8c38daf399604a7655661cc9199c69164"
    ],
    "threshold": 1
   },

We don't expect different keys with the same keyid, do we?

[joshuagl]

root.add_key(role, keyid, key_metadata) - root.update seemed ambiguous since a new keyid is appended to the role's list of keyids

update() would be consistent with Targets.update(), which either replaces an existing entry in the targets dict, or adds a new one when the filename (key) is new.

I see your point about consistency but what would root.update('targets', keyid ...) mean with this metadata format:

 "targets": {
    "keyids": [
     "135c2f50e57ff11e744d234a62cebad8c38daf399604a7655661cc9199c69164"
    ],
    "threshold": 1
   },

You're right, update() doesn't make much sense with a list. 👍

We don't expect different keys with the same keyid, do we?

Not as I understand it. From TAP 12

keyids defined within a metadata file must be unique. For example, a root file that delegates trust to root, snapshot, timestamp, and top-level targets should provide unique keyids for each key trusted to sign metadata for these roles.

[joshuagl]

Thanks for working through the details with me

[joshuagl]

Trying a close/re-open to trigger CI.