-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from python-jose to joserfc? #503
Comments
This might have become a security issue: We got a warning for a CVE: https://nvd.nist.gov/vuln/detail/CVE-2024-23342 for Also the ecdsa package itself mentions that it is explicitly not production ready because of side channel attacks. |
+1 on this ^ Came to raise this issue. Our scan showed a high vulnerability because of |
+1 Same for us. Its impacting production deployment. Any plans to resolve this issue @marcospereirampj |
Suggestion for alternative package: jwcrypto. It's backed by 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. |
+1 on this issue |
Also affecting one of my projects here. Curious to see any resolutions / workarounds people have done? |
Are you all still affected by this issue? |
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.
|
More issues or differences:
|
# Per the jwcrypto dev, `exp` and `nbf` are always checked | |
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) | |
return jwt.json_decode(full_jwt.claims) |
Per the jwcrypto dev,
exp
andnbf
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.
@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 |
@Nathan-Furnal The defaults are not standard I would say no. But the main problem here is that I can't pass |
I'm not sure how to fix it yet but I'll investigate |
you can unconditionally add exp and nbf verification unless they are explicitly disabled in your api by passing verify_exp/nbf: False |
Hi, maintainer of a FastAPI keycloak middleware here, leveraging your library. We've also been stumbling across another inconsistency. With 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 My proposal would be along these lines:
|
If you have a specific need not covered by jwcrypto, feel free to open a feature request. |
what about license of jwcrypto which is LGPLv3+? |
Is it going to cause you any issue? |
Thanks a lot for the suggestions and discussion everybody :) I'm currently gravitating towards making a full breaking change for the 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 |
Since python-jose is badly mainted, how about switching to joserfc from authlib (http://jose.authlib.org, https://github.com/authlib/joserfc/)?
It seems to be very standards orientated and has 100% code coverage.
Also it is available under BSD 3-Clause License and is backed by the company behind authlib, which also is available as Open Source.
The biggest downside is probably that is still a young project and APIs might change between minor versions (breaking changes are mentioned in release notes).
The text was updated successfully, but these errors were encountered: