Skip to content
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

Should jwt.decode accept PyJWK keys? #864

Closed
woodruffw opened this issue Mar 7, 2023 · 4 comments · Fixed by #886
Closed

Should jwt.decode accept PyJWK keys? #864

woodruffw opened this issue Mar 7, 2023 · 4 comments · Fixed by #886
Labels
stale Issues without activity for more than 60 days

Comments

@woodruffw
Copy link
Contributor

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 😅

pyjwt/jwt/api_jwt.py

Lines 182 to 198 in 777efa2

def decode(
self,
jwt: str | bytes,
key: str | bytes = "",
algorithms: Optional[List[str]] = None,
options: Optional[Dict[str, Any]] = None,
# deprecated arg, remove in pyjwt3
verify: Optional[bool] = None,
# could be used as passthrough to api_jws, consider removal in pyjwt3
detached_payload: Optional[bytes] = None,
# passthrough arguments to _validate_claims
# consider putting in options
audience: Optional[Union[str, Iterable[str]]] = None,
issuer: Optional[str] = None,
leeway: Union[int, float, timedelta] = 0,
# kwargs
**kwargs,

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
Copy link
Contributor Author

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

self.key = self.Algorithm.from_jwk(self._jwk_data)

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
Copy link
Contributor Author

woodruffw commented Mar 7, 2023

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
Copy link
Collaborator

auvipy commented Apr 16, 2023

I guess we should close this now?

@github-actions
Copy link

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

@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Jun 16, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues without activity for more than 60 days
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants