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

Make VerificationError available in the API #691

Closed
mayaCostantini opened this issue Jun 27, 2023 · 6 comments
Closed

Make VerificationError available in the API #691

mayaCostantini opened this issue Jun 27, 2023 · 6 comments
Labels
enhancement New feature or request
Milestone

Comments

@mayaCostantini
Copy link
Contributor

mayaCostantini commented Jun 27, 2023

Description

Make VerificationError defined in sigstore/_cli.py available in the verification API as it could be reused outside of the CLI.
See #692 for example implementation.

@woodruffw
Copy link
Member

Thanks for filing this!

Could you say a bit more about your intended use case here? VerificationError really only gets used in the CLI, since the public and private APIs prefer to use the result-type-style VerificationResult/VerificationFailure instead.

@mayaCostantini
Copy link
Contributor Author

This would be mostly for reuse in other CLIs that wrap sigstore-python functionalities. It also seemed to me like a generic error that could possibly be reused elsewhere by API users to raise from instead of returning a result type value.

@woodruffw
Copy link
Member

This would be mostly for reuse in other CLIs that wrap sigstore-python functionalities. It also seemed to me like a generic error that could possibly be reused elsewhere by API users to raise from instead of returning a result type value.

Making sure I understand: by other CLIs, do you mean other Python programs that are importing sigstore._cli and re-using its internals? Doing so is understandable, but I'd prefer (for now) not to encourage that -- the _cli module and its interfaces are private and aren't considered part of sigstore-python's semver contract, meaning that anybody who relies on them will experience frequent breakage as we move things around 🙂

Overall, I think I'd prefer if we stick to the result-style API as the primary source of error state in the public APIs -- it's arguably less Pythonic, but it allows us to encode the error states in the type system rather than in an unchecked exception hierarchy. My experience with other end-user cryptography APIs (like pyjwt) is that using an exception hierarchy for these things inevitably leads to exception type leakage, which in turn means that users end up doing things like this:

try:
   something()
except OurType:
   normal_failure_mode()
except Exception:
   weird_failure_mode()

...which in turn indicates a porous API and means that we can't make semver guarantees around error states, like we can currently.

Does that reasoning make sense to you? If so, I'm inclined to close #692 and keep these exception types private; IMO it might also make sense to add some documentation emphasizing the technical rationale above 🙂

@mayaCostantini
Copy link
Contributor Author

do you mean other Python programs that are importing sigstore._cli and re-using its internals?

I meant other Python CLIs importing from the sigstore-python stable API. The idea was to be able to import the VerificationError defined here from the API rather than implementing the same / a custom one in case those tools need some exception to fail on directly.

My experience with other end-user cryptography APIs (like pyjwt) is that using an exception hierarchy for these things inevitably leads to exception type leakage

Ok I was not aware that this type of issue could arise, so it makes sense to keep returning a result type value in this case. Thanks for the detailed answer (worth documenting indeed), closing #692 then 👍

@woodruffw
Copy link
Member

Sounds good! I'll repurpose this issue to track that documentation.

@woodruffw
Copy link
Member

FYI @mayaCostantini: we've refactored the verification APIs to be exception driven, so VerificationError will be part of the public API starting in the 3.x series 🙂

xref #959

@woodruffw woodruffw added this to the 3.0 milestone Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants