-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
What's the best way to test this locally? I'm not familiar with the process for locally building and using packages. |
@haydentherapper Do you want to run tests/linting locally, or invoke the client with your changes? |
/gcbrun |
Invoke the client with my changes, I’m working on getting things set up for local linting. |
You should be able to run:
to install the local source tree as an 'editable' install, meaning that invoking |
ae3b9e3
to
1c29aeb
Compare
Thanks @di, that worked! Tested with Not certain how to get the ambient signing test working. |
Thanks @haydentherapper!
I can take a look at this tomorrow if you'd like. |
@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. |
/gcbrun |
1c29aeb
to
031917f
Compare
According to this documentation, it's because this PR is based on a fork. From the page:
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 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. |
This seems fine to me. Either way, we should probably come up with some way to notify if this starts failing: #161 |
/gcbrun |
Looks like all tests are passing now, we just need to resolve #159 (comment) |
I can take a stab at that in a moment. |
Thanks y'all! @woodruffw, 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 🙂 |
e7b5b2f
to
a4e5ca0
Compare
Comments resolved, I'm assuming I don't have the ability to invoke |
/gcbrun |
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]>
a4e5ca0
to
3adfbf0
Compare
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!
/gcbrun |
The main differences are:
SCT, and it's no longer returned in a header.
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