marcospereirampj / python-keycloak

MIT License
692 stars 296 forks source link

changes in decode_token(...) #551

Closed muehlbacher closed 3 months ago

muehlbacher commented 3 months ago

In Version 3.9.0 it was possible to use a dict as key: e.g. key={"kid":"...","kty":"RSA","alg":"RS512","use":"sig","n":"...","e":"...","x5c":["..."],"x5t":"...","x5t#S256":"..."}

this was changed in the newer versions. Due to the line key.encode('utf-8') an error is thrown when using dicts.

Why was this changed?

ryshoooo commented 3 months ago

Hi @muehlbacher

The change was introduced to remove dependency on python-jose and replace it with jwcrypto (see https://github.com/marcospereirampj/python-keycloak/pull/531). In the current version, we sort of have a mix of python-jose signatures that get translated into jwcrypto options, but this also has issues as well (see f.e. https://github.com/marcospereirampj/python-keycloak/issues/532, https://github.com/marcospereirampj/python-keycloak/issues/503).

Moving forward from python-keycloak >=4, we will only support jwcrypto options and signatures specifically.

JulienBrodin commented 3 months ago

Hello @ryshoooo , I use decode_token with the "verify_signature" option to "False".So I set the "key" attribute to empty string. It used to work with Jose, however, with jwcrypto. It raises a value error exception. Is it possible to allow empty value for the key ? On the other hand a list of keys would useful for those who implement the key rotation in Keycloak. For example : I load the keys when I start my app instead of calling Keycloak every time I need to decide the token

ryshoooo commented 3 months ago

Hi @JulienBrodin

That's a fair point, I tend to do that as well (even though it is definitely not the recommended practice). I'm not sure right now whether verify_signature equivalent is supported by jwcrypto or not.

But honestly this is pushing it a bit to the edge. You don't need to use python-keycloak to decode a token from keycloak, you can use any library or custom-made code to decode the token outside of this. It probably comes down to the scope of what the decode_token should do.

My ideal scenario is to do something like client.decode_token(token) and I get decoded token out and that's it. How much customization should go into this method, I'm not sure, but by default I assume the method would fetch the public key and use it to decode and verify the token and return a validated decoded version of the token to me as the user, i.e. the function should follow the best standard practice. Overriding the key and decoding options should be optional.

If jwcrypto doesn't allow you to decode a token without verification, then perhaps this library should not support this behavior as it is just not a good thing to do in the first place. I get that there are use-cases where trust is established by default, but prefer to have the user go on its own and implement their own unsafe decoding logic rather than the library should support it directly.

JulienBrodin commented 3 months ago

Hello @ryshoooo Indeed you're right, I am going to implement the decode function as it is standard. Thank you for your answer