Remove ecdsa dependency #403
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
@justinabrokwah-okta - I'm curious to get your thoughts here, I think you might be the most familiar with the current implementation. |
Contributor
Thanks so much for this PR, sorry for the delay in response. I'll try to take a look at this today but skimming through, it looks like an overdue change to make. Also tagging @bryanapellanes-okta for visibility |
Contributor
|
@agburch Thank you so much for your contribution! I'll review this week and consult with @justinabrokwah-okta . If no issues or concerns arise, I'll plan to merge and publish a new release by the end of next week. |
justinabrokwah-okta
approved these changes
May 21, 2024
This was referenced May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR proposes to remove the
ecdsadependency completely by refactoring to usejwcryptoandpyjwt. This change is motivated by the recent CVE that demonstrated the potential vulnerabilities inecdsa. But regardless of any specific vuln, the maintainers of theecdsalibrary have commented to suggest that it is not intended for "production use":https://github.com/tlsfuzzer/python-ecdsa?tab=readme-ov-file#Security
By removing
python-josethis PR does remove functionality from thejwtmodule (e.g. specifying alternate hash algorithms or JWT_OPTIONS). But as of today, these features appear to be unused. However please let me know if I'm mistaken here. As far as this PR is concerned though, the output ofJWT.create_token()has not changed.I believe this PR extends the work done in PR #398 by fully removing the package in question. It should also address:
Issues #395
Issues #388