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

TrustUpdater error handling #525

Merged
merged 7 commits into from
Mar 9, 2023

Conversation

tnytown
Copy link
Collaborator

@tnytown tnytown commented Mar 6, 2023

Summary

As discussed on #357, TrustUpdater does not have a coherent error handling scheme. The following exceptions are exposed to users:

  • OSError, when file read/write issues occur
  • TUF's DownloadError, when TUF fails to download metadata / target files
  • TUF's RepositoryError, when TUF metadata is invalid
  • Exception, raised when we don't find the entries we expect in TUF metadata

This PR wraps TUF's error types with our own TUFError, which will help us provide useful diagnostics to users when a TUF-internal issue occurs. Additionally, MetadataError is raised when the get methods on TrustUpdater don't find what they need.

Also in this PR is the structure for a new global error handling system. The Error type implements utilities, namely print_and_exit, for providing diagnostics to users. TUFError extends from this type.

Further work might include adapting existing error types, e.g. CertificateVerificationFailure, to this model. Doing this might make it easier for API consumers to handle Sigstore exceptions and allow us to export all of our error types from one place.

Resolves #357.

Release Note

  • A new error handling system was added. Diagnostic messages are now printed for certain errors.

@woodruffw
Copy link
Member

CC @jku as well 🙂

@tnytown tnytown force-pushed the andrew/exception-handling branch 3 times, most recently from eed4a94 to fd05543 Compare March 7, 2023 17:30
@tnytown tnytown marked this pull request as ready for review March 7, 2023 17:31
woodruffw
woodruffw previously approved these changes Mar 7, 2023
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM

@woodruffw
Copy link
Member

Hmm, not sure why the lint broke all of a sudden:

sigstore/verify/models.py:243: error: Unexpected keyword argument "log_index" for "LogInclusionProof"; did you mean "logIndex"?  [call-arg]
sigstore/verify/models.py:243: error: Unexpected keyword argument "root_hash" for "LogInclusionProof"; did you mean "rootHash"?  [call-arg]
sigstore/verify/models.py:243: error: Unexpected keyword argument "tree_size" for "LogInclusionProof"; did you mean "treeSize"?  [call-arg]
sigstore/verify/verifier.py:240: error: Argument "artifact_hash" to "LogEntryMissing" has incompatible type "str"; expected "HexStr"  [arg-type]
Found 4 errors in 2 files (checked 27 source files)
make: *** [Makefile:65: lint] Error 1
Error: Process completed with exit code 2.

@tnytown
Copy link
Collaborator Author

tnytown commented Mar 7, 2023

Yeah, it's working on my local machine (??) Can you trigger a rerun?

@woodruffw
Copy link
Member

woodruffw commented Mar 7, 2023

Yeah, it's working on my local machine ?? Can you trigger a rerun?

Yep, triggered. My guess is that it's a version thing -- IIRC we run the lints against 3.7 locally, but the latest stable Python in CI.

Edit: Spoke too soon -- we also use 3.7 in CI. Maybe a mypy update broke things?

@woodruffw
Copy link
Member

Maybe a mypy update broke things?

Yep, this is it -- mypy had their first stable release and it looks like some things changed. I'll do a separate PR for that.

@woodruffw woodruffw mentioned this pull request Mar 7, 2023
@woodruffw
Copy link
Member

Blocked on #530.

@tnytown
Copy link
Collaborator Author

tnytown commented Mar 7, 2023

Signed-off-by: Andrew Pan <a@tny.town>
sigstore/_errors.py Outdated Show resolved Hide resolved
Signed-off-by: William Woodruff <william@yossarian.net>
@woodruffw
Copy link
Member

/gcbrun

@woodruffw
Copy link
Member

Lint is failing on some APIs missing docstrings (we might need to add an exclude rule somewhere, since these are private APIs?)

Files skipped (0):
RESULT: FAILED (minimum: 100.0%, actual: 97.5%)
make: *** [Makefile:65: lint] Error 1

Signed-off-by: Andrew Pan <a@tny.town>
@tnytown
Copy link
Collaborator Author

tnytown commented Mar 9, 2023

Took a cursory glance at the interrogate documentation. It seems like we already have ignore-semiprivate enabled, which should exclude the _errors module. Maybe the ignore doesn't nest, i.e. a "public" member of a semiprivate module still has to be documented?

I'm adding documentation to stuff added by this PR to pass the lint, but maybe we should look into this in the future.

@woodruffw
Copy link
Member

Took a cursory glance at the interrogate documentation. It seems like we already have ignore-semiprivate enabled, which should exclude the _errors module. Maybe the ignore doesn't nest, i.e. a "public" member of a semiprivate module still has to be documented?

Yeah, that smells like a bug to me (or, at least a documentation deficiency). Could you raise an upstream issue with them?

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM structurally, needs a CHANGELOG entry 🙂

@tnytown
Copy link
Collaborator Author

tnytown commented Mar 9, 2023

Yeah, that smells like a bug to me (or, at least a documentation deficiency). Could you raise an upstream issue with them?

Will do!

LGTM structurally, needs a CHANGELOG entry 🙂

Added to #525 (comment) :) Do we want a different entry for #531?

@woodruffw
Copy link
Member

Do we want a different entry for #531?

Either a separate entry or adding to this one works for me!

woodruffw
woodruffw previously approved these changes Mar 9, 2023
@woodruffw
Copy link
Member

/gcbrun

Signed-off-by: Andrew Pan <a@tny.town>
@woodruffw
Copy link
Member

/gcbrun

@woodruffw woodruffw merged commit b606b6c into sigstore:main Mar 9, 2023
@woodruffw woodruffw deleted the andrew/exception-handling branch March 9, 2023 16:52
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.

_internal.tuf: exception handling
2 participants