-
Notifications
You must be signed in to change notification settings - Fork 2
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
remove Verifier param from verify() API #62
Conversation
This removes a foreign type by instead constructing the Verifier under the hood within verify(). Signed-off-by: William Woodruff <william@trailofbits.com>
WIP. Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Signed-off-by: William Woodruff <william@trailofbits.com>
Co-authored-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I only left a naming suggestion/question
src/pypi_attestations/_impl.py
Outdated
policies: list[VerificationPolicy] = [ | ||
policy.OIDCIssuerV2("https://token.actions.githubusercontent.com"), | ||
policy.OIDCSourceRepositoryURI(f"https://github.com/{self.repository}"), | ||
_GitHubTrustedPublisherPolicy(self.repository, self.workflow), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the name _GitHubTrustedPublisherPolicy
suggests that it's, by itself, the full policy for GitHub. Maybe we could rename it to something like _GitHubBuildConfigURIPolicy
?
Or alternatively, make _GitHubTrustedPublisherPolicy
return the full AllOf(OIDCIssuer,OIDCSourceRep,...)
policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, good idea -- I'll make it contain the AllOf
internally.
Signed-off-by: William Woodruff <william@trailofbits.com>
WIP;See #55 (comment) for motivating context.This removes a foreign type by instead constructing the Verifier under the hood within verify().
Key changes:
Attestation.verify()
now constructs the verifier internally, based on thestaging
kwarg.Attestation.verify()
can now takePublisher
s, which are converted into their appropriate policies under the hood. Policies can also still be passed in directly.Publisher
variants now have_as_policy()
, which is a private API for the publisher to verification policy transform.This will hopefully make things easier for downstream users, particularly people who are pulling provenance objects from PyPI via PEP 740 and trying to verify them (but are getting stuck on the transforms between the "PEP 740" and "Sigstore" views).