fix(auth): delegate JWT parsing to github.com/go-jose/go-jose by manusa · Pull Request #189 · containers/kubernetes-mcp-server

manusa · 2025-07-17T10:59:51Z

I tried to use the jose library (required by oidc) but it's not as lenient as golang-jwt.

Encapsulated JWT validation logic (client-side) in (c *JWTClaims) Validate(audience string). Initial step to #179 / #176 (comment)

Tests verify all local JWT parsing and validations. More tests coming in follow-up PR.

/cc @ardaguclu

Signed-off-by: Marc Nuri <marc@marcnuri.com>

ardaguclu · 2025-07-17T11:32:08Z

go.mod

 	github.com/BurntSushi/toml v1.5.0
 	github.com/coreos/go-oidc/v3 v3.14.1
 	github.com/fsnotify/fsnotify v1.9.0
+	github.com/golang-jwt/jwt/v4 v4.5.2


I agree that this simplifies the code. On the other hand, we simply require some field based validations after extracting the JWT token. We don't need sophisticated security based processes which is supposed to be handled by OIDC or API Server.

Adding a new dependency just for this simple sanity check brings about some downsides. For example, https://github.com/golang-jwt/jwt/security mentions about the security posture. GHSA-mh63-6h87-95cp looks like an high severity. This was fixed. But who knows what else.

In my opinion, we don't need to rely on external dependency for something that we can handle. Maybe we can move JWT based functions into its dedicated directory. WDYT?

Since we have already an indirect dependency to go-jose due to https://github.com/coreos/go-oidc/blob/a7c457eacb849c163a496b29274242474a8f44ab/go.mod#L8C2-L8C31, maybe it makes sense to use it. But I see that you have tried this as first choice.

Adding a new dependency just for this simple sanity check brings about some downsides. For example, https://github.com/golang-jwt/jwt/security mentions about the security posture. GHSA-mh63-6h87-95cp looks like an high severity. This was fixed. But who knows what else.

Oh crap, I added the wrong version 😓

In my opinion, we don't need to rely on external dependency for something that we can handle.

My original plan was to reuse the jose library (brought in by oidc). I'm not happy bringing in dependencies either.

So the additional benefit is that signatures are checked plus we don't need to rely on our own parsing which might be missing something else (e.g. you added padding for base64, but it's also difficult to track what else might be missing).

Let me give it another spin

Parsing and validating is now delegated to go-jose.
This should cover both concerns:

The dependency is reused from go-oidc, so we're not bringing in a new one

Whatever downsides we might face would already be brought in by relying on go-oidc and its side-effects

This sounds reasonable. I'll review in depth tomorrow.

Signed-off-by: Marc Nuri <marc@marcnuri.com>

ardaguclu

Overall looks good to me. Dropped a few comments.

This is totally your preference but maybe #179 PR can be added in this PR as well?.

ardaguclu · 2025-07-18T08:24:51Z

pkg/http/authorization.go

 			// Because missing expected audience and expired tokens must be
 			// rejected already.
-			claims, err := validateJWTToken(token, audience)
+			claims, err := ParseJWTClaims(token)


What about we only have one function that manages parsing, validating, extracting. This function returns scopes and error. Like this;

scopes, err := validateJWTAndGetScopes(token, audience, "") // 3rd parameter is authorization url to check the issuer, if it is set

So that we don't need this;

if err == nil && claims != nil { err = claims.Validate(audience) }

I think that would be the next step.
We need a function or an entity that orchestrates all of the validation

ardaguclu · 2025-07-18T08:26:07Z

pkg/http/authorization.go

-
-	return claims, nil
+// Validate Checks if the JWT claims are valid and if the audience matches the expected one.
+func (c *JWTClaims) Validate(audience string) error {


Suggested change

func (c *JWTClaims) Validate(audience string) error {

func validateJWTAndGetScopes(token string, audience string, issuerURL string) ([]string, error) {

tkn, err := jwt.ParseSigned(token, allSignatureAlgorithms)

if err != nil {

return nil, fmt.Errorf("failed to parse JWT token: %w", err)

}

claims := &JWTClaims{}

err = tkn.UnsafeClaimsWithoutVerification(claims)

if err != nil {

return nil, fmt.Errorf("failed to extract claims: %w", err)

}

expected := jwt.Expected{

AnyAudience: jwt.Audience{audience},

Time: time.Now(), // internally it is set to time.Now(), maybe explicitly setting is better

}

if issuerURL != "" { // maybe not the scope of this PR, you can add it or skip it

expected.Issuer = issuerURL

}

err = claims.Claims.Validate(expected)

if err != nil {

return nil, fmt.Errorf("JWT validation failed: %w", err)

}

var scopes []string

if claims.Scope != "" {

scopes = strings.Fields(claims.Scope)

}

return scopes, nil

}

I'd prefer to keep JWTToken as a single entity for further refactoring

ardaguclu · 2025-07-18T08:27:16Z

pkg/http/authorization.go

-	}
-
-	decoded, err := base64.URLEncoding.DecodeString(payload)
+func ParseJWTClaims(token string) (*JWTClaims, error) {


We won't need this, as validateJWTAndGetScopes handles it.

I'd prefer to keep it separate for now

ardaguclu · 2025-07-18T08:28:26Z

pkg/http/authorization_test.go

@@ -1,187 +1,222 @@
 package http


Unit tests will also be simplified, because they'll have to exercise only the validateJWTAndGetScopes function.

ardaguclu · 2025-07-18T08:29:49Z

pkg/http/authorization.go


+var allSignatureAlgorithms = []jose.SignatureAlgorithm{
+	jose.EdDSA,
+	jose.HS256,


It looks like go-oidc does not easily support verifying symmetric tokens (the ones contain HS prefixes). But it is better to add this in here, in case in the future we want to support this.

~~OK, I copied the list from them (IIRC)~~

I copied the list from https://github.com/go-jose/go-jose/blob/3a80e136a96e747bf44049414eadc02828df4d33/jose-util/crypto.go#L48-L62

Since the original implementation was completely lenient on the signature I assumed we wanted to allow anything, hence reusing a list.

I should have been clearer. I think, this list is correct and nice. The problem is in oidcProvider side which does not support symmetric algorithms such as HS256. So I think, I need to change oidcProvider side and we should keep this list as is.

manusa · 2025-07-18T09:24:32Z

Overall looks good to me. Dropped a few comments.

This is totally your preference but maybe #179 PR can be added in this PR as well?.

I'd prefer to do the refactor in very small steps.
There are also areas of the auth "module" that aren't tested that I'd like to test before changing more things.

This is should be considered as an initial step to simplify the JWTClaims struct.
This entity can then evolve to orchestrate all validation or be merged into another one.

e.g. it could be renamed to something else and then be the OIDC provider holder or even the kubernetes holder to perform the verifyToken operation (now in kubernetes)

Signed-off-by: Marc Nuri <marc@marcnuri.com>

fix(auth): delegate JWT parsing to github.com/golang-jwt/jwt

24e8ed3

Signed-off-by: Marc Nuri <marc@marcnuri.com>

manusa force-pushed the refactor/auth branch from 500a104 to 24e8ed3 Compare July 17, 2025 11:06

ardaguclu reviewed Jul 17, 2025

View reviewed changes

fix(auth): delegate JWT parsing to go-jose

f281313

Signed-off-by: Marc Nuri <marc@marcnuri.com>

manusa requested a review from ardaguclu July 17, 2025 13:17

manusa changed the title ~~fix(auth): delegate JWT parsing to github.com/golang-jwt/jwt~~ fix(auth): delegate JWT parsing to github.com/go-jose/go-jose Jul 17, 2025

ardaguclu reviewed Jul 18, 2025

View reviewed changes

fix(auth): delegate JWT parsing to go-jose - review comment

5cc6098

Signed-off-by: Marc Nuri <marc@marcnuri.com>

manusa added this to the 0.1.0 milestone Jul 18, 2025 — with automated-tasks

manusa merged commit 775fa21 into main Jul 18, 2025
9 of 10 checks passed

manusa deleted the refactor/auth branch July 18, 2025 11:01

manusa mentioned this pull request Jul 29, 2025

test(auth): complete test suite for unauthorized scenarios #220

Merged

-func (c *JWTClaims) Validate(audience string) error {
+func validateJWTAndGetScopes(token string, audience string, issuerURL string) ([]string, error) {
+	tkn, err := jwt.ParseSigned(token, allSignatureAlgorithms)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse JWT token: %w", err)
+	}
+	claims := &JWTClaims{}
+	err = tkn.UnsafeClaimsWithoutVerification(claims)
+	if err != nil {
+		return nil, fmt.Errorf("failed to extract claims: %w", err)
+	}
+	expected := jwt.Expected{
+		AnyAudience: jwt.Audience{audience},
+		Time:        time.Now(), // internally it is set to time.Now(), maybe explicitly setting is better
+	}
+	if issuerURL != "" { // maybe not the scope of this PR, you can add it or skip it
+		expected.Issuer = issuerURL
+	}
+	err = claims.Claims.Validate(expected)
+	if err != nil {
+		return nil, fmt.Errorf("JWT validation failed: %w", err)
+	}
+	var scopes []string
+	if claims.Scope != "" {
+		scopes = strings.Fields(claims.Scope)
+	}
+	return scopes, nil
+}

Comments

Conversation

manusa commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manusa Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manusa commented Jul 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

manusa Jul 18, 2025 •

edited

Loading

ardaguclu Jul 18, 2025 •

edited

Loading