search

marcospereirampj / python-keycloak

MIT License
692 stars 296 forks source link

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

Closed cm253 closed 3 months ago

cm253 commented 5 months ago

Hi!

I'm referring to the pull request https://github.com/marcospereirampj/python-keycloak/pull/531, where node-jose has been replaced by jwcrypto.

See the following comment:

https://github.com/marcospereirampj/python-keycloak/blob/2125d1e788a5153eb8ec769cee691edf2324fe8d/src/keycloak/keycloak_openid.py#L543

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 commented 4 months ago

This needs urgent attention. @marcospereirampj

aboubacs commented 4 months ago

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 commented 4 months ago

Related conversation also at https://github.com/marcospereirampj/python-keycloak/issues/503#issuecomment-1970898659

waza-ari commented 4 months ago

Related conversation in a library relying on python-keycloak: https://github.com/waza-ari/fastapi-keycloak-middleware/issues/30

ryshoooo commented 4 months ago

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.