Migrate from python-jose to joserfc? about python-keycloak HOT 18 CLOSED

This might have become a security issue: We got a warning for a CVE: https://nvd.nist.gov/vuln/detail/CVE-2024-23342 for ecdsa which is a direct dependency of python-jose..

Also the ecdsa package itself mentions that it is explicitly not production ready because of side channel attacks.

from python-keycloak.

+1 on this issue

from python-keycloak.

Hi, maintainer of a FastAPI keycloak middleware here, leveraging your library. We've also been stumbling across another inconsistency.

With python-jose the default behaviour for certain tokens (aud for example) was to verify the token if it exists, this doesn't exist anymore now though. With verify_aud set to True, a token with a missing aud claim did get parsed correctly in the past, now it would raise an Exception. Not sure what can be done about this, as this behaviour is not achievable with the settings exposed by jwcrypto

Regarding the other claims I'd vote to expose more control to the user of the library. Currently it is like this:

        options = kwargs.get("options", {})
        check_claims = {}
        if options.get("verify_aud") is True:
            check_claims["aud"] = self.client_id

        k = jwk.JWK.from_pem(key.encode("utf-8"))
        full_jwt = jwt.JWT(jwt=token, key=k, algs=algorithms, check_claims=check_claims)

I completely understand the idea to keep the interface backward compatible, but this approach has various issues. In the previous versions it was completely possible to set whatever options you liked. Right now it is impossible to set check_claims or any other setting.

My proposal would be along these lines:

• Pass user provided values for check_claims and default_claims from kwargs to the JWT constructor if provided
• If these values are not provided but options is provided, use the jose default settings for check_claims (or maybe more sensible ones) and use verify_<claim> entries in options to override the default.
• If nothing is provided, rely on jwcrypto default settings

from python-keycloak.

Thanks a lot for the suggestions and discussion everybody :)

I'm currently gravitating towards making a full breaking change for the decode_token functionality, essentially making the old jose options deprecated/removed and following the jwcrypto default options to be the standard. I completely agree with you @waza-ari, that the intention of the implementation is good, but it then imposes a sneaky requirement on us to maintain a translation and compatibility layer between jose and jwcrypto, which I'm not a big fan of.

So my intention for the solution is to give the user full ability to configure the jwcrypto functions directly, i.e. like

def decode_token(self, token, key, algorithms=["RS256"], **kwargs):
    full_jwt = jwt.JWT(jwt=token, key=key, algs=algorithms, **kwargs)
    return jwt.json_decode(full_jwt.claims)

Please let me know if you see any issues with this approach.

EDIT: I might keep the jwk.JWK.from_pem(key.encode("utf-8")) as a convenience, I'm just not sure right now

from python-keycloak.

Just an FYI: while this is only a minor version update and efforts have been made to keep the API the same, there is still a different "API" as far as exception handling goes.

from jose.exceptions import xyz becomes from jwcrypto.jwt import uvw, or ValueError etc to handle expired tokens and other use cases.

from python-keycloak.

@Nathan-Furnal The defaults are not standard I would say no. But the main problem here is that I can't pass options={"verify_aud": True, "verify_exp": True, "verify_nbf": True} - it is impossible to verify aud without destroying the exp and nbf verification.

from python-keycloak.

+1 on this ^ Came to raise this issue. Our scan showed a high vulnerability because of ecdsa this morning.

from python-keycloak.

+1 Same for us. Its impacting production deployment. Any plans to resolve this issue @marcospereirampj

from python-keycloak.

Suggestion for alternative package: jwcrypto. It's backed by cryptography and has decently recent commits. From personal experience, we've successfully used it in a few projects. The docs are its main weakness since they only show working examples.

The issue at hand doesn't affect us since we don't do ECC in our projects. However I would still like to see it fixed. There should be enough suitable JOSE implementations in Python.

from python-keycloak.

Also affecting one of my projects here. Curious to see any resolutions / workarounds people have done?

from python-keycloak.

Are you all still affected by this issue?

from python-keycloak.

More issues or differences:

exp and nbf not always checked

Per the jwcrypto dev, exp and nbf are always checked

This statement isn't entirely correct. exp and nbf are implicitly checked only when check_claims is not provided.

https://jwcrypto.readthedocs.io/en/latest/jwt.html#classes

Note: if check_claims is not provided the ‘exp’ and ‘nbf’ claims are checked

So there is definitely still some backwards incompatibility, the above code will not validate exp and nbf when we pass only options={"verify_aud": True} and this previously was the case.
This is definitely a bug.

python-jose had these defaults always applied.

{
  'verify_signature': True,
  'verify_aud': True,
  'verify_iat': True,
  'verify_exp': True,
  'verify_nbf': True,
  'verify_iss': True,
  'verify_sub': True,
  'verify_jti': True,
  'verify_at_hash': True,
  'require_aud': False,
  'require_iat': False,
  'require_exp': False,
  'require_nbf': False,
  'require_iss': False,
  'require_sub': False,
  'require_jti': False,
  'require_at_hash': False,
  'leeway': 0,
}

No way to check the at_hash

We used to do:

options = {"verify_at_hash": True}  # among others
self.keycloak_openid.decode_token(
    token,
    key=key,
    options=options,
    access_token=access_token,
)

And now the access token no longer gets validated.

I also don't really see any supported way within jwcrypto at first sight. The claim is not implemented.

from python-keycloak.

@Wim-De-Clercq from a discussion with the jwcrypto maintainer, it seems that the jose defaults are not standard? This is not helpful ofc but this is probably something that deserves to be in a test case in this project. @marcospereirampj Since this is stemming from my MR I'm willing to fix it, either rolling it back or use another lib. Or ask jwcrypto's @simo5 what his opinion is on the matter.

from python-keycloak.

I'm not sure how to fix it yet but I'll investigate

from python-keycloak.

you can unconditionally add exp and nbf verification unless they are explicitly disabled in your api by passing verify_exp/nbf: False
It generally never makes sense to skip that verification anyway so it should be safe to always have them set in the check_claims dictionary by default.

from python-keycloak.

If you have a specific need not covered by jwcrypto, feel free to open a feature request.
I will have to think if letting aud to be optional is a good idea, the idea of requiring aud is that you want to allow only specific clients to access a service, making it optional though, allows any client in as long as they know to omit the aud claim ... perhaps you should not enforce the aud via jwcrypto in the optional case, but simply pull token.claims out of it after validation to match any optional 'aud's ?

from python-keycloak.

what about license of jwcrypto which is LGPLv3+?

from python-keycloak.

Is it going to cause you any issue?

from python-keycloak.