Fix access token validation #183
Conversation
|
@jimmyjames - I have not forgotten about you! I'm working on a fix for the same problem in the SDK (while using a refresh token) and want to see if that change could address this as well. The work here is not lost either way, I think we should examine both approaches to make sure we're doing the right thing here. Edit: 👇 |
src/Auth0/Login/Auth0Service.php
Outdated
| } | ||
|
|
||
| // Use IdTokenVerifier since Auth0-issued JWTs contain the 'sub' claim, which is used by the Laravel user model | ||
| $token_verifier = new IdTokenVerifier( |
There was a problem hiding this comment.
When we were discussing whether to use TokenVerifier or IdTokenVerifier, it didn't occurred to me to look back through the reason why the former even exists in the first place (PHP SDK issue). The problem we'll see with using this is that the azp claim won't appear in the aud array. These are not OIDC JWTs so we should not validate them as such. We have a docs page on validating access tokens which mentions the standard claims (detailed here) and adds aud as well. That was the whole reason behind the existence of TokenVerifier so let's definitely use that here :)
There was a problem hiding this comment.
Thanks for the background @joshcanhelp, that makes sense.
I did pull down auth0/auth0-PHP#434, and while it will get past the missing nonce failure if we pass the options ["nonce" => false] when calling decodeIdToken, it will fail on the audience validation as decodeIdToken constructs the token verifier using the clientId instead of the audience.
But given that we should use TokenVerifier instead of IdTokenVerifier for the reasons you listed above, it seems we either still need to do the validation here in laravel-auth0 (but change to use TokenVerifier, or provide another API or configuration point to allow decoding/validating of Auth0-issued JWT access tokens. What do you think the best approach is?
There was a problem hiding this comment.
Updated this PR to use TokenVerifier instead of IdTokenVerifier per the discussion above and after discussing with @joshcanhelp
| $signature_verifier = new AsymmetricVerifier($jwks); | ||
| } else if ('HS256' === $idTokenAlg) { | ||
| $signature_verifier = new SymmetricVerifier($this->auth0Config['client_secret']); | ||
| } |
There was a problem hiding this comment.
else... alg not supported? if this is not possible at this point, please drop a comment there for the next maintainer / security reviewer
There was a problem hiding this comment.
This came up in the PHP SDK as well. It does look a bit like null passed to TokenVerifier could be treated as alg:none (which is not the case but still). Same in the PHP SDK:
https://github.com/auth0/auth0-PHP/blob/master/src/Auth0.php#L657
I would throw an InvalidTokenException here.
https://github.com/auth0/auth0-PHP/blob/master/src/Exception/InvalidTokenException.php
There was a problem hiding this comment.
👍Pushed another commit that throws an InvalidTokenException when using an unsupported signing algorithm.
|
|
3 participants
Changes
Fixes
decodeJWTto decode JWT access tokens:IdTokenVerifierfrom auth0-php, since Auth0-issued JWTs contain asubclaim and this is used by the Laravel user model.supported_algsconfig; defaults to RS256.Testing
Would like to add additional unit tests, either as a follow-up commit or separate PR. Would be good to use a mock and confirm that the
verifyAPI is called with the configured options as we'd expect, but theIdTokenVerifierclass is marked asfinal, so another approach will have to be taken.Checklist
[X] I have read the Auth0 general contribution guidelines
[X] I have read the Auth0 Code of Conduct