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

Add support to PKCE in OIDC connector #3777

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

johnvan7
Copy link

@johnvan7 johnvan7 commented Oct 2, 2024

Overview

This PR adds support for PKCE in the OIDC connector.

PKCE (Proof Key for Code Exchange) - RFC 7636 - is an extension to the Authorization Code flow to prevent CSRF (Cross-Site Request Forgery) and authorization code injection attacks.

PKCE Flow

                                             +-------------------+
                                             | Upstream Connector|
   +--------+                                | +---------------+ |
   |        |--(A)- Authorization Request ---->|               | |
   |        |       + t(code_verifier), t_m  | | Authorization | |
   |        |                                | |    Endpoint   | |
   |        |<-(B)---- Authorization Code -----|               | |
   |        |                                | +---------------+ |
   |   dex  |                                |                   |
   |        |                                | +---------------+ |
   |        |--(C)-- Access Token Request ---->|               | |
   |        |          + code_verifier       | |    Token      | |
   |        |                                | |   Endpoint    | |
   |        |<-(D)------ Access Token ---------|               | |
   +--------+                                | +---------------+ |
                                             +-------------------+

The PKCE flow requires a code challenge to be passed to the authorization endpoint and a code verifier to be sent to the token endpoint.

What this PR does / why we need it

Dex currently does not support PKCE flow for upstream OIDC providers.
However, some identity providers (IdPs) require PKCE flow support to complete the authentication process.

Key features

  • It uses dynamic configuration through OAuth2 discovery metadata (supports S256 and plain).
  • It is possible (but optional) to specify an algorithm to use in the PKCEChallenge key of the connector's config.
  • The generated code is saved in the connectorData of the auth request in the database, allowing the management of multiple PKCE connections simultaneously across multiple running instances of dex.

Does this PR introduce a user-facing change?

  • Adds the optional PKCEChallenge configuration option to the OIDC connector to specify an algorithm to use in the PKCE flow.

Special notes for your reviewer

Related PRs:

Related discussions:

@ClaudioNTT
Copy link

ClaudioNTT commented Oct 2, 2024

I've tested the PR, and it works as expected, using the PKCE flow with OAuth0 and PingFederate
@nabokihms @johnvan7 @cameronbrunner

@kratosmat
Copy link

Hi Giovanni,
I hope you receive the approval, we also need this feature, thanks a lot.

connector/oidc/oidc.go Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
connector/oidc/oidc.go Outdated Show resolved Hide resolved
@nabokihms
Copy link
Member

nabokihms commented Oct 18, 2024

Could you also please sign your commits? DCO will not pass otherwise

@johnvan7 johnvan7 changed the title Add support to PKCE in OIDC connector Add support to PKCE in OIDC connector Signed-off-by: johnvan7 <giovanni.vella98@gmail.com> Oct 19, 2024
@johnvan7 johnvan7 changed the title Add support to PKCE in OIDC connector Signed-off-by: johnvan7 <giovanni.vella98@gmail.com> Add support to PKCE in OIDC connector Oct 19, 2024
@johnvan7 johnvan7 force-pushed the feat/pkce-challenge-support branch 2 times, most recently from 4c63988 to 6eee8c1 Compare October 19, 2024 15:17
@johnvan7
Copy link
Author

@nabokihms I've done the requested changes and signed my commits.
I've added pkceVerifierCache to handle multiple concurrent connections, inspired by PR #3195.

connector/oidc/oidc.go Outdated Show resolved Hide resolved
Signed-off-by: johnvan7 <giovanni.vella98@gmail.com>
Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
Signed-off-by: johnvan7 <giovanni.vella98@gmail.com>
Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
Signed-off-by: johnvan7 <giovanni.vella98@gmail.com>
Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
Signed-off-by: johnvan7 <giovanni.vella98@gmail.com>
Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
@nabokihms
Copy link
Member

@johnvan7 could you please investigate the issue with tests and the lint?

Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
@johnvan7
Copy link
Author

@nabokihms could you please restart workflows?
It should be fixed

@nabokihms
Copy link
Member

@sagikazarmark could you please take a look? There is an important change to the rock solid interface. Are you OK with this?

Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
Signed-off-by: Giovanni Vella <giovanni.vella98@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants