Closed raayu83 closed 4 months ago
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.
+1 on this ^ Came to raise this issue. Our scan showed a high vulnerability because of ecdsa
this morning.
+1 Same for us. Its impacting production deployment. Any plans to resolve this issue @marcospereirampj
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.
+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.
from jose.exceptions import xyz
becomes from jwcrypto.jwt import uvw
, or ValueError etc to handle expired tokens and other use cases.
More issues or differences:
exp
and nbf
not always checkedPer 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,
}
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 jwcrypto
's @simo5 what his opinion is on the matter.
@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.
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 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.
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:
check_claims
and default_claims
from kwargs
to the JWT constructor if providedoptions
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 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 ?
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 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
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).