Skip to content

fix(auth): delegate JWT parsing to github.com/go-jose/go-jose #189

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.

Sign up for GitHub

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

Merged
manusa merged 3 commits into from
Jul 18, 2025
Merged

fix(auth): delegate JWT parsing to github.com/go-jose/go-jose #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24e8ed3
fix(auth): delegate JWT parsing to github.com/golang-jwt/jwt
manusa Jul 17, 2025
f281313
fix(auth): delegate JWT parsing to go-jose
manusa Jul 17, 2025
5cc6098
fix(auth): delegate JWT parsing to go-jose - review comment
manusa Jul 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ require (
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/go-jose/go-jose/v4 v4.0.5
github.com/mark3labs/mcp-go v0.34.0
github.com/pkg/errors v0.9.1
github.com/spf13/afero v1.14.0
Expand Down Expand Up @@ -52,7 +53,6 @@ require (
github.com/fxamacker/cbor/v2 v2.7.0 // indirect
github.com/go-errors/errors v1.4.2 // indirect
github.com/go-gorp/gorp/v3 v3.1.0 // indirect
github.com/go-jose/go-jose/v4 v4.0.5 // indirect
github.com/go-logr/logr v1.4.2 // indirect
github.com/go-openapi/jsonpointer v0.21.0 // indirect
github.com/go-openapi/jsonreference v0.20.2 // indirect
Expand Down
99 changes: 35 additions & 64 deletions pkg/http/authorization.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ package http

import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"net/http"
"strings"
"time"

"github.com/coreos/go-oidc/v3/oidc"
"github.com/go-jose/go-jose/v4"
"github.com/go-jose/go-jose/v4/jwt"
"k8s.io/klog/v2"

"github.com/manusa/kubernetes-mcp-server/pkg/mcp"
Expand Down Expand Up @@ -55,7 +54,10 @@ func AuthorizationMiddleware(requireOAuth bool, serverURL string, mcpServer *mcp
// Validate the token offline for simple sanity check
// Because missing expected audience and expired tokens must be
// rejected already.
claims, err := validateJWTToken(token, audience)
claims, err := ParseJWTClaims(token)
Copy link
Member

@ardaguclu ardaguclu Jul 18, 2025

Choose a reason for hiding this comment

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

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)
			}

Copy link
Member Author

@manusa manusa Jul 18, 2025

Choose a reason for hiding this comment

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

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

if err == nil && claims != nil {
err = claims.Validate(audience)
}
if err != nil {
klog.V(1).Infof("Authentication failed - JWT validation error: %s %s from %s, error: %v", r.Method, r.URL.Path, r.RemoteAddr, err)

Expand Down Expand Up @@ -118,11 +120,25 @@ func AuthorizationMiddleware(requireOAuth bool, serverURL string, mcpServer *mcp
}
}

var allSignatureAlgorithms = []jose.SignatureAlgorithm{
jose.EdDSA,
jose.HS256,
Copy link
Member

@ardaguclu ardaguclu Jul 18, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@manusa manusa Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I copied the list from them (IIRC)

Copy link
Member Author

@manusa manusa Jul 18, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ardaguclu ardaguclu Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

jose.HS384,
jose.HS512,
jose.RS256,
jose.RS384,
jose.RS512,
jose.ES256,
jose.ES384,
jose.ES512,
jose.PS256,
jose.PS384,
jose.PS512,
}

type JWTClaims struct {
Issuer string `json:"iss"`
Audience any `json:"aud"`
ExpiresAt int64 `json:"exp"`
Scope string `json:"scope,omitempty"`
jwt.Claims
Scope string `json:"scope,omitempty"`
}

func (c *JWTClaims) GetScopes() []string {
Expand All @@ -132,66 +148,21 @@ func (c *JWTClaims) GetScopes() []string {
return strings.Fields(c.Scope)
}

func (c *JWTClaims) ContainsAudience(audience string) bool {
switch aud := c.Audience.(type) {
case string:
return aud == audience
case []interface{}:
for _, a := range aud {
if str, ok := a.(string); ok && str == audience {
return true
}
}
case []string:
for _, a := range aud {
if a == audience {
return true
}
}
}
return false
}

// validateJWTToken validates basic JWT claims without signature verification and returns the claims
func validateJWTToken(token, audience string) (*JWTClaims, error) {
parts := strings.Split(token, ".")
if len(parts) != 3 {
return nil, fmt.Errorf("invalid JWT token format")
}

claims, err := parseJWTClaims(parts[1])
if err != nil {
return nil, fmt.Errorf("failed to parse JWT claims: %v", err)
}

if claims.ExpiresAt > 0 && time.Now().Unix() > claims.ExpiresAt {
return nil, fmt.Errorf("token expired")
}

if !claims.ContainsAudience(audience) {
return nil, fmt.Errorf("token audience mismatch: %v", claims.Audience)
}

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 {
Copy link
Member

@ardaguclu ardaguclu Jul 18, 2025

Choose a reason for hiding this comment

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

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
}

Copy link
Member Author

@manusa manusa Jul 18, 2025

Choose a reason for hiding this comment

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

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

return c.Claims.Validate(jwt.Expected{
AnyAudience: jwt.Audience{audience},
})
}

func parseJWTClaims(payload string) (*JWTClaims, error) {
// Add padding if needed
if len(payload)%4 != 0 {
payload += strings.Repeat("=", 4-len(payload)%4)
}

decoded, err := base64.URLEncoding.DecodeString(payload)
func ParseJWTClaims(token string) (*JWTClaims, error) {
Copy link
Member

@ardaguclu ardaguclu Jul 18, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@manusa manusa Jul 18, 2025

Choose a reason for hiding this comment

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

I'd prefer to keep it separate for now

tkn, err := jwt.ParseSigned(token, allSignatureAlgorithms)
if err != nil {
return nil, fmt.Errorf("failed to decode JWT payload: %v", err)
return nil, fmt.Errorf("failed to parse JWT token: %w", err)
}

var claims JWTClaims
if err := json.Unmarshal(decoded, &claims); err != nil {
return nil, fmt.Errorf("failed to unmarshal JWT claims: %v", err)
}

return &claims, nil
claims := &JWTClaims{}
err = tkn.UnsafeClaimsWithoutVerification(claims)
return claims, err
}

func validateTokenWithOIDC(ctx context.Context, provider *oidc.Provider, token, audience string) error {
Expand Down
Loading
Loading