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

Exceptions in metadata API, especially verify() #1351

Closed
jku opened this issue Apr 15, 2021 · 2 comments · Fixed by #1435
Closed

Exceptions in metadata API, especially verify() #1351

jku opened this issue Apr 15, 2021 · 2 comments · Fixed by #1435
Assignees
Labels
backlog Issues to address with priority for current development goals
Milestone

Comments

@jku
Copy link
Member

jku commented Apr 15, 2021

I've just tried verifying metadata using the new API. I wanted to do this:

if md.verify(key):
    print("yay")

but it looks like I'll end up with

from securesystemslib import exceptions as sslib_exceptions
from tuf import exceptions
from tuf.api import serialization

try:
    if md.verify(key):
        print("yay")
except (
    exceptions.Error, 
    sslib_exceptions.FormatError,
    serialization.SerializationError, 
    sslib_exceptions.CryptoError,
    sslib_exceptions.UnsupportedAlgorithmError
):
    # 🤷
    pass        

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:

  • It feels like in most cases bleeding these securesystemslib errors through the API is wrong: I don't really want to handle them in my client code and I don't think we should expect client developers to know how to do that
  • why are serialization errors defined in a different place than other errors? should serialization errors at least derive from generic exceptions.Error so I could pass all TUF errors with a single line if I want to (like I think makes sense with verify())
@jku
Copy link
Member Author

jku commented Apr 19, 2021

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.

  • SerializationError:

    • should not require me to import a file I don't otherwise need but maybe that's fine because ...
    • I think this is an internal error of the Serializer (as in we do not know of a case where it should happen: this is very different from e.g. Metadata.from_dict() where SerializationError is what happens if metadata is broken in any way). Maybe we should not list this in verify() documentation
    • Proposal: Remove mentions from Metadata API docs as internal errors
  • Error:

    • this happens if zero signatures were found for key. Is this an error? I feel like this is just return false -- or at least similar to the case where none of the signatures match (in which case we return false)
    • this also happens if there are multiple keyids but that would mean spec violation...
    • Proposal:
      • zero signatures for keyid should result in return false
      • we should file an issue if Metadata does not enforce keyid uniqueness.
      • we should consider multiple keyids case in verify() an internal error: throw but don't document?
  • CryptoError,

    • SSLib throws this if keyid does not match the one in signature. In Metadata.verify() this would be an internal error
    • Proposal: internal error, let it get raised but don't document as exception to handle
  • FormatError

    • SSLib throws if keydict or signature are not properly formatted
    • Metadata should ensure keydict is valid but I'm not sure it can validate signature (without actually running verify on loading the metadata)
    • keydict validity would be an internal error. If signature is not properly formatted should that just mean return false -- this feels a bit wrong though?
    • Proposal: throw a tuf.exceptions.FormatError : there's something unexpected in the data we got
  • UnsupportedAlgorithmError

    • SSLib throws if the keydict is unsupported. SSLib could provide a cheap function to check this on Metadata creation but I'm not sure it's worth it
    • Proposal: throw a tuf.exceptions.FormatError : there's something unexpected in the data we got (it could also be an installation error with missing deps -- but we can't know so can only report the message)

@jku jku changed the title Exceptions (especially) in metadata API Exceptions in metadata API, especially verify() May 26, 2021
@jku
Copy link
Member Author

jku commented May 26, 2021

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?

@jku jku added the backlog Issues to address with priority for current development goals label May 26, 2021
@sechkova sechkova added this to the weeks22-23 milestone May 26, 2021
jku pushed a commit to jku/python-tuf that referenced this issue May 28, 2021
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>
jku pushed a commit to jku/python-tuf that referenced this issue May 28, 2021
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>
jku pushed a commit to jku/python-tuf that referenced this issue May 28, 2021
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>
jku pushed a commit to jku/python-tuf that referenced this issue May 28, 2021
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>
jku pushed a commit to jku/python-tuf that referenced this issue Jun 3, 2021
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 pushed a commit to jku/python-tuf that referenced this issue Jun 7, 2021
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>
@sechkova sechkova modified the milestones: weeks22-23, weeks24-25 Jun 9, 2021
@jku jku closed this as completed in #1435 Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants