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

[theory]

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

[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!