-
Notifications
You must be signed in to change notification settings - Fork 60
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 PKCE to OIDC Auth #188
Conversation
path_oidc.go
Outdated
@@ -454,6 +454,13 @@ func (b *jwtAuthBackend) createOIDCRequest(config *jwtConfig, role *jwtRole, rol | |||
|
|||
if config.hasType(responseTypeIDToken) { | |||
options = append(options, oidc.WithImplicitFlow()) | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a strict type for PKCE response type? The else here seems too broad, especially if we add more flows in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to test for responseTypeCode
since PKCE only (?) applied there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RFC says that "clients utilizing the Authorization Code Grant are susceptible to the authorization code interception attack". So I agree we should just check for responseTypeCode
. I will update the PR. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add PKCE to OIDC authorization code logins * Add tests * fix comment typos * check for code response type Co-authored-by: Jim Kalafut <jkalafut@hashicorp.com>
Overview
Add PKCE to OIDC Auth
Design of Change
use the cap library to set the pkce option
Contributor Checklist
[x] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible
Tests