-
Notifications
You must be signed in to change notification settings - Fork 7
Drop protobufs, bump sigstore version #144
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
Conversation
* Update import for ClientTrustConfig * Use force_tlog_version = 1 when signing for now: This makes sure we don't get rekor v2 entries before we want them * tests: Update expected error message when using wrong instance
before this gets merged, I would like to make sure I understand how PyPI verifies these to ensure there are no unintended incompatibility issues: marking draft |
resp = requests.get( | ||
"https://raw.githubusercontent.com/sigstore-conformance/extremely-dangerous-public-oidc-beacon/refs/heads/current-token/oidc-token.txt" | ||
) |
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.
copying from the other PR:
unfortunately GitHub cached raw.githubusercontent.com quite aggressively when I last tried this -- it usually works but the error rate was still annoyingly high. This is why conformance uses the clumsy looking "git clone" approach https://github.com/sigstore/sigstore-conformance/blob/e0997c248c40ee615c1e8aa1e3ee043a62920951/test/conftest.py#L131
maybe this is not a show stopper here: we could do this and improve if it fails too often
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.
Yeah, not a blocker IMO. We can re-evaluate if the error rate ends up being annoying.
Thanks for the PR @jku ! This looks good to me at a first glance, I need to test it against
Here is where PyPI extracts and verifies the attestations uploaded by users, if you want to take a look |
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.
This LGTM. I tested the changes here against warehouse
, and everything works as expected. I'll wait a bit before merging in case @woodruffw wants to take a look
env: | ||
# Use the pubic OIDC beacon for online tests, rather than relying | ||
# on the workflow's own ID token. | ||
EXTREMELY_DANGEROUS_PUBLIC_OIDC_BEACON: 1 |
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.
Thanks a ton for doing this @jku!
trust_config = ClientTrustConfig.staging() if args.staging else ClientTrustConfig.production() | ||
# Make sure we use rekor v1 until attestations are compatible with v2 | ||
trust_config.force_tlog_version = 1 |
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.
Not an issue for this PR, but I kind of wish we had a public API for this operation that wasn't setting an attribute/property directly 😅 -- I can see us perhaps wanting to remove this attribute at some point.
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! Only nitpicks.
Summary
Effect on signing and verifying
force_tlog_version=1
) entries even when the sigstore instance starts to advertize rekor2 for signing clientsThis PR contains commits from #132 from @woodruffw and adds a few additional commits.
Closes #131, closes #141, replaces #132.