Idp specific default flows

[houdini91]

Suggestion:
Adding interactive flows for each specific identity provider, allowing users to skip the main idp selection page.
I think one less click can improve the UX a bit.
Further UX improvement is gained when browser uses a default idp account the user does may not need to interactively intervene at all.
Such psodo-auto flow may also be valuable in automation uses cases, for example a git hook signing SLSA provenance.

Hope this helps .
mikey strauss

[bobcallaway]

This is an interesting idea, but hard-codes the current deployment state of the public service into the (hopefully) more generic codebase, which I'd generally like to avoid. If we were to switch to a different provider (e.g. keycloak), this would require code changes to adapt.

I think you can append connector_id=github-sigstore-prod as a query parameter to the configured auth URL to automatically select github.

[houdini91]

@bobcallaway
Hi,
I agree your suggestion is better (And also works), let me see if understood correctly.
I changed InteractiveIDTokenGetter to support extra options to be added to the auth URL.
Added example flows as usage example of the change (should i delete the examples so there is no hardcoded values of the public service?, maybe i can move them somewhere else in code base?).

Or maybe you meant application using Sigstore library will implement there own OIDConnect function, with the modified auth URL?

[bobcallaway]

I'm good with the additions to the InteractiveTokenGetter struct (minus the nit about the field name). Are you thinking we should leave in the changes in pkg/oauthflow/flow.go or were they there just as an example?

[houdini91]

@bobcallaway
Fixed your comments,
flow.go are working objects (on the public instance) and may be useful for library users (such as cosign).
I would leave them if it was up to me, maybe find a other place to import the public instance specific url option and constsants.
I thought you preferred i don't add that part to the pull-request, leaving the implementation to the library users.
What do you think?

[houdini91]

@bobcallaway
Removed the flow.go changes, it may help speed up the review.

[houdini91]

Turns out #sigstore/fulcio#204 broke the connector_id argument.
I am closing this PR.

[bobcallaway]

the connector IDs did change, but I think this patch is still valid if we update them to match...

[houdini91]

@bobcallaway
If you think it's still possible I be happy to continue.
Can you point me on the right direction?

[houdini91]

@bobcallaway
i think understand correct me if i am wrong.

I can use the new connector_ids values to choose between idps.
I tested the new Ids and they seem to work.

https://github.com/login/oauth
https://accounts.google.com
https://login.microsoftonline.com

After the change do the Default(privider)IDTokenGetter make more sense as part of the code base?

[bobcallaway]

After the change do the Default(privider)IDTokenGetter make more sense as part of the code base?

I think its fine if you want to put them in.

[bobcallaway]

Perhaps we should add some comments to these (as well as updating the names) to denote that they are only meant to be used with oauth2.sigstore.dev ; otherwise this LGTM

[houdini91]

I updated a bit of names and comments
Did you mean renaming the Default[provider]IDTokenGetter as well?
Is there any specific naming prefix you would rather?
PublicInstance, SigstoreDev, .. ?

[bobcallaway]

I'm fine with PublicInstance as you propose here. I think with renaming the IDTokenGetters we'd be ready to merge this. Thanks for the persistence on getting this to the finish line :)

[houdini91]

No problem, its my pleasure thanks for the review.
Its my first contribution slowly i hope to have help out.
If you like you can take a look at other PR's i have open on cosign and go-securesystemslib.