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

identity/oidc: Adds proof key for code exchange (PKCE) support #13917

Merged
merged 14 commits into from
Feb 15, 2022

Conversation

austingebauer
Copy link
Contributor

@austingebauer austingebauer commented Feb 5, 2022

Overview

This PR adds proof key for code exchange (PKCE) support to OIDC providers. Details on PKCE are available in rfc7636.

The following table shows the behavior given various PKCE-related inputs. "Present" means that the parameter was provided to the API. "Absent" means the parameter was not provided to the API or is empty. Some of this behavior isn't defined by the RFC, so I'm flexible in changing it based on feedback.

This also adds the concept of public and confidential clients. Confidential clients are the default type and may use PKCE. Public clients do not have a client secret and are required to use PKCE.

Authorization Endpoint

code_challenge code_challenge_method behavior
Absent Absent PKCE not used
Absent Present PKCE not used
Present Absent PKCE used with default method from rfc7636 (plain)
Present Present PKCE used with given method

Token Endpoint

code_verifier PKCE used in authorization behavior
Absent No PKCE not used (code_verifier not verified)
Absent Yes invalid_request error
Present No invalid_request error
Present Yes code_verifier is verified using method

Testing

I've added tests that exercise PKCE using the hashicorp/cap OIDC client. I've also added some tests to verify that errors are returned when expected.

Documentation will follow in a separate PR.

@vercel vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 19:09 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 7, 2022 19:09 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 7, 2022 21:10 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 7, 2022 21:10 Inactive
vault/identity_store_oidc_provider.go Show resolved Hide resolved
vault/identity_store_oidc_provider.go Outdated Show resolved Hide resolved
vault/identity_store_oidc_provider.go Outdated Show resolved Hide resolved
@calvn
Copy link
Contributor

calvn commented Feb 8, 2022

Should there be a param on the OIDC provider config endpoint to enforce PKCE? Asking since there's an expected error in the spec if clients don't provide a code_challenge to a provider that requires it.

@vercel vercel bot temporarily deployed to Preview – vault February 9, 2022 23:19 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 23:19 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 9, 2022 23:31 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 23:31 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 9, 2022 23:41 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 9, 2022 23:41 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 11, 2022 21:06 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 11, 2022 21:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 11, 2022 21:21 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 11, 2022 23:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 11, 2022 23:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 14, 2022 23:58 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 14, 2022 23:58 Inactive
Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@vercel vercel bot temporarily deployed to Preview – vault-storybook February 15, 2022 06:57 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 15, 2022 06:57 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 15, 2022 18:19 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 15, 2022 18:19 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 15, 2022 19:01 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 15, 2022 19:01 Inactive
@austingebauer austingebauer merged commit e6a5730 into main Feb 15, 2022
@austingebauer austingebauer deleted the oidc-pkce branch February 15, 2022 20:02
Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got around to this. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants