Middleware never validates claims despite claiming to

[veqryn]

The middleare specifically states: We explicitly toggle SkipClaimsValidation in thejwt-goparser so that we can control when the claims are validated - in our case, by the Verifier http middleware handler. https://github.com/go-chi/jwtauth/blob/master/jwtauth.go#L42-L46

Then, in the Verifier middleware, the code doesn't ever actually validate the claims at all: https://github.com/go-chi/jwtauth/blob/master/jwtauth.go#L146-L150 https://github.com/go-chi/jwtauth/blob/master/jwtauth.go#L109-L130

The best it does it check the exp field manually, using code that doesn't need to exist since jwt-go already has this functionality built in.

My suggestion is to stop skipping claims validation, and stop checking the exp claim manually, and instead let jwt-go verify the exp claim along with the nbf and iat claims.

If you agree, I can make a pull request.

[VojtechVitek]

Sounds good.

We use our own Verifier middleware, so that's where the confusion came from. Good catch!

[ernsheong]

Any update? 😁 Seems like a semi-serious security flaw.

[pkieltyka]

@ernsheong which default validations are missing besides checking validity of the signature, the algo matches and the token isn't expired? as in https://github.com/go-chi/jwtauth/blob/master/jwtauth.go#L109-L130

[ernsheong]

Something I can think of is the "aud" claim. Tricky but ideal is how can we let the caller handle this on their own without letting this library do all the heavy lifting.

[pkieltyka]

@ernsheong in this case, as the docs suggest, you should write your own Verifier where the defaults aren't sufficient. But I don't see any security issues with the default, but as security issues are serious, it's nice to have a second set of eyes

[ernsheong]

Apologies, I might have overstated. Seems more acceptable now that I've studied it further.

Another separate concern is I'm not sure where this is coming from. Couldn't trace it from jwt-go: https://github.com/go-chi/jwtauth/blob/master/jwtauth.go#L112

Also, the docs said to "write your own" Authenticator, so I assumed Verifier was supposed to not be customizable.

[pkieltyka]

@ernsheong yea, good catch on that line. It's likely to do nothing at all, and was made to offer a more explicit error, but wouldn't be a security flaw. We should improve that for sure

[ernsheong]

@pkieltyka Thanks for the quick replies, created https://github.com/go-chi/jwtauth/issues/29 to track that. Loving chi! Sorry for spamming this thread 😅

[pkieltyka]

@ernsheong naw dude, thank you for giving it a look and submitting the concern, we can definitely improve it given the noop code :)

[pkieltyka]

I'll close this so we can continue in #29