Uh oh!
There was an error while loading. Please reload this page.
Metadata API: implement sig verification in Key, store id in key - #1423
Conversation
sechkova
left a comment
There was a problem hiding this comment.
As already mentioned in the issue, I am in favour of these changes.
Left some thoughts/questions as comments.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Changes based on Teodoras review:
|
joshuagl
left a comment
There was a problem hiding this comment.
I know that it feels a bit repetitive/redundant in the API, but given the consistent use of the term keyid in the specification and in the code (i.e. Key.__init__() and Key.from_dict() both take a keyid parameter), would it make sense to rename Key.id -> Key.keyid?
jku
commented
Jun 7, 2021
I would have called the arguments id as well but that shadows the builtin |
joshuagl
left a comment
There was a problem hiding this comment.
Other than id -> keyid, this LGTM.
This simplifies life for API users as usually a key needs its identifier: this is already visible in how update() becomes simpler in the API. The downside is that 'from_dict()' now has two arguments (so arguably the name is not great anymore but it still does _mostly_ the same job as other from_dicts). This is an API change, if a minor one. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
This is likely not needed by users of the API (as they are interested in the higher level functionality "verify delegate metadata with threshold of signatures"). Moving verify to Key makes the API cleaner because including both "verify myself" and "verify a delegate with threshold" can look awkward in Metadata, and because the ugly Securesystemslib integration is now Key class implementation detail (see Key.to_securesystemslib_key()). Also raise on verify failure instead of returning false: this was found to confuse API users (and was arguably not a pythonic way to handle it). * Name the function verify_signature() to make it clear what is being verified. * Assume only one signature per keyid exists: see theupdateframework#1422 * Raise only UnsignedMetadataError (when no signatures or verify failure), the remaining lower level errors will be handled in theupdateframework#1351 * Stop using a "keystore" in tests for the public keys: everything we need is in metadata already This changes API, but also should not be something API users want to call in the future when "verify a delegate with threshold" exists. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
jku
commented
Jun 8, 2021
oh forgot to metnion: the only changes in the latest force push are renaming "key.id" to "key.keyid" |
Fixes#1417
Adding id to Key class simplifies life for API users as usually a key needs its identifier: this is visible in how Root.add_key() becomes simpler and in how
verify_signature()is now possible to implement in key without an additional id argument.Moving verification to Key arguably also makes the API cleaner because including both "verify myself" and "verify a delegate with threshold" can look awkward in Metadata, and because the somewhat ugly Securesystemslib integration is now Key class implementation detail (see e.g. call to
format_metadata_to_key()).Returning bool on verify failure was found to confuse API users (both me and Teodora misused it the first time) and was arguably not a pythonic way to handle it: Now an exception is raised.
There are two issues in this code that are still unresolved:
I intend to fix these in the next weeks