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

chore!: replace pyjwkest with PyJWT #31829

Closed
wants to merge 6 commits into from

Conversation

iamsobanjaved
Copy link
Contributor

Description

Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.

Useful information to include:

  • Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
    "Developer", and "Operator".
  • Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
  • Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
    changes.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for testing this change.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other information

Include anything else that will help reviewers and consumers understand the change.

  • Does this change depend on other changes elsewhere?
  • Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
  • If your database migration can't be rolled back easily.

@@ -4303,7 +4303,7 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring
CREDIT_TASK_MAX_RETRIES = 5

# Dummy secret key for dev/test
SECRET_KEY = 'dev key'
SECRET_KEY = 'dev-key'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change isn't a blocker, it will be reverted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does PyJWT forbid the underscore for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this change early in the process but will be reverted in the next commit as it isn't needed anymore. PyJWT doesn't base64 encode if we create PyJWK and then use that for signing the token, so added that manually here in this line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Hmm. So we won't need to change SECRET_KEY in configs, right?

@timmc-edx
Copy link
Contributor

timmc-edx commented Mar 14, 2023

It looks like p, q, and the other optional values are for performance optimization. If you back out the changes to lms/envs/test.py and then delete the p and q fields, tests pass. (This is also true on the master branch.) There's code in PyJWT's jwt.algorithms that fills in the optional parameters if they're missing, cryptojwt.jwk.jwk has similar. [EDIT: I meant to look in pyjwkest instead. I couldn't find that code in that library; I suspect it's actually done in pycryptodome.] I believe we should be able to update the stage and prod keys to remove these values, making them forward-compatible with these code changes. (We would also need to include these instructions in the Release Notes.)

My only concern is that I don't know how significant the performance impact is. However, I notice that _encode_and_sign currently parses JSON and creates a JWS object every time it is needed—so we could actually improve on existing performance by having that be done just once in a memoized function. (Django settings aren't supposed to change at runtime.)

@timmc-edx
Copy link
Contributor

timmc-edx commented Mar 15, 2023

After some discussion, the approaches I see:

  1. As above, just remove the optional parameters and take the performance hit. Except... I don't think the caching option that I suggested works with pyjwkest, since the JWS object is built around the payload, not the keys. PyJWT takes a better approach, but that doesn't help us during the transition period.
  2. Rotate signing keys while staying on pyjwkest, ensuring that the new key has all of the optional fields, and then switch to PyJWT. Pretty straightforward, although it would require updating public keys on a bunch of IDAs.
  3. Update all keys to include all optional parameters. We could write a small script which, given a private key, strips out any optional params and then calls the from_jwk and to_jwk methods on jwt.algorithms.RSAAlgorithm. This has the effect of filling in the optional parameters.

At this point I'm favoring option 2 because it entails the least amount of new code.

timmc-edx added a commit that referenced this pull request Mar 15, 2023
This doesn't actually work with pyjwkest, but will with PyJWT. It was
intended for use with #31829
but it won't actually be usable until after the upgrade.
@timmc-edx timmc-edx self-assigned this Apr 26, 2023
@timmc-edx
Copy link
Contributor

The keys for stage, prod, and edge (though not sandbox) have been enhanced by the new https://github.com/openedx/edx-platform/blob/master/scripts/jwk-precompute-params.py script, which means we can now move forward with this work. Could you run the old signing key through that script? See openedx/edx-drf-extensions#337 for a formatting suggestion that allows easier inspection and copy/paste of JSON.

Also, could you change the commit and PR commit type to feat!? chore is really just for completely mechanical changes that are "uninteresting", like routine dependency upgrades that are thought not to have any effect, or translation updates, etc. Since these commit types are used in generating changelogs, it's better to err on the side of feat/fix -- chore gets hidden. (This goes for the other PRs in this group as well.)

@mumarkhan999 mumarkhan999 force-pushed the iamsobanjaved/replace-pyjwkest-authlib branch from b90a927 to 845dbac Compare May 19, 2023 10:57
@mumarkhan999
Copy link
Member

mumarkhan999 commented May 19, 2023

Closing this PR in favor of #32270

@mumarkhan999 mumarkhan999 deleted the iamsobanjaved/replace-pyjwkest-authlib branch May 19, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants