feat: Support PKCE in OIDC connector #3188
Conversation
Some OIDC providers require the PKCE flow, so a code challenge must be passed to the authorize endpoint, and a code verifier must be sent on the subsequent authorization token exchange. This adds the configuration option `enablePKCE` to the OIDC connector to enable these PKCE parameters. Signed-off-by: Anshul Sirur <[email protected]>
vixus0
force-pushed
the
connector-oidc-pkce
branch
from
ac34641 to
32afd90
Compare
vixus0
changed the title
vixus0
changed the title
|
I did a similar implementation here: #3195 with the biggest difference being that mine generates a new verifier for each login session. As 'oauth2.GenerateVerifier()' just gets a single random string it is stated that a new one should be done for each authorization: https://cs.opensource.google/go/x/oauth2/+/refs/tags/v0.15.0:pkce.go;l=22 |
|
Sorry I missed your PR when I did mine... I had been thinking about it prior to your work and missed this one when I started mine. |
| @@ -184,6 +187,11 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e | |||
| c.PromptType = "consent" | |||
| } | |||
|
|
|||
| pkceVerifier := "" | |||
| if c.EnablePKCE { | |||
There was a problem hiding this comment.
Do you know if we need an option for this? We can rely on what the provider exposes via the discovery endpoint.
"code_challenge_methods_supported": [
"S256",
"plain"
],
If the code_challenge_methods_supported section is present, we can use PKCE.
There was a problem hiding this comment.
@vixus0 my bad, I overlooked the info from your first post
Dynamic configuration using the OAuth2 discovery metadata isn't possible as the coreos/go-oidc library currently doesn't expose code_challenge_methods_supported in the Provider.
Let's leave a comment in the code about this for other developers.
There was a problem hiding this comment.
So, after checking the docs once again, there is an option to extract the provider's metadata by using https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#Provider.Claims
|
|
||
| if c.pkceVerifier != "" { | ||
| c.pkceVerifier = oauth2.GenerateVerifier() | ||
| opts = append(opts, oauth2.S256ChallengeOption(c.pkceVerifier)) |
There was a problem hiding this comment.
S256 can be unsupported by the provider. It is required to either clarify that Dex only supports the S256 challenge or provide support for others.
We can rename the option EnablePKCE to PKCEChallenge instead of the boolean variable and let users specify the type of challenge they want to use.
| @@ -265,6 +275,12 @@ func (c *oidcConnector) LoginURL(s connector.Scopes, callbackURL, state string) | |||
| if s.OfflineAccess { | |||
| opts = append(opts, oauth2.AccessTypeOffline, oauth2.SetAuthURLParam("prompt", c.promptType)) | |||
| } | |||
|
|
|||
| if c.pkceVerifier != "" { | |||
| c.pkceVerifier = oauth2.GenerateVerifier() | |||
There was a problem hiding this comment.
Isn't this setting just one verifier for all clients? It appears that if 2 clients were logging in at the same time the first one's verifier would be overwritten by the seconds preventing use of the proper verifier during token exchange (line 317-320).
|
Hello everyone, will the merge request be approved? When will it be merged? |
|
Hi, I've tested this feature and it works well! |
very good!! same here, I've tested with Ping Federate and it works perfectly @nabokihms |
|
This pr cannot be merged I’m afraid:
If anybody wants to finish the work, feel free to do this |
|
Unfortunately I wasn't able to dedicate any more time to these changes, but I'm glad the feature was picked up again! Closing in favour of #3777. |
Overview
This adds a configuration option
enablePKCEto the OIDC connector to send code challenge and verifier parameters as required for PKCE.What this PR does / why we need it
Some OIDC providers require the PKCE flow, so a code challenge must be passed to the authorize endpoint, and a code verifier must be sent on the subsequent authorization token exchange.
Dex currently supports PKCE for downstream clients, but not for upstream OIDC providers.
Dynamic configuration using the OAuth2 discovery metadata isn't possible as the
coreos/go-oidclibrary currently doesn't exposecode_challenge_methods_supportedin theProvider.Special notes for your reviewer
Does this PR introduce a user-facing change?