v2.1.0 Diversions from JOSE By validating audiences when none expected

[theory]

Checklist

• I have looked into the README and have not found a suitable solution or answer.
• I have looked into the documentation and have not found a suitable solution or answer.
• I have searched the issues and have not found a suitable solution or answer.
• I have upgraded to the latest version of this SDK and the issue still persists.
• I have searched the Auth0 Community forums and have not found a suitable solution or answer.
• I agree to the terms within the Auth0 Code of Conduct.

Description

Upgrading a service to v2.1.0 and now getting a token validation error we don't see with v2.0.1, apparently caused by something in #176. The error is:

jwt invalid: failed to deserialize token claims: could not get token claims: square/go-jose: unsupported key type/format

I have been comparing the changes to validator/validator.go in that commit, and it sure seems like the process is the same. In v2.0.1 it's

	key, err := v.keyFunc(ctx)
	if err != nil {
		return nil, fmt.Errorf("error getting the keys from the key func: %w", err)
	}

	claimDest := []interface{}{&jwt.Claims{}}
	if v.customClaims != nil {
		claimDest = append(claimDest, v.customClaims())
	}

	if err = token.Claims(key, claimDest...); err != nil {
		return nil, fmt.Errorf("could not get token claims: %w", err)
	}

And in v2.1.0 it's:

	key, err := v.keyFunc(ctx)
	if err != nil {
		return jwt.Claims{}, nil, fmt.Errorf("error getting the keys from the key func: %w", err)
	}

	claims := []interface{}{&jwt.Claims{}}
	if v.customClaimsExist() {
		claims = append(claims, v.customClaims())
	}

	if err = token.Claims(key, claims...); err != nil {
		return jwt.Claims{}, nil, fmt.Errorf("could not get token claims: %w", err)
	}

These seem fundamentally the same, yet if I replace validator/validator.go with a copy from v2.0.1 it works! I'm pretty mystified what could have changed, hoping you all have some idea.

Reproduction

Internal code from $work I can't share, but happy to try things or print debugging output to try to figure it out.

Go JWT Middleware version

v2.1.0

Go version

v1.21

[theory]

Maddeningly, the fix has nothing to do with the processing of token claims; I have no idea why that is the error I usually get, but sometimes the error is the more useful jwt.ErrInvalidAudience:

square/go-jose/jwt: validation failed, invalid audience claim (aud)

Looking at the the JOSE source, which was the behavior prior to #176, it skips audience validation if there is no list of expected audiences. To match that behavior, the change would be:

--- a/validator/validator.go
+++ b/validator/validator.go
@@ -142,15 +142,17 @@
 		return jwt.ErrInvalidIssuer
 	}
 
-	foundAudience := false
-	for _, value := range expectedClaims.Audience {
-		if actualClaims.Audience.Contains(value) {
-			foundAudience = true
-			break
+	if len(expectedClaims.Audience) != 0 {
+		foundAudience := false
+		for _, value := range expectedClaims.Audience {
+			if actualClaims.Audience.Contains(value) {
+				foundAudience = true
+				break
+			}
+		}
+		if !foundAudience {
+			return jwt.ErrInvalidAudience
 		}
-	}
-	if !foundAudience {
-		return jwt.ErrInvalidAudience
 	}
 
 	if actualClaims.NotBefore != nil && expectedClaims.Time.Add(leeway).Before(actualClaims.NotBefore.Time()) {

But looking at #211, it seems as though perhaps we need to add audiences to our validation. We, it turns out, had custom validation configured like so:

		return validator.New(
			provider.KeyFunc,
			validator.RS256,
			cfg.Issuer,
			// The included audience validator is validating that every audience provided is present on claims, but we need to allow
			// any of the provided audiences so do that with a custom validator
			[]string{},
			validator.WithCustomClaims(func() validator.CustomClaims {
				return NewOneOfAudienceClaimValidator(allowedAudiences)
			}),
		)

But perhaps we were misreading the source previously, cause looking at it now it looks like it is indeed validating any audience. Is that right?

[ewanharris]

Hey @theory 👋🏻 , apologies for the delay getting back here.

You're right in the belief that this is down to #176, that change made the middleware always attempt to validate the aud claim in the JWT against the list of provided audiences even if none are provided. That change validates that one of the provided audiences matches so I think you should be good to replace your custom claims handler with just providing the audiences directly.

This change was then improved upon later in #183 to return an error during if the provided audience array is empty (rather than nil) but unfortunately hasn't been released so I'll look to get that released in the near future to hopefully make this error more intuitive.

[theory]

Thanks! I've updated our code to remove our custom audience validation and all seems well now. Will be handy to have the improved error output. Not sure there's anything to be done if someone wants to replace the audience validation, though. Not that it's a good idea, mind.

[ewanharris]

Great to hear, and yeah as mentioned in #211 the audience validation is a requirement this library has based on its purpose so the previous behaviour was classified as a bug.

I'll close this issue out, but thanks again for filing!