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

Fixes #964: Validate key against allowed types for Algorithm family #985

Merged
merged 15 commits into from
Feb 11, 2025

Conversation

pachewise
Copy link
Contributor

@pachewise pachewise commented Sep 12, 2024

  • add tests, make sure current test suite passes
  • remove TODOs

Following discussion in #964, this PR adds checks to make sure the key loaded by any of the has_crypto = True Algorithm children is of the correct type for that Algorithm family. I still need to figure out how to check whether it's the right "flavor" of the algo family (for example, that the SHA256 algo is encoding/decoding using a key generated via SHA256).

@pachewise pachewise force-pushed the fix/964 branch 2 times, most recently from dbf2c25 to 20e81e2 Compare September 12, 2024 20:59
@github-actions github-actions bot added the stale Issues without activity for more than 60 days label Nov 12, 2024
@github-actions github-actions bot closed this Nov 19, 2024
@auvipy auvipy reopened this Feb 5, 2025
@pachewise pachewise marked this pull request as ready for review February 5, 2025 15:24
@pachewise
Copy link
Contributor Author

@auvipy I think this is ready for review, though I'm working on the tests today. Been a while since I opened this PR, so some decisions may not be as good anymore; generally, I tried to keep the same "logic" of the code (keeping multiple returns, etc) while just inserting the self.check_crypto_key_type() before returning.

@github-actions github-actions bot removed the stale Issues without activity for more than 60 days label Feb 6, 2025
@@ -163,6 +199,30 @@ def compute_hash_digest(self, bytestr: bytes) -> bytes:
else:
return bytes(hash_alg(bytestr).digest())

def check_crypto_key_type(self, key: PublicKeyTypes | PrivateKeyTypes):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also considered putting this in a sub-ABC "CryptoAlgorithm", but that seemed like more changes for not as much value. Let me know if you'd rather I do that instead, or if you would prefer an alternate approach.

@auvipy auvipy self-requested a review February 6, 2025 06:36
jwt/algorithms.py Outdated Show resolved Hide resolved
@auvipy auvipy merged commit d5ca701 into jpadilla:master Feb 11, 2025
23 checks passed
@pachewise pachewise deleted the fix/964 branch February 11, 2025 20:42
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.

2 participants