Skip to content

backend: auth: Extract ParseClusterAndToken from headlamp.go#3494

Merged
illume merged 1 commit intokubernetes-sigs:mainfrom
skoeva:auth3
Aug 26, 2025
Merged

backend: auth: Extract ParseClusterAndToken from headlamp.go#3494
illume merged 1 commit intokubernetes-sigs:mainfrom
skoeva:auth3

Conversation

@skoeva
Copy link
Contributor

@skoeva skoeva commented Jun 17, 2025

This change extracts the ParseClusterAndToken from headlamp.go into the new auth package.

Part of:

Updates

  • Handles case-insensitive Bearer and trims spaces
  • Reuses the compiled regex
  • Validates cluster contexts and tokens
  • Rejects multiple tokens and handles paths with no cluster

Testing

cd backend
go test -v -p 1 ./... -run TestParseClusterAndToken

@skoeva skoeva requested a review from illume June 17, 2025 22:38
@skoeva skoeva self-assigned this Jun 17, 2025
@skoeva skoeva added backend Issues related to the backend auth Authentication or authorization related oidc Issue related to OIDC labels Jun 17, 2025
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 17, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 17, 2025
@skoeva skoeva force-pushed the auth3 branch 2 times, most recently from 6f68878 to dada774 Compare June 18, 2025 13:43
Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

I like how you moved the regex compilation out of the function.

Looking at how shouldBypassOIDCRefresh uses the return values of this, I noticed above there was a check that the URL is a /clusters/ URL or not, but it's good to check in this function and a regex is being used anyway.

I found the spec for the "Authorization: Bearer" token is here: https://datatracker.ietf.org/doc/html/rfc6750#section-2.1 (Probably worth adding this link into the function documentation).

I wonder if we should add validation of the token characters in here? (see valid ones in the spec above). I feel like they should be validated... but is here the right spot?

Technically this is a cluster context we are taking in from the URL. Although there is ambiguity in the frontend UI between cluster names and context names, I think we should be specific in the backend and mention that we are returning a cluster context.

The other thing I was wondering if we should validate is the cluster context name? We do validate that in other parts of our code, so maybe here too?

@skoeva skoeva force-pushed the auth3 branch 6 times, most recently from b9864b7 to 51e1ad3 Compare June 20, 2025 13:23
@skoeva skoeva changed the title backend: auth: Extract ParseClusterAndBearerToken from headlamp.go backend: auth: Extract ParseClusterAndToken from headlamp.go Jun 20, 2025
@skoeva skoeva requested a review from illume June 20, 2025 14:04
@illume
Copy link
Contributor

illume commented Jul 2, 2025

Note: we should check all the different formats manually. #3494 (comment)

Also, makeDNSFriendly doesn't handle @ characters, and it should? Plus some other things ($ signs etc). These work in the URL, so we should let these characters validate.

In kubeconfig.go it loads all of kubeconfigs and replaces the context names with dns friendly ones. So we assume the cluster from the URL is dns friendly. (That is, the / and ' ' are converted to something that works in URLs).

@illume illume removed request for joaquimrocha and sniok July 3, 2025 07:08
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2025
This change extracts the ParseClusterAndToken from headlamp.go into
the new auth package.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2025
Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

🎉 thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: illume, skoeva

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2025
@illume illume merged commit 1797e0d into kubernetes-sigs:main Aug 26, 2025
6 of 7 checks passed
@skoeva skoeva deleted the auth3 branch September 2, 2025 14:04
@illume illume added this to the v0.35.0 milestone Sep 2, 2025
@illume illume added the quality Related to improving the quality of the app label Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. auth Authentication or authorization related backend Issues related to the backend cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. oidc Issue related to OIDC quality Related to improving the quality of the app size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

Development

Successfully merging this pull request may close these issues.

4 participants