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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace pyjwkest with pyjwt #311

Merged
merged 1 commit into from
May 16, 2023

Conversation

mumarkhan999
Copy link
Member

@mumarkhan999 mumarkhan999 commented Mar 16, 2023

As we're removing pyjwkest from our dependencies, this PR is in the continuation of that task.

In this PR the JWK private-public keypair has been updated as the new package PyJWT which we're using instead of Pyjwkest is not compatible with the older ones.


Similar PRs

Another ticket

Slack thread [Private]

Tool used for JWK keypair creation

Previous attempts

@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch 4 times, most recently from 43ff502 to 70f2003 Compare March 16, 2023 12:21
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 70f2003 to f819431 Compare March 16, 2023 14:49
@mumarkhan999 mumarkhan999 changed the title feat: replace pyjwkest with pyjwt chore: replace pyjwkest with pyjwt Mar 16, 2023
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from f819431 to cf402f5 Compare March 20, 2023 07:33
test_settings.py Outdated Show resolved Hide resolved
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch 2 times, most recently from 40d63b8 to 7750a04 Compare March 24, 2023 15:50
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 09f0927 to 46e21b4 Compare April 6, 2023 10:44
@timmc-edx
Copy link
Contributor

timmc-edx commented Apr 26, 2023

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.)

timmc-edx added a commit that referenced this pull request Apr 26, 2023
This would be my suggestion for how to handle the existing test settings
in <#311>.

- Using a multiline string rather than a collection of strings allows
  developers to copy and paste the JSON freely, without having to splice it
  back together.
- Run the `JWT_PRIVATE_SIGNING_JWK` through the new
  `jwk-precompute-params.py` script in edx-platform.

Keeping the original keypair (but upgrading the signing key) allows the
diff to show exactly what has changed. Deployers will be upgrading their
own production keys, so it's best to have this match their upgrade process
from Palm to Quince as closely as possible.
timmc-edx added a commit that referenced this pull request Apr 26, 2023
This would be my suggestion for how to handle the existing test settings
in <#311>.

- Using a multiline string rather than a collection of strings allows
  developers to copy and paste the JSON freely, without having to splice it
  back together.
- Run the `JWT_PRIVATE_SIGNING_JWK` through the new
  `jwk-precompute-params.py` script in edx-platform. Note that the `p` and
  `q` parameters switch place. This should be harmless -- I believe those
  are the two original primes in the RSA key, and it doesn't matter which
  is which.

Keeping the original keypair (but upgrading the signing key) allows the
diff to show exactly what has changed. Deployers will be upgrading their
own production keys, so it's best to have this match their upgrade process
from Palm to Quince as closely as possible.
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch 3 times, most recently from 3aa3806 to 40d971d Compare May 9, 2023 08:02
@mumarkhan999 mumarkhan999 changed the title chore: replace pyjwkest with pyjwt feat: replace pyjwkest with pyjwt May 9, 2023
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 40d971d to f87bfd6 Compare May 9, 2023 08:37

if add_symmetric_keys:
# symmetric key
key_set.add({'key': jwt_issuer['SECRET_KEY'], 'kty': 'oct'})
# symmetric_key
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] Looks like an accidental change here of a space to an underscore.

@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from f87bfd6 to fab29c5 Compare May 15, 2023 08:13
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from fab29c5 to 676126e Compare May 16, 2023 10:55
@mumarkhan999 mumarkhan999 force-pushed the umar/replace-jwkest-with-pyjwt branch from 676126e to f50a9a7 Compare May 16, 2023 10:56
@mumarkhan999 mumarkhan999 merged commit 7ee9917 into master May 16, 2023
8 checks passed
@mumarkhan999 mumarkhan999 deleted the umar/replace-jwkest-with-pyjwt branch May 16, 2023 12:14
@timmc-edx
Copy link
Contributor

Looks like you dropped the 8.8.0 version bump again. I'll add that in a new PR and do a release.

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

2 participants