jpadilla / pyjwt

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

Should `jwt.decode` accept `PyJWK` keys? #864

Closed woodruffw closed 1 year ago

woodruffw commented 1 year ago

First of all, thanks for pyjwt!

I'm implementing the consumer side of an OIDC flow, with the verification keys being retrieved periodically from the OIDC provider's JSON Web Key (JWK) set. I then turn each JWK's contents into this library's PyJWK type.

Expected Result

For verification, I'm doing something like this:

signed_payload = jwt.decode(
    unverified_token,
    key=key,
    algorithms=["RS256"],
    options=dict(
        verify_signature=True,
        # "require" only checks for the presence of these claims, not
        # their validity. Each has a corresponding "verify_" kwarg
        # that enforces their actual validity.
        require=["iss", "iat", "nbf", "exp", "aud"],
        verify_iss=True,
        verify_iat=True,
        verify_nbf=True,
        verify_exp=True,
        verify_aud=True,
    ),
    issuer=self.issuer_url,
    audience=self.audience,
    leeway=30,
)

...where key is a PyJWK.

I expect this to succeed and return the signed claim dictionary (assuming key is valid).

Actual Result

Instead, I get a TypeError with the following message: Expecting a PEM-formatted key.

I traced this down to algorithms.py, where the key is expected to be in PEM format for RSA keys:

https://github.com/jpadilla/pyjwt/blob/777efa2f51249f63b0f95804230117723eca5d09/jwt/algorithms.py#L294C15-L295

...which made me check the types for the jwt.decode API, where I realized that this wasn't supported to begin with 😅

https://github.com/jpadilla/pyjwt/blob/777efa2f51249f63b0f95804230117723eca5d09/jwt/api_jwt.py#L182-L198

Request

So, my feature request: could jwt.decode accept PyJWK instances for the key= parameter?

I think most of the scaffolding for this is already in place: the Algorithm ABC already has a from_jwk classmethod, so each concrete implementation could use it in its concrete prepare_key implementation. From there, the top-level signature(s) could be broadened from str | bytes to str | bytes | PyJWK.

Alternatives considered

Given that the top level decode API only accepts PEM-encoded str or bytes objects, I'm not sure if there's a clean alternative to this.

I could do something hacky and manually re-encoded the PyJWK as a PEM-encoded key, potentially with the help of the cryptography APIs. But that seems error prone 🙂

woodruffw commented 1 year ago

Oh, I see now that each PyJWK also has a key attribute, which provides a concrete Algorithm instance:

https://github.com/jpadilla/pyjwt/blob/777efa2f51249f63b0f95804230117723eca5d09/jwt/api_jwk.py#L60

The documentation implies that this should work:

https://pyjwt.readthedocs.io/en/stable/usage.html#retrieve-rsa-signing-keys-from-a-jwks-endpoint

...but I'm not sure how that makes it through prepare_key, since it's called unconditionally and doesn't include an instance check for its own type. I'll experiment and find out.

woodruffw commented 1 year ago

Ah, from_jwk doesn't return an Algorithm subclass, but an instance of the actual underlying cryptography key type (e.g. _RSAPublicKey).

So, PyJWK.key should always work. Sorry for the stream of consciousness here!

IMO it would still be a nice improvement to the public APIs to allow PyJWK as a top-level type, or at least to adjust the hints to emphasize that cryptography's key types are supported.

auvipy commented 1 year ago

I guess we should close this now?

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