-
Notifications
You must be signed in to change notification settings - Fork 419
backend: headlamp: Add pkce support #3692
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: k-airos The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @k-airos! |
76a41bf
to
eb171ed
Compare
Thanks @k-airos ! I asked in the issue for feedback, and ran the CI checks. We'd need to documentation into the headlamp helm chart at some point (README.md, values.yaml and values.schema. Let me know if you'd like to add it in this PR, or we can do it after this is merged. |
Thanks! I've added the documentation to README.md, updated values.yaml and values.schema.json accordingly. |
@k-airos much appreciated. I see Erik from the issue gave a thumbs up. Let's wait a little bit to see if anyone else from that issue has feedback. I see there's an error in CI:
To test this locally you can run: |
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.
Pull Request Overview
This PR adds PKCE (Proof Key for Code Exchange) support to the OIDC authentication flow in Headlamp, enhancing security for OAuth 2.0 authorization code flows. PKCE is particularly important for public clients and helps mitigate authorization code interception attacks.
- Added PKCE configuration option with default value of false
- Implemented cryptographic functions for PKCE code verifier and challenge generation
- Updated OIDC flow to conditionally use PKCE parameters during authorization and token exchange
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
charts/headlamp/values.yaml | Added usePKCE configuration option with default false |
charts/headlamp/values.schema.json | Added schema definition for usePKCE boolean field |
charts/headlamp/templates/secret.yaml | Added usePKCE to secret template |
charts/headlamp/templates/deployment.yaml | Added OIDC_USE_PKCE environment variable handling |
charts/headlamp/README.md | Updated documentation to include usePKCE configuration |
backend/pkg/config/config.go | Added OidcUsePKCE field and flag definition |
backend/cmd/server.go | Added oidcUsePKCE to server configuration |
backend/cmd/headlamp.go | Implemented PKCE cryptographic functions and updated OAuth flow |
Comments suppressed due to low confidence (1)
backend/cmd/headlamp.go:264
- [nitpick] The default value for PKCE should be
true
rather thanfalse
. PKCE is a security enhancement recommended by RFC 7636 and OAuth 2.1, and there's no significant downside to enabling it by default. Consider changing the default to improve security posture.
case strings.HasPrefix(r.Host, "localhost:") || r.TLS == nil:
@@ -213,6 +224,10 @@ spec: | |||
# Check if useAccessToken is non false either from env or oidc.config | |||
- "-oidc-use-access-token=$(OIDC_USE_ACCESS_TOKEN)" | |||
{{- end }} | |||
{{- if or (ne ($oidc.usePKCE | toString) "false") (ne $usePKCE "") }} |
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 variable $usePKCE
is referenced but not defined in this context. This appears to be a copy-paste error from the useAccessToken logic above. It should likely be ($oidc.usePKCE | toString)
or the environment variable check should be removed.
{{- if or (ne ($oidc.usePKCE | toString) "false") (ne $usePKCE "") }} | |
{{- if or (ne ($oidc.usePKCE | toString) "false") (ne $usePKCE "false") }} |
Copilot uses AI. Check for mistakes.
oauth2Token, err = oauthConfig.Config.Exchange( | ||
oauthConfig.Ctx, | ||
r.URL.Query().Get("code"), | ||
oauth2.SetAuthURLParam("code_verifier", oauthConfig.CodeVerifier), |
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 oauth2.SetAuthURLParam
function is being used incorrectly for token exchange. For PKCE token exchange, you should use oauth2.SetAuthURLParam
with the Exchange
method, but the parameter should be passed as an option to Exchange
, not as an auth URL parameter. Use oauth2.SetAuthURLParam("code_verifier", oauthConfig.CodeVerifier)
as an option to the Exchange
call.
Copilot uses AI. Check for mistakes.
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.
I tried testing this in in-cluster and the app doesn't work without the client-secret because of this client-secret check. We have to make client-secret optional in the kubconfig.OidcConfig for the user to make headlamp work without client-secret.
headlamp/backend/pkg/kubeconfig/kubeconfig.go
Line 911 in 68e7477
if oidcClientID != "" && oidcClientSecret != "" && oidcIssuerURL != "" && oidcScopes != "" { |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Summary
Add PKCE support for OIDC authentication flow
Related Issue
Fixes #3137
Changes