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

BaseFile._verify_hashes: handle sslib errors #1454

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

sechkova
Copy link
Contributor

Fixes #1452

Description of the changes being introduced by the pull request:
BaseFile._verify_hashes handles the following sslib errors raised during hash verification:

  • securesystemslib.exceptions.UnsupportedAlgorithmError
  • securesystemslib.exceptions.FormatError

and reraises them as tuf.exceptions.UnsupportedAlgorithmError.

securesystemslib.exceptions.FormatError should not be raised if the Target/MetaFile object is
created correctly through its constructor but is possible if Target/MetaFile.hashes is modified
directly so catching it just in case.

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

@sechkova
Copy link
Contributor Author

Depends on #1451 so opened as a draft.

@jku
Copy link
Member

jku commented Jun 18, 2021

This is the core question:

reraises them as tuf.exceptions.UnsupportedAlgorithmError

this is the obvious choice... yet I'm left wondering if it's actually useful for the API users.

  • in my usage I've only been interested in "have all hashes been verified to match the content?" and now I need to handle another exception to reliably get an answer
  • Is there a reasonable use for UnsupportedAlgorithmError? What might an application do in this case that it does not want to do in the case of LengthOrHashMismatchError?

Follow up in the case that there is a real use case:

  • if there is a reasonable use case, should the error we use derive from LengthOrHashMismatchError so that users who do not care about unsupported algorithms could just handle that one?

sslib_exceptions.UnsupportedAlgorithmError,
sslib_exceptions.FormatError,
) as e:
raise exceptions.UnsupportedAlgorithmError(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming we go with this, it needs to be documented in at least the public methods

@sechkova
Copy link
Contributor Author

sechkova commented Jun 21, 2021

What I think is worth doing is giving the interested users, if any, information about the error. Following this thought maybe your proposal of deriving UnsupportedAlgorithmError from LengthOrHashMismatchError is a good compromise making everyone relatively happy.

@jku
Copy link
Member

jku commented Jun 21, 2021

What I think is worth doing is giving the interested users, if any, information about the error.

This is absolutely not contested: we should have actionable error messages.

But I would like to only add errors if they have a purpose: to understand which component might handle the error (the component may be a hypothetical one: I'd just like to understand the use case). Adding more complex errors in later releases is easy, removing existing ones is really hard.

Securesystemslib digest() and digest_fileobject()
calls raise sslib specific exceptions that need to be
handled and re-raised as TUF exceptions.

Updated tests in test_api.py accordingly.

Signed-off-by: Teodora Sechkova <[email protected]>
@sechkova sechkova force-pushed the hashes-handle-sslib-errors branch from c839884 to 752a741 Compare June 22, 2021 08:36
@sechkova sechkova marked this pull request as ready for review June 22, 2021 08:36
@sechkova
Copy link
Contributor Author

Kept only LengthOrHashMismatchError since this is easily extendable later and for consistency with #1435.
Rebased on develop, not a draft anymore.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks

tests/test_api.py Show resolved Hide resolved
@jku jku merged commit 7108ea2 into theupdateframework:develop Jun 23, 2021
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

Successfully merging this pull request may close these issues.

MetaFile/TargetFile: ensure securesystemslib errors are all handled
2 participants