Add jwks support #437

damelLP · 2021-07-29T01:50:21Z

In some cases for RSA the tokens are rotated and we need a way of resolving the tokens from a JWK_URL.

Since pyjwt ===2.0.0 they added a pyjwkclient that has inbuilt caching of the keys: https://github.com/jpadilla/pyjwt/blob/79c23d7d9d32364be8f94680d8eda7135c3a15d5/jwt/jwks_client.py#L11

I needed it for Auth0 to work.

similar to #200 and #250

Needed to generate a second priv/pub pair and didn't want to add significant extra scrolling.

Since pyjwt 2.0.0 they have added support for resolving JWT keys from a some hosted key registry. https://pyjwt.readthedocs.io/en/latest/usage.html#retrieve-rsa-signing-keys-from-a-jwks-endpoint This functionality is required to be able to use Auth0 with simplejwt

Andrew-Chen-Wang

thanks for the linting job 👍

rest_framework_simplejwt/backends.py

Andrew-Chen-Wang · 2021-07-29T02:45:46Z

Thanks for the PR!

I'm sure this serves a useful purpose for adding jwks support. What I'm not sure about is why you asked about:

unless there is an existing mechanism for handling the rotating keys

It's something I'd like to add, but I don't see that here. How does rotating key support come into play here? If the entire point of this PR is to support key rotation, I'd rather do it the slow method: a sequence of keys that are tested.

damelLP · 2021-07-29T03:04:47Z

Thanks for the PR!

I'm sure this serves a useful purpose for adding jwks support. What I'm not sure about is why you asked about:

unless there is an existing mechanism for handling the rotating keys

It's something I'd like to add, but I don't see that here. How does rotating key support come into play here? If the entire point of this PR is to support key rotation, I'd rather do it the slow method: a sequence of keys that are tested.

np at all! I've made the requested changes too!

For the rotating keys I mean at the point of decoding as some external services like auth0 do it already and expose the current valid public key via the kid in the headers and querying a key registry which I believe is the standard for how that should be done.

For encoding where this would be the issuer of the pairs and have to update the kid for some distant client, I imagine there is still a lot of work that needs to be done. Probably some mechanism register kid,pubkey pairs on some registry that a client will have access to.

Andrew-Chen-Wang · 2021-07-29T03:31:25Z

expose the current valid public key via the kid in the headers

I imagine there is still a lot of work that needs to be done. Probably some mechanism register kid

Does this PR about kid help to get started on any further support (though I think it does the opposite by generating kids?): #253 If so, a new PR to update it and responses to concerns in the comments would be appreciated :)

I believe this library was not originally designed for external services in mind.

querying a key registry which I believe is the standard for how that should be done.

This is definitely something we don't implement and I ask everyone to make a custom implementation instead since I can't maintain a service that I don't use. That can change if 1. there's a PR that makes an abstract mechanism based on the RFCs and 2. there are unit tests to the service itself.

damelLP · 2021-07-29T03:51:26Z

Does this PR about kid help to get started on any further support (though I think it does the opposite by generating kids?)

Yes this PR is the opposite of generating kids.

This is definitely something we don't implement and I ask everyone to make a custom implementation instead since I can't maintain a service that I don't use.

This isn't to add any extra services this is the established mechanism that's supported in PyJWT since version 2.0.0. I'm using the recommended PyJWKClient as per the docs which handles everything that conforms to this RFC: https://datatracker.ietf.org/doc/html/rfc7517.

damelLP · 2021-07-29T03:55:20Z

This isn't to add any extra services

The user uses whatever endpoint they have the JWK at by passing JWK_URL="some_endpoint".

The responsibility of that service working is still on the user

damelLP · 2021-07-29T13:14:48Z

Hey @Andrew-Chen-Wang,

Are we good to merge it now?

Or are there more changes you need me to do?

Andrew-Chen-Wang · 2021-07-29T16:06:44Z

Hi @damelLP apologies. I wanted to take one last look over this weekend before merging (it was past me bedtime yesternight :) so I didn't entirely trust my review).

Planning on a release this weekend as well.

Andrew-Chen-Wang

LGTM tysm @damelLP just need to lint through again (run make lint-roll or at least isort again) unfortunately, Jazzband policy is to get another reviewer to review everything, so it'll be delayed until I can convince the head of Jazzband to let me merge my own PRs (without this perm, I'll be delayed quite heavily in further releases). I'd give it ETA Tuesday / Wednesday. Sorry!

rest_framework_simplejwt/backends.py

damelLP · 2021-08-04T14:15:19Z

Hey @Andrew-Chen-Wang, how we doing?

Andrew-Chen-Wang · 2021-08-06T21:44:00Z

Thanks for the PR!

javialon26 · 2021-08-19T03:15:44Z

@Andrew-Chen-Wang do you have an estimated date for the release of this feature? thank you.

Andrew-Chen-Wang · 2021-08-19T03:23:35Z

@javialon26 I will be taking advantage of an approved PR to add changelog messages since no one else maintains this repository. Give me 1-2 hours and you'll see the new release. Apologies for the wait!

Kandeel4411 · 2021-08-23T15:31:52Z

rest_framework_simplejwt/backends.py

+            return self.signing_key
+
+        if self.jwks_client:
+            return self.jwks_client.get_signing_key_from_jwt(token).key


@Andrew-Chen-Wang Heya, I had a question about this part if you don't mind me asking but is there a reason we are getting the signing key from the jwt as the verifying key? Shouldn't it get the public key from the jwk-uri instead of the private/signing key? ( I had a jwk endpoint that I was using that only returns the public key to be used for verification but it doesn't work since this expects the signing key )

We're trying to decode the token; naming is a little weird ngl https://pyjwt.readthedocs.io/en/stable/usage.html#retrieve-rsa-signing-keys-from-a-jwks-endpoint

Ahh I see, thanks and sorry for the trouble!

np and no worries!

damelLP added 5 commits July 29, 2021 02:13

Pull the priv/pub keys into separate module

0f9e05a

Needed to generate a second priv/pub pair and didn't want to add significant extra scrolling.

Add test for resolving the public key from jwks

610e50b

Add JWK_URL setting to the state and settings.py

f9e6ad8

Update docs with JWK_URL option

bb78772

damelLP mentioned this pull request Jul 29, 2021

Verify token signature from public key in third party service #250

Open

Andrew-Chen-Wang requested changes Jul 29, 2021

View reviewed changes

rest_framework_simplejwt/backends.py Outdated Show resolved Hide resolved

rest_framework_simplejwt/backends.py Outdated Show resolved Hide resolved

rest_framework_simplejwt/backends.py Outdated Show resolved Hide resolved

Add requested changes

ee62618

Andrew-Chen-Wang requested changes Aug 1, 2021

View reviewed changes

rest_framework_simplejwt/backends.py Outdated Show resolved Hide resolved

damelLP added 2 commits August 1, 2021 20:29

Sort imports

44e6d68

Fix minor linting errors

88a3d41

Andrew-Chen-Wang approved these changes Aug 6, 2021

View reviewed changes

Andrew-Chen-Wang merged commit f97ef69 into jazzband:master Aug 6, 2021

Kandeel4411 reviewed Aug 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jwks support #437

Add jwks support #437

damelLP commented Jul 29, 2021

Andrew-Chen-Wang left a comment

Andrew-Chen-Wang commented Jul 29, 2021

damelLP commented Jul 29, 2021

Andrew-Chen-Wang commented Jul 29, 2021

damelLP commented Jul 29, 2021 •

edited

Loading

damelLP commented Jul 29, 2021

damelLP commented Jul 29, 2021 •

edited

Loading

Andrew-Chen-Wang commented Jul 29, 2021 •

edited

Loading

Andrew-Chen-Wang left a comment •

edited

Loading

damelLP commented Aug 4, 2021

Andrew-Chen-Wang commented Aug 6, 2021

javialon26 commented Aug 19, 2021

Andrew-Chen-Wang commented Aug 19, 2021

Kandeel4411 Aug 23, 2021

Andrew-Chen-Wang Aug 23, 2021

Kandeel4411 Aug 23, 2021

Andrew-Chen-Wang Aug 23, 2021

Add jwks support #437

Add jwks support #437

Conversation

damelLP commented Jul 29, 2021

Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

Andrew-Chen-Wang commented Jul 29, 2021

damelLP commented Jul 29, 2021

Andrew-Chen-Wang commented Jul 29, 2021

damelLP commented Jul 29, 2021 • edited Loading

damelLP commented Jul 29, 2021

damelLP commented Jul 29, 2021 • edited Loading

Andrew-Chen-Wang commented Jul 29, 2021 • edited Loading

Andrew-Chen-Wang left a comment • edited Loading

Choose a reason for hiding this comment

damelLP commented Aug 4, 2021

Andrew-Chen-Wang commented Aug 6, 2021

javialon26 commented Aug 19, 2021

Andrew-Chen-Wang commented Aug 19, 2021

Kandeel4411 Aug 23, 2021

Choose a reason for hiding this comment

Andrew-Chen-Wang Aug 23, 2021

Choose a reason for hiding this comment

Kandeel4411 Aug 23, 2021

Choose a reason for hiding this comment

Andrew-Chen-Wang Aug 23, 2021

Choose a reason for hiding this comment

damelLP commented Jul 29, 2021 •

edited

Loading

damelLP commented Jul 29, 2021 •

edited

Loading

Andrew-Chen-Wang commented Jul 29, 2021 •

edited

Loading

Andrew-Chen-Wang left a comment •

edited

Loading