jpadilla / pyjwt

JSON Web Token implementation in Python
https://pyjwt.readthedocs.io
MIT License
5.05k stars 676 forks source link

very unhelpfull / missleading error messages on get_signing_key_from_jwt #820

Closed julianhahn closed 1 year ago

julianhahn commented 1 year ago

Hello,

I spend a few hours today for a bug that shouldn't have been there. I set up my python application where I recieve a jwt token from oauth-proxy, validate, decode and echo the contents. Running locally, everything went perfect. Running in a container I got the message "The JWK Set did not contain any usable keys". I checked, oauth configs, credentials, my code, put a few hours into trying to debug the python code while running in podman.

Then after a few hours my colleague helped and ran the project on his machine. He got the same error. So what does his machine has not, that mine has? Dependencies.

After another hour of debuging, we found out, that my jwt token is encoded with RSA. For pyjwt to decode the key we need to use RSA. To use RSA the cryptography is nessecary. I got this installed in my global dependencies and forgot to use an env.

The thing is: Alls this could have been prevented, if the raised error message would be "found RSA - cannot decode because dependency cryptography is missing"

Instead the error handling is as follows:

api_jwk.py - PyJWKSet:

class PyJWKSet:
    def __init__(self, keys: list[dict]) -> None:
        self.keys = []

        if not keys:
            raise PyJWKSetError("The JWK Set did not contain any keys")

        if not isinstance(keys, list):
            raise PyJWKSetError("Invalid JWK Set value")

        for key in keys:
            try:
                *self.keys.append(PyJWK(key))*
            *except PyJWKError:*
                # skip unusable keys
                *continue*

        if len(self.keys) == 0:
            raise PyJWKSetError("The JWK Set did not contain any usable keys")

->

class PyJWK:
    def __init__(self, jwk_data, algorithm=None):
        self._algorithms = get_default_algorithms()

->

ef get_default_algorithms():
    """
    Returns the algorithms that are implemented by the library.
    """
    default_algorithms = {
        "none": NoneAlgorithm(),
        "HS256": HMACAlgorithm(HMACAlgorithm.SHA256),
        "HS384": HMACAlgorithm(HMACAlgorithm.SHA384),
        "HS512": HMACAlgorithm(HMACAlgorithm.SHA512),
    }

    if has_crypto:
        default_algorithms.update({
                "RS256": RSAAlgorithm(RSAAlgorithm.SHA256),
   ...

and has_crypto comes from algorithms.py

try:
    import cryptography.exceptions
....
except ModuleNotFoundError:
    has_crypto = False

As summary: You iterate through every key, looking if there is an algorithm, find that a dependecie is missing and skip this key, and buring the error message. Afterwards the length of the list of keys is 0 - you get the message of no usable keys and are none the wiser, why this is the case. please rework you try catch error flow, so that a developer knows when a dependency is missing for the current key!

TimoWilhelm commented 1 year ago

I just encountered the same issue today. Thankfully I found this issue so I didn't have to spend a few hours debugging this. I agree a better error message would be very helpful!

HanhThong commented 1 year ago

I faced with the same issue. This is not a bug, you must install cryptography to use another algorithms such as RS256.

masalim2 commented 1 year ago

It's not a bug, but it is easy to miss this requirement and spend some time chasing down the cause. For instance, this just happened to me where cryptography was installed in my dev environment but missing from a Docker image.

Instead of silently swallowing the PyJWKErrors, what if we determined that crypto was needed but missing and added a hint to the exception in api_jwk.py:

        if len(self.keys) == 0:
            if not has_crypto and any(key["alg"] in crypto_algs for key in keys):
                 hint = "You need to install `cryptography` or `pyjwt[crypto]` to decode tokens using RS256"
            else:
                 hint = ""
            raise PyJWKSetError(f"The JWK Set did not contain any usable keys. {hint}")
julianhahn commented 1 year ago

I didn't say it's a bug in the package, rather the error handling caused a bug in my project :) exactly @masalim2, a simple warning message would have reduced the time needed to solve the issue down to few minutes. The reason, why I dind't commit something like your suggestion, is that there are a lot of diffrent modules in the file with the big "try to import, else throw the error away" and I'm not sure the other modules got proper error handling, or we would simply fix the issue for cryptography but not the other modules. Thats why I wrote that "there is need to rework the error handling with modules"

kaisung commented 1 year ago

I ran into the same issue, thanks for Google search leading me to the solution here. Agree this isn't a bug but adding a hint in the error message as @masalim2 proposed seems like a good usability improvement.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

koiker commented 2 months ago

Probably the easiest way to solve this is to just print a warning message in the algorithms.py when the exception ModuleNotFoundError is triggered and show a warning that Cryptography lib is not present. I just spent some time today with the same error to find out that my PyCharm was executing the script with one virtual env while the project had another interpreter who has crypto installed.