feat: replace pyjwkest with pyjwt #32270

mumarkhan999 · 2023-05-19T11:29:19Z

Things to be done in this PR

This is part of openedx/edx-drf-extensions#290

Upgraded JWT_SIGNING_KEYS using https://github.com/openedx/edx-platform/blob/master/scripts/jwk-precompute-params.py
Replaced Pyjwkest usage with Pyjwt in codebase
Docs updated
Tests updated
edx-drf-extesion is being installed from pypi
feat: make jwt decode fucntion generic edx-drf-extensions#354 is merged

timmc-edx · 2023-05-23T00:19:53Z

The source changes so far look good. There are still some references to pyjwkest in the repo, though: Tests, a management command, docs, example code in an ADR, and the requirements files. The PR title makes me think the intent is to remove it entirely. If not, what boundary are you hoping to draw for this PR?

mumarkhan999 · 2023-05-26T14:37:11Z

The source changes so far look good. There are still some references to pyjwkest in the repo, though: Tests, a management command, docs, example code in an ADR, and the requirements files. The PR title makes me think the intent is to remove it entirely. If not, what boundary are you hoping to draw for this PR?

Except for management command and requirements files, all the above-mentioned tasks will be done in this PR.

timmc-edx · 2023-06-03T00:16:07Z

openedx/core/djangoapps/oauth_dispatch/tests/mixins.py

@@ -33,25 +33,37 @@ def _decode_jwt(verify_expiration):
            Helper method to decode a JWT with the ability to
            verify the expiration of said token
            """
-            keys = KEYS()
+            key_set = []


It looks like this code largely duplicates what's in edx-drf-extensions. Can we call that code instead?

(Having the duplicated code increases maintenance burden but also risks us having a mismatch in behavior between the tests and how we actually verify and decode JWTs.)

I'm planning to move JWT-related similar code to this repo https://github.com/edx/token-utils. But that repo is under edx organization. What's your say about that?

It's possible that move makes sense, but it deserves as ADR, and will have some consequences:

Ownership of token-utils is currently owned by cosmonauts, and I imagine we would need a discussion about that. Would it move to arbi-bom or arch-bom? We'd need to discuss with both teams.

Yes, if this became a required dependency, it would need to move to the openedx repo.

Would these utilities be exposed via edx-drf-extensions as well, or just through the new repo? Would it depend on whether or not it was specifically JWT auth related?

The repo itself is not in a great state. It looks like https://github.com/edx/token-utils/blob/main/docs/decisions/0001-purpose-of-this-repo.rst was never updated, and I imagine other docs, so its purpose is not yet clearly documented.

Things to consider.

It's hard to say whether a move to token-utils is a good idea without knowing more about what that repo is supposed to be. (It's not clear from the README or other docs.) I am interested in knowing more about it, but it feels out of scope for the present work.

Here's what I was picturing for deduplication: #32466 -- this rewrites the mixin to use Django setting overrides so that it can just call our standard decoders.

It's hard to say whether a move to token-utils is a good idea without knowing more about what that repo is supposed to be. (It's not clear from the README or other docs.)

The repo token-utils was created for edx-exams, where they wanted to create custom signed JWTs (i.e. non-auth JWTs). I'll leave it to you all decide how to proceed, but it is important to note that edx-exams has its own token signing which will need to go through this same process. See https://github.com/edx/edx-internal/blob/2785fc7599e1f39fbd5387c3da18c0e74883f4b2/argocd/applications/edx-exams/prod-config.yaml#L102-L104 for the config. Note: it seems this setting was designed to just allow a single signing key per service for all custom signed JWTs. Not sure if that is the best design, but that is certainly not an issue for today.

@mumarkhan999: When you get the chance, can you confirm that you read this and you are aware of the other JWT_PRIVATE_SIGNING_JWK cases? This is all I see using https://github.com/search?q=org%3Aedx%20JWT_PRIVATE_SIGNING_JWK&type=code, expect a local config in edx-exams.

@robrap Yeah I read it. And No I wasn't aware of this local config.
Will look into that once start working on that repo.

@mumarkhan999: Is there a top-level Github Issue or Confluence page tracking this work across all services where we can document these types of things?

@robrap Yes we have one. Here's the link
openedx/edx-drf-extensions#290

The ticket does not have much detail for now. But planning to add some there.

Thanks. I added a link to that ticket in the PR description. I think it would be useful to add references to the top-level ticket in PR descriptions and/or commit comments so we could find all the parts from the top-level issue as needed.

openedx/core/djangoapps/oauth_dispatch/docs/decisions/0008-use-asymmetric-jwts.rst

requirements/edx/base.txt

openedx/core/djangoapps/oauth_dispatch/tests/mixins.py

openedx/core/djangoapps/oauth_dispatch/tests/test_views.py

openedx/core/djangoapps/oauth_dispatch/docs/decisions/0008-use-asymmetric-jwts.rst

timmc-edx · 2023-10-16T18:05:45Z

Other than the doc tweaks I think this is ready to go.

mumarkhan999 · 2023-10-16T18:51:15Z

Closing to rerun stuck jobs.

timmc-edx

Looks great, thank you!

Co-authored-by: Tim McCormack <[email protected]>

mumarkhan999 · 2023-10-17T10:32:35Z

@timmc-edx

Hi tim, while testing this PR on sandbox I got this error

    jwt = _create_jwt(request, user, expires_in)
  File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/user_authn/cookies.py", line 319, in _create_jwt
    return create_jwt_from_token(access_token, DOTAdapter(), use_asymmetric_key=True)
  File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/oauth_dispatch/jwt.py", line 110, in create_jwt_from_token
    jwt_token_dict = create_jwt_token_dict(token_dict, oauth_adapter, use_asymmetric_key)
  File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/oauth_dispatch/jwt.py", line 83, in create_jwt_token_dict
    jwt_access_token = _create_jwt(
  File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/oauth_dispatch/jwt.py", line 196, in _create_jwt
    return _encode_and_sign(payload, use_asymmetric_key, secret)
  File "/edx/app/edxapp/edx-platform/openedx/core/djangoapps/oauth_dispatch/jwt.py", line 286, in _encode_and_sign
    jwk = PyJWK(key, algorithm)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/jwt/api_jwk.py", line 60, in __init__
    self.key = self.Algorithm.from_jwk(self._jwk_data)
  File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/jwt/algorithms.py", line 433, in from_jwk
    raise InvalidKeyError(
jwt.exceptions.InvalidKeyError: RSA key must include all parameters if any are present besides d

As far as I remember we have updated the keys. But it seems we have missed a few environments.
Can you please mention for which environments we've updated the keys?
What are the remaining ones?

And also can you please update the keys in the remaining envs so we can merge this PR?

Thanks.

robrap · 2023-10-17T13:46:15Z

Hi tim, while testing this PR on sandbox I got this error

Does this mean that https://github.com/edx/sandbox-internal was missed? (What about https://github.com/edx/edge-internal/ ?) Or were the changes elsewhere?

timmc-edx · 2023-10-17T15:26:04Z

From my notes in edx/edx-arch-experiments#261 it sounds like sandboxes actually generate their keypair during provisioning, rather than being configured with a static value. We deleted the configured sandbox keys some months ago, so something else is going on here. (See that ticket for a list of environments that were updated.)

mumarkhan999 · 2023-10-17T19:18:36Z

I tested this branch on an already available sandbox (triggered on master branch). On master the generate_jwt_signing_key is outdated, that's why the keys are outdated.

Now I triggered a new one using the current branch. Let's see how it works.

edx-pipeline-bot · 2023-10-18T11:18:07Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-10-18T11:51:32Z

2U Release Notice: This PR has been deployed to the edX production environment.

kdmccormick · 2023-10-23T16:12:40Z

Hey @mumarkhan999 , thank you for this upgrade. In the future, when you make a breaking change, please use an exclamation point in your commit message prefix, for example chore!: replace pyjwkest with pyjwt. That'll help Tutor maintainers notice the change and make the appropriate Tutor update.

mumarkhan999 mentioned this pull request May 19, 2023

chore!: replace pyjwkest with PyJWT #31829

Closed

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 5237860 to 57a8f54 Compare May 19, 2023 11:38

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch 5 times, most recently from 052da28 to b026585 Compare May 26, 2023 14:35

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from b026585 to 1a8f1a3 Compare May 26, 2023 14:58

mumarkhan999 requested a review from timmc-edx May 31, 2023 07:22

timmc-edx reviewed Jun 3, 2023

View reviewed changes

openedx/core/djangoapps/oauth_dispatch/docs/decisions/0008-use-asymmetric-jwts.rst Outdated Show resolved Hide resolved

mumarkhan999 mentioned this pull request Jun 6, 2023

Update management command for creating jwks #32377

Merged

2 tasks

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 1ea2427 to c5de62d Compare June 16, 2023 12:51

mumarkhan999 requested review from feanil and a team as code owners June 16, 2023 12:51

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from c5de62d to 58d2482 Compare June 16, 2023 12:58

mumarkhan999 removed request for a team and feanil June 16, 2023 13:00

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch 2 times, most recently from be38fdf to be72f9b Compare June 16, 2023 16:50

mumarkhan999 commented Jun 16, 2023

View reviewed changes

requirements/edx/base.txt Outdated Show resolved Hide resolved

mumarkhan999 requested a review from timmc-edx June 19, 2023 09:29

mumarkhan999 commented Jun 19, 2023

View reviewed changes

openedx/core/djangoapps/oauth_dispatch/tests/mixins.py Outdated Show resolved Hide resolved

mumarkhan999 mentioned this pull request Jun 19, 2023

feat: make jwt decode fucntion generic openedx/edx-drf-extensions#354

Merged

2 tasks

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch 2 times, most recently from 7c3d53c to e28da09 Compare July 4, 2023 13:59

mumarkhan999 commented Oct 13, 2023

View reviewed changes

openedx/core/djangoapps/oauth_dispatch/tests/test_views.py Show resolved Hide resolved

mumarkhan999 marked this pull request as ready for review October 13, 2023 14:12

mumarkhan999 requested a review from robrap October 13, 2023 14:13

timmc-edx reviewed Oct 16, 2023

View reviewed changes

openedx/core/djangoapps/oauth_dispatch/docs/decisions/0008-use-asymmetric-jwts.rst Outdated Show resolved Hide resolved

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 18ecc67 to 4721a82 Compare October 16, 2023 18:30

mumarkhan999 requested a review from timmc-edx October 16, 2023 18:38

mumarkhan999 closed this Oct 16, 2023

mumarkhan999 reopened this Oct 16, 2023

timmc-edx approved these changes Oct 16, 2023

View reviewed changes

mumarkhan999 and others added 7 commits October 17, 2023 12:36

chore: replace pyjwkest with pyjwt

7e820a6

squash! update ADR

c1bc0a6

squash! update ADR

49b82ce

squash! update ADR

e378336

squash! update ADR

6cfdb47

squash! update keys template

cddd3de

Co-authored-by: Tim McCormack <[email protected]>

squash! update ADR

db05c30

mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 49c3504 to db05c30 Compare October 17, 2023 07:36

mumarkhan999 merged commit 92731be into master Oct 18, 2023
61 checks passed

mumarkhan999 deleted the umar/replace-jwkest-with-pyjwt branch October 18, 2023 10:15

kdmccormick mentioned this pull request Oct 25, 2023

[Test failure] TC_LEARNER_7: Can't create an account openedx/wg-build-test-release#318

Closed

iamsobanjaved mentioned this pull request Oct 30, 2023

Remove pyjwkest from generate_jwt_signing_key #32125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace pyjwkest with pyjwt #32270

feat: replace pyjwkest with pyjwt #32270

mumarkhan999 commented May 19, 2023 •

edited

Loading

timmc-edx commented May 23, 2023

mumarkhan999 commented May 26, 2023

timmc-edx Jun 3, 2023

mumarkhan999 Jun 6, 2023 •

edited

Loading

robrap Jun 6, 2023

timmc-edx Jun 14, 2023

robrap Jun 15, 2023

robrap Jun 16, 2023

mumarkhan999 Jun 16, 2023

robrap Sep 12, 2023

mumarkhan999 Sep 13, 2023 •

edited

Loading

robrap Sep 13, 2023

timmc-edx commented Oct 16, 2023

mumarkhan999 commented Oct 16, 2023

timmc-edx left a comment

mumarkhan999 commented Oct 17, 2023

robrap commented Oct 17, 2023

timmc-edx commented Oct 17, 2023

mumarkhan999 commented Oct 17, 2023

edx-pipeline-bot commented Oct 18, 2023

edx-pipeline-bot commented Oct 18, 2023

kdmccormick commented Oct 23, 2023

feat: replace pyjwkest with pyjwt #32270

feat: replace pyjwkest with pyjwt #32270

Conversation

mumarkhan999 commented May 19, 2023 • edited Loading

timmc-edx commented May 23, 2023

mumarkhan999 commented May 26, 2023

Choose a reason for hiding this comment

mumarkhan999 Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mumarkhan999 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timmc-edx commented Oct 16, 2023

mumarkhan999 commented Oct 16, 2023

timmc-edx left a comment

Choose a reason for hiding this comment

mumarkhan999 commented Oct 17, 2023

robrap commented Oct 17, 2023

timmc-edx commented Oct 17, 2023

mumarkhan999 commented Oct 17, 2023

edx-pipeline-bot commented Oct 18, 2023

edx-pipeline-bot commented Oct 18, 2023

kdmccormick commented Oct 23, 2023

mumarkhan999 commented May 19, 2023 •

edited

Loading

mumarkhan999 Jun 6, 2023 •

edited

Loading

mumarkhan999 Sep 13, 2023 •

edited

Loading