🐛 Enforce HS256-only JWT parsing — fix for TAG-Security review#4356
🐛 Enforce HS256-only JWT parsing — fix for TAG-Security review#4356clubanderson merged 1 commit intomainfrom
Conversation
All JWT parsing now goes through middleware.ParseJWT() which uses a shared
jwt.Parser configured with jwt.WithValidMethods([]string{"HS256"}).
Previously, ParseWithClaims was called without algorithm restriction — the
library's default accepts any signing method. This could theoretically
allow algorithm confusion attacks (e.g., HS384, RS256-with-HMAC-key).
Defense-in-depth: the keyfunc also explicitly checks token.Method is
*jwt.SigningMethodHMAC before returning the secret.
Four call sites consolidated:
- JWTAuth middleware (HTTP API)
- ValidateJWT (WebSocket/exec)
- RefreshToken handler
- Logout handler
Signed-off-by: Andrew Anderson <andy@clubanderson.com>
|
/lgtm |
|
@clubanderson: you cannot LGTM your own PR. DetailsIn response to this:
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. |
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clubanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clubanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole canceled.
|
kubestellar-prow
bot
added
the
approved
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/M
There was a problem hiding this comment.
Pull request overview
Consolidates JWT parsing/validation into a single middleware helper that enforces HS256-only parsing to prevent JWT algorithm confusion attacks, aligning implementation with the stated TAG-Security posture.
Changes:
- Introduces
middleware.ParseJWT()backed by a sharedjwt.Parserconfigured withWithValidMethods([]string{"HS256"})plus a defense-in-depth HMAC method check. - Updates all production call sites that previously used
jwt.ParseWithClaims()directly to usemiddleware.ParseJWT()instead.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/api/middleware/auth.go | Adds HS256-only shared parser and ParseJWT(); updates middleware validation paths to call it. |
| pkg/api/handlers/auth.go | Updates logout/refresh handlers to parse tokens via middleware.ParseJWT() for consistent algorithm enforcement. |
| var jwtParser = jwt.NewParser(jwt.WithValidMethods([]string{"HS256"})) | ||
|
|
||
| // ParseJWT parses and validates a JWT token using the shared HS256-only parser. | ||
| // All JWT validation in the codebase should use this function (or the JWTAuth |
There was a problem hiding this comment.
The docstring claims "All JWT validation in the codebase" should use ParseJWT/JWTAuth, but there is at least one remaining direct jwt.ParseWithClaims usage in tests (pkg/api/handlers/auth_test.go). Either update that test to call ParseJWT too, or narrow this comment to explicitly refer to production/authentication paths to avoid a misleading guarantee.
| // All JWT validation in the codebase should use this function (or the JWTAuth | |
| // Production authentication paths should use this function (or the JWTAuth |
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 1 code suggestion(s) and 0 general comment(s). @copilot Please apply all of the following code review suggestions:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Summary
Consolidates all JWT parsing into a shared
middleware.ParseJWT()function that usesjwt.WithValidMethods([]string{"HS256"})— the golang-jwt/v5 library's recommended approach for preventing algorithm confusion attacks.Before:
jwt.ParseWithClaims()called directly at 4 locations without algorithm restriction.After: Single
ParseJWT()with HS256-only enforcement + defense-in-depth HMAC method check in keyfunc.This is a prerequisite for the CNCF TAG-Security self-assessment (cncf/toc#2106) where we claim HS256-only enforcement.
Test plan
TestJWTAuth— valid, missing, invalid sig, expired, query param)