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.

Sign up for GitHub

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

Jump to bottom

Compatibility Fixes for PyJWT/Cryptography Versions #222

Closed
wants to merge 3 commits into from
Closed

Compatibility Fixes for PyJWT/Cryptography Versions #222

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28f7e68
update at_hash check to handle byte or str type returned from base64.…
jrschiestle Mar 7, 2024
e39a0d3
allow id and access tokens passed as str or bytestr to Cognito class …
jrschiestle Mar 7, 2024
8317fae
Merge branch 'master' into master
ludeeus May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pycognito/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,8 +277,15 @@ def verify_token(self, token, id_name, token_use):
# Compute and verify at_hash (formerly done by python-jose)
if "at_hash" in verified:
alg_obj = jwt.get_algorithm_by_name(header["alg"])
digest = alg_obj.compute_hash_digest(self.access_token)
at_hash = base64.urlsafe_b64encode(digest[: (len(digest) // 2)]).rstrip("=")
try:
Copy link

Choose a reason for hiding this comment

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

Can you provide more details about what this is supposed to fix?
The linked issue (#223) points more to how the access_token provided for the Cognito class changed.

The current code follows what the JWT library suggests for this:
https://github.com/jpadilla/pyjwt/blob/master/docs/usage.rst#oidc-login-flow (at the bottom)

Copy link
Author

Choose a reason for hiding this comment

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

This is supposed to deal with functions expecting strings vs bytes differently with updated versions of the cryptography dependency.

The try-catch block allows the digest to be computed in either case and then the at_hash step is split based on the type returned by the digest computation since this changes depending on so that the strip step does not fail.

The idea was not to change function but to make it robust to some typing mismatches that were occurring. That said, I am not experiencing the issue now with python 3.11.9 and the updated dependencies.

Copy link

Choose a reason for hiding this comment

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

Okey, that is why I asked, beacuse after looking at different versions, there is no differences between them in what they return or expect as inputs.

Copy link

Choose a reason for hiding this comment

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

Going to close this one for now, as it no longer seems to be needed.
Thanks anyway 👍

digest = alg_obj.compute_hash_digest(self.access_token)
except TypeError:
digest = alg_obj.compute_hash_digest(self.access_token.encode("utf-8"))
at_hash = base64.urlsafe_b64encode(digest[: (len(digest) // 2)])
if isinstance(at_hash, bytes):
at_hash = at_hash.rstrip(b"=").decode("utf-8")
else:
at_hash = at_hash.rstrip("=")
if at_hash != verified["at_hash"]:
raise TokenVerificationException(
"at_hash claim does not match access_token."
Expand Down