Skip to content

Commit

Permalink
Add alg check
Browse files Browse the repository at this point in the history
  • Loading branch information
Micah Parks committed Oct 31, 2022
1 parent a939f14 commit a55209a
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 15 deletions.
44 changes: 30 additions & 14 deletions jwks.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@ import (
"net/http"
"sync"
"time"

"github.com/golang-jwt/jwt/v4"
)

var (
// ErrJWKAlgMismatch indicates that the given JWK was found, but its "alg" parameter's value did not match that of
// the JWT.
ErrJWKAlgMismatch = errors.New(`the given JWK was found, but its "alg" parameter's value did not match the expected algorithm`)

// ErrJWKUseWhitelist indicates that the given JWK was found, but its "use" parameter's value was not whitelisted.
ErrJWKUseWhitelist = errors.New(`the given JWK was found, but its "use" parameter's value was not whitelisted`)

Expand Down Expand Up @@ -39,21 +45,23 @@ type JWKUse string

// jsonWebKey represents a JSON Web Key inside a JWKS.
type jsonWebKey struct {
Curve string `json:"crv"`
Exponent string `json:"e"`
K string `json:"k"`
ID string `json:"kid"`
Modulus string `json:"n"`
Type string `json:"kty"`
Use string `json:"use"`
X string `json:"x"`
Y string `json:"y"`
Algorithm string `json:"alg"`
Curve string `json:"crv"`
Exponent string `json:"e"`
K string `json:"k"`
ID string `json:"kid"`
Modulus string `json:"n"`
Type string `json:"kty"`
Use string `json:"use"`
X string `json:"x"`
Y string `json:"y"`
}

// parsedJWK represents a JSON Web Key parsed with fields as the correct Go types.
type parsedJWK struct {
use JWKUse
public interface{}
algorithm string
public interface{}
use JWKUse
}

// JWKS represents a JSON Web Key Set (JWK Set).
Expand Down Expand Up @@ -124,8 +132,9 @@ func NewJSON(jwksBytes json.RawMessage) (jwks *JWKS, err error) {
}

jwks.keys[key.ID] = parsedJWK{
use: JWKUse(key.Use),
public: keyInter,
algorithm: key.Algorithm,
use: JWKUse(key.Use),
public: keyInter,
}
}

Expand Down Expand Up @@ -181,7 +190,7 @@ func (j *JWKS) ReadOnlyKeys() map[string]interface{} {
}

// getKey gets the jsonWebKey from the given KID from the JWKS. It may refresh the JWKS if configured to.
func (j *JWKS) getKey(kid string) (jsonKey interface{}, err error) {
func (j *JWKS) getKey(kid string, token *jwt.Token) (jsonKey interface{}, err error) {
j.mux.RLock()
pubKey, ok := j.keys[kid]
j.mux.RUnlock()
Expand Down Expand Up @@ -221,5 +230,12 @@ func (j *JWKS) getKey(kid string) (jsonKey interface{}, err error) {
}
}

tokenAlg, ok := token.Header["alg"].(string)

This comment has been minimized.

Copy link
@sermojohn

sermojohn Oct 31, 2022

The token header is controlled by the unauthenticated client, so this way the algorithm verification can be bypassed.

Enforcing algorithm verification can be controlled by the algorithm JWK attribute. When the JWK has an algorithm specified, the token should match always match the algorithm.

if ok {
if pubKey.algorithm != "" && pubKey.algorithm != tokenAlg {
return nil, fmt.Errorf(`%w: JWK "alg" parameter value %q does not match token "alg" parameter value %q`, ErrJWKAlgMismatch, pubKey.algorithm, tokenAlg)
}
}

return pubKey.public, nil
}
2 changes: 1 addition & 1 deletion keyfunc.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func (j *JWKS) Keyfunc(token *jwt.Token) (interface{}, error) {
return nil, fmt.Errorf("%w: could not convert kid in JWT header to string", ErrKID)
}

return j.getKey(kid)
return j.getKey(kid, token)

This comment has been minimized.

Copy link
@sermojohn

sermojohn Oct 31, 2022

Both kid and alg are extracted from the token, so I am thinking that both parameters should be explicitly given to getKey.

This comment has been minimized.

Copy link
@MicahParks

MicahParks Oct 31, 2022

Owner

Makes sense to me. I'll make that change.

}

// base64urlTrailingPadding removes trailing padding before decoding a string from base64url. Some non-RFC compliant
Expand Down

0 comments on commit a55209a

Please sign in to comment.