No way to distinguish between "invalid" and expired JWT token

[guillerodriguez]

When using the FirebaseJwt adapter, if an expired token is passed in the request, this is handled as an "invalid" token and there is no way to differentiate between this condition and a token with e.g. invalid signature.

This is inconvenient since it is normal for tokens to expire during regular operation, but if a token is invalid for other reasons this may indicate an issue somewhere or an attack. Thus it would be nice to be able to differentiate between these cases.

The call chain is the following:

• ResourceController::getAccessTokenData
• JwtAccessToken::getAccessToken
• FirebaseJwt::decode, this is first called with $allowedAlgorithms = null, which should "decode, but not verify", and then a second time with the (possibly client-specific) allowed algorithms.

The problem is that in FirebaseJwt, the decode method verifies the dates (this does not happen with the Jwt class).

There are two possible ways to fix this:

1. Patch firebase/php-jwt and provide a way to disable the checking of the expiration date. The caller would then be responsible for this check. In our case this is done already in ResourceController::getAccessTokenData.
2. Change the API of EncryptionUtil::decode, which currently returns either the JWT payload or false, so that it can return different values for "expired" and "any other decoding or validation problems".

A possible (non-optimal) workaround is to set JWT::leeway to a very large value, which effectively disables all date validations. Unfortunately this also disables the validation of the iat and nbf claims, which is undesirable.

[guillerodriguez]

Also see: firebase/php-jwt#531

[bshaffer]

We could follow the pattern set by the other controllers here, and pass the Request to the encryption util, and that could set the proper response code and error message. Would that be sufficient?

[guillerodriguez]

Yes, that would indeed be sufficient. FirebaseJwt can then handle ExpireException separately from other exceptions.

…

We could follow the pattern set by the other controllers here, and pass the Request to the encryption util, and that could set the proper response code and error message. Would that be sufficient? — Reply to this email directly, view it on GitHub <#1053 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS67KFSGQRNWLZDM4QLIHLXXYZWTANCNFSM6AAAAAA4C7KOMM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Guillermo Rodriguez Garcia ***@***.***

[bshaffer]

Would you be willing to submit a pull request for the suggested changes?

On Tue, Aug 29, 2023 at 2:46 PM Guillermo Rodríguez < ***@***.***> wrote: Yes, that would indeed be sufficient. FirebaseJwt can then handle ExpireException separately from other exceptions. > We could follow the pattern set by the other controllers here, and pass > the Request to the encryption util, and that could set the proper > response code and error message. Would that be sufficient? > > — > Reply to this email directly, view it on GitHub > < #1053 (comment)>, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAS67KFSGQRNWLZDM4QLIHLXXYZWTANCNFSM6AAAAAA4C7KOMM> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> > -- Guillermo Rodriguez Garcia ***@***.*** — Reply to this email directly, view it on GitHub <#1053 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZMBOCVHBX6R75TNE4P7LXXZPKNANCNFSM6AAAAAA4C7KOMM> . You are receiving this because you commented.Message ID: ***@***.***>

-- Brent Shaffer

[guillerodriguez]

@bshaffer I can try. This requires modifying the API of AccessTokenInterface and EncryprionUtil to take a Response (I assume you meant this rather than Request), which will break API compatibility. Is this ok ?

[guillerodriguez]

@bshaffer Specifically this would require the following API changes:

• Modify AccessTokenInterface::getAccessToken to take a ResponseInterface
• Modify EncryptionInterface::decode to take a ResponseInterface

Is this ok ?

[guillerodriguez]

@bshaffer Can you confirm the outlined approach is OK?

[guillerodriguez]

@bshaffer I am happy to prepare a PR with the changes that we have discussed but I would need some feedback on the suggested approach (see my earlier comment about API changes). Can you comment on this?