-
Notifications
You must be signed in to change notification settings - Fork 271
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
Exceptions in metadata API, especially verify() #1351
Comments
I think a way forward is to just review all case individually and afterwards try to come up with generic rules. I think there are cases where we do want to throw but do not want to document it: these are "internal error" cases that should not happen and that users cannot realistically prepare for. These should be documented in code though The other group of errors here are the ones that are a result of malformed or unexpected metadata: I think these should all fall under one top-level exception so at least clients can handle that by outright refusing to handle the metadata.
|
I'm limiting this more to verify() for now -- we should be more conscious about the errors in general but... one issue at a time I guess? |
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. * 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 -- 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 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. * 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 -- 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>
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. * 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 -- 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>
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 (e.g. call to format_metadata_to_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>
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>
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>
I've just tried verifying metadata using the new API. I wanted to do this:
but it looks like I'll end up with
It feels like we could do better.
This issue is about exceptions handling in general but I'm fine if the solutions are case-by-case (like maybe verify specifically just does not need to throw at all).
Some specific issues:
The text was updated successfully, but these errors were encountered: