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

Switch to Fulcio v2 API #159

Merged
merged 1 commit into from
Jul 20, 2022
Merged

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Jul 18, 2022

The main differences are:

  • RootCert -> TrustBundle now returns a list of chains
  • SigningCertificate differentiates between the embedded and detached
    SCT, and it's no longer returned in a header.
  • Support for gRPC, but this continues to use HTTP.

No changes necessary for the request construction, CertificateSigningRequest is still the name of the field on the request.

Signed-off-by: Hayden Blauzvern [email protected]

Summary

Release Note

Documentation

@haydentherapper
Copy link
Contributor Author

What's the best way to test this locally? I'm not familiar with the process for locally building and using packages.

@di
Copy link
Member

di commented Jul 18, 2022

@haydentherapper Do you want to run tests/linting locally, or invoke the client with your changes?

@di
Copy link
Member

di commented Jul 18, 2022

/gcbrun

@tetsuo-cpp tetsuo-cpp self-requested a review July 18, 2022 05:20
@haydentherapper
Copy link
Contributor Author

Invoke the client with my changes, I’m working on getting things set up for local linting.

@di
Copy link
Member

di commented Jul 18, 2022

You should be able to run:

$ python -m pip install -e .

to install the local source tree as an 'editable' install, meaning that invoking python -m sigstore anywhere will execute the files in your source tree every time.

@haydentherapper haydentherapper force-pushed the v2-fulcio branch 2 times, most recently from ae3b9e3 to 1c29aeb Compare July 19, 2022 03:39
@haydentherapper
Copy link
Contributor Author

Thanks @di, that worked!

Tested with python3 -m sigstore sign --identity-token <token> foo.txt, it worked as expected. Also tested trust_bundle manually by calling it in the code - There's a bug we're fixing in Fulcio that is concatenating all certificates together, so trust_bundle only returns the first cert, but this should be corrected once the Fulcio fix is rolled out.

Not certain how to get the ambient signing test working.

@tetsuo-cpp
Copy link
Contributor

tetsuo-cpp commented Jul 19, 2022

Thanks @haydentherapper!

Not certain how to get the ambient signing test working.

I can take a look at this tomorrow if you'd like.

@haydentherapper
Copy link
Contributor Author

@tetsuo-cpp Any help would be great! The error just says that ambient credentials aren't available, maybe it's just a flake? I see Dustin also filed a ticket to create a more actionable error message.

@di
Copy link
Member

di commented Jul 19, 2022

/gcbrun

@tetsuo-cpp
Copy link
Contributor

@tetsuo-cpp Any help would be great! The error just says that ambient credentials aren't available, maybe it's just a flake? I see Dustin also filed a ticket to create a more actionable error message.

According to this documentation, it's because this PR is based on a fork.

From the page:

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception to this behavior is where an admin user has selected the Send write tokens to workflows from pull requests option in the GitHub Actions settings.

Given that we use the OIDC identity for release signing, I'm not sure that allowing write tokens for PRs is the answer. Maybe we should reserve this job just for when commits make their way into main and drop them from PR testing?

What do you think @woodruffw @di?

@woodruffw
Copy link
Member

Given that we use the OIDC identity for release signing, I'm not sure that allowing write tokens for PRs is the answer. Maybe we should reserve this job just for when commits make their way into main and drop them from PR testing?

What do you think @woodruffw @di?

That works for me. Alternatively we could check whether the originator is a fork or not, and simply skip those tests if it is.

@di di mentioned this pull request Jul 20, 2022
@di
Copy link
Member

di commented Jul 20, 2022

Maybe we should reserve this job just for when commits make their way into main and drop them from PR testing?

This seems fine to me. Either way, we should probably come up with some way to notify if this starts failing: #161

@di
Copy link
Member

di commented Jul 20, 2022

/gcbrun

@di
Copy link
Member

di commented Jul 20, 2022

Looks like all tests are passing now, we just need to resolve #159 (comment)

@woodruffw
Copy link
Member

Looks like all tests are passing now, we just need to resolve #159 (comment)

I can take a stab at that in a moment.

@woodruffw woodruffw self-assigned this Jul 20, 2022
@haydentherapper
Copy link
Contributor Author

Thanks y'all!

@woodruffw, I can resolve this too, unless you've already started it.

@woodruffw
Copy link
Member

I can resolve this too, unless you've already started it.

Nope, go for it! I just realized that I probably can't push to your fork anyways 🙂

@haydentherapper haydentherapper force-pushed the v2-fulcio branch 2 times, most recently from e7b5b2f to a4e5ca0 Compare July 20, 2022 18:26
@haydentherapper
Copy link
Contributor Author

Comments resolved, I'm assuming I don't have the ability to invoke /gcbrun.

@woodruffw
Copy link
Member

/gcbrun

Verified

This commit was signed with the committer’s verified signature.
The main differences are:
* RootCert -> TrustBundle now returns a list of chains
* SigningCertificate differentiates between the embedded and detached
  SCT, and it's no longer returned in a header.
* Support for gRPC, but this continues to use HTTP.

Signed-off-by: Hayden Blauzvern <[email protected]>
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

/gcbrun

@woodruffw woodruffw merged commit 6993d75 into sigstore:main Jul 20, 2022
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.

None yet

4 participants