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.

Sign up for GitHub

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

Jump to bottom

sign: set the oidc redirect uri #1675

Merged
merged 4 commits into from
Apr 1, 2022
Merged

sign: set the oidc redirect uri #1675

merged 4 commits into from
Apr 1, 2022

Conversation

hectorj2f
Copy link
Contributor

@hectorj2f hectorj2f commented Mar 28, 2022 •
edited
Loading

Summary

When using certain OIDC providers, you might need to set a specific a redirect_uri. This implementation defaults to the old mechanism setting a listener on localhost and letting the OS to pick a port if the redirect_uri is not set.

To set the redirect_uri value, I added a new flag named --oidc-redirect-uri.

This PR is related to sigstore/sigstore#353.

Ticket Link

Fixes #1105 and #1311

Release Note

Add oidc-redirect-url flag to specify the redirect url for the sign command

@hectorj2f hectorj2f self-assigned this Mar 28, 2022
@hectorj2f hectorj2f changed the title Set redirect uri sign: set the oidc redirect uri Mar 28, 2022
hectorj2f
hectorj2f commented Mar 28, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2022 •
edited
Loading

Codecov Report

Merging #1675 (9c5ec91) into main (9496416) will decrease coverage by 0.02%.
The diff coverage is 10.52%.

@@            Coverage Diff             @@
##             main    #1675      +/-   ##
==========================================
- Coverage   29.43%   29.40%   -0.03%     
==========================================
  Files         141      141              
  Lines        8413     8420       +7     
==========================================
  Hits         2476     2476              
- Misses       5668     5675       +7     
  Partials      269      269
Impacted Files Coverage Δ
cmd/cosign/cli/attest.go 0.00% <0.00%> (ø)
...cosign/cli/fulcio/fulcioverifier/fulcioverifier.go 23.17% <0.00%> (ø)
cmd/cosign/cli/options/oidc.go 0.00% <0.00%> (ø)
cmd/cosign/cli/policy_init.go 1.42% <0.00%> (-0.01%) ⬇️
cmd/cosign/cli/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go 14.57% <0.00%> (ø)
cmd/cosign/cli/sign/sign_blob.go 0.00% <ø> (ø)
cmd/cosign/cli/signblob.go 0.00% <0.00%> (ø)
cmd/cosign/cli/fulcio/fulcio.go 25.60% <25.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9496416...9c5ec91. Read the comment docs.

@hectorj2f hectorj2f force-pushed the set_redirect_uri branch 4 times, most recently from 35c6fa1 to d9f705c Compare March 29, 2022 11:03
@hectorj2f
Copy link
Contributor Author

I am failing similar issues to #1676 regarding the windows build's step 🤔 .

@dlorenc
Copy link
Member

dlorenc commented Mar 29, 2022

Cc @cpanato

haydentherapper
haydentherapper approved these changes Mar 29, 2022
View reviewed changes
cmd/cosign/cli/options/oidc.go Outdated Show resolved Hide resolved
cmd/cosign/cli/options/oidc.go Outdated Show resolved Hide resolved
@cpanato
Copy link
Member

cpanato commented Mar 30, 2022

see comment here #1676 (comment)

@hectorj2f
Copy link
Contributor Author

Thanks @cpanato, I'll follow the recommendation in #1676 (comment)

@hectorj2f hectorj2f force-pushed the set_redirect_uri branch 4 times, most recently from b167c26 to 9c5ec91 Compare March 30, 2022 19:06
@hectorj2f hectorj2f requested review from cpanato and removed request for haydentherapper March 30, 2022 22:29
cpanato
cpanato reviewed Mar 31, 2022
View reviewed changes
go.mod Outdated Show resolved Hide resolved
@hectorj2f hectorj2f force-pushed the set_redirect_uri branch 3 times, most recently from e21cd0d to 2bed7a9 Compare March 31, 2022 17:42
@dlorenc
Copy link
Member

dlorenc commented Mar 31, 2022

Is this ready for merge?

@haydentherapper
Copy link
Contributor

We can probably wait til end of week to see if the Let's Encrypt PR gets merged?

Was there a reason we needed the sigstore fork?

@hectorj2f
Copy link
Contributor Author

We can probably wait til end of week to see if the Let's Encrypt PR gets merged?

sgtm

Was there a reason we needed the sigstore fork?

@cpanato made similar changes to sigstore/sigstore regarding the boulder package.

@haydentherapper
Copy link
Contributor

If we override boulder in Cosign, I don't think we need the sigstore/sigstore fork too.

@haydentherapper
Copy link
Contributor

It's been merged! letsencrypt/boulder#6029 Yay!

@dlorenc
Copy link
Member

dlorenc commented Mar 31, 2022

Nice! this should be good for a merge after tests pass.

@dlorenc dlorenc merged commit 286bb0c into sigstore:main Apr 1, 2022
@github-actions github-actions bot added this to the v1.7.0 milestone Apr 1, 2022
@hectorj2f hectorj2f deleted the set_redirect_uri branch April 1, 2022 08:37
@hectorj2f
Copy link
Contributor Author

Thanks ;)

mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
@mlieberman85
sign: set the oidc redirect uri (sigstore#1675)
145f6cf 
* oidc: set redirect uri if needed

Signed-off-by: hectorj2f <[email protected]>

* docs: add oidc-redirect-uri optional flag

Signed-off-by: hectorj2f <[email protected]>

* cmd: add the oidc redirect uri to key.Opts

Signed-off-by: hectorj2f <[email protected]>

* add missing third_party packages

Signed-off-by: hectorj2f <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpanato cpanato cpanato left review comments

@haydentherapper haydentherapper haydentherapper approved these changes

@dlorenc dlorenc dlorenc approved these changes

@mattmoor mattmoor Awaiting requested review from mattmoor

Assignees

@hectorj2f hectorj2f

Labels
None yet
Projects
None yet
Milestone
v1.7.0
Development

Successfully merging this pull request may close these issues.

random callback uri port
5 participants
@hectorj2f @codecov-commenter @dlorenc @cpanato @haydentherapper