"exp" claim not checked because of check_claims parameter (jwcrypto)

[cm253]

Hi!

I'm referring to the pull request #531, where node-jose has been replaced by jwcrypto.

See the following comment:

python-keycloak/src/keycloak/keycloak_openid.py

Line 543 in 2125d1e

# Per the jwcrypto dev, `exp` and `nbf` are always checked

You are passing the check_claims dict to the JWT constructor. In this case, the "exp" claim is not checked if it is valid. The reason is, jwcrypto doesn't check the expiration time if the check_claims parameter has been passed, see the following code:

https://github.com/latchset/jwcrypto/blob/5dc2ea2a87ea9fb3ed833f9f0f7864edc7b01e7b/jwcrypto/jwt.py#L472

An empty dict is not None, so currently we check only if the "exp" claim exists and is a valid integer. To test it you can add a breakpoint in the _check_exp function and see if this function is called:

https://github.com/latchset/jwcrypto/blob/5dc2ea2a87ea9fb3ed833f9f0f7864edc7b01e7b/jwcrypto/jwt.py#L452

In order to also check the validity of the token you have to set 'exp': None in the dict. I came across this when I implemented the token check by myself using the jwcrypto lib.

Regards,
Chris

[sauerburger]

This needs urgent attention. @marcospereirampj

[aboubacs]

The change was for sure made too fast, there is no way to control the options anymore:

https://github.com/marcospereirampj/python-keycloak/pull/531/files#diff-0bbc8509272e0bb1f2f6be00fcb4f0ae29ff7522c88dcc085a305a0224659aa6R542-R547

=> What about all the other options ? Like verify_signature, etc

[Wim-De-Clercq]

Related conversation also at #503 (comment)

[waza-ari]

Related conversation in a library relying on python-keycloak: waza-ari/fastapi-keycloak-middleware#30

[ryshoooo]

Thanks for bringing this up @cm253

I really feel reluctant to make a mapping between the old jose options to the jwcrypto options. Honestly, I prefer to make a breaking change here, change the signature completely, and just pass options for jwcrypto directly such that users can modify this to their own extent.

By merging #531 we have unfortunately made a breaking change already as the behavior is not the same anymore, so we might as well make it official and change the signature of the function.