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

Fixed idtoken error #322

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Conversation

mambelli
Copy link
Contributor

Fixed writing of cached token as str
and removed in token_util.py extra decoding that was corrupting idtokens and causing the Glidein schedd authentication to fail.

jwt.encode can accept a string as a key, but will encode it in utf-8, not latin-1, causing a different value (corrupted token)

@namrathaurs namrathaurs self-requested a review July 21, 2023 19:32
Copy link
Contributor

@namrathaurs namrathaurs left a comment

Choose a reason for hiding this comment

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

Added few comments which could be considered as suggestions if applicable/relevant.

lib/token_util.py Outdated Show resolved Hide resolved
lib/token_util.py Outdated Show resolved Hide resolved
lib/token_util.py Outdated Show resolved Hide resolved
…ra decoding that was corrupting idtoken

jwt.encode can accept a string as key, but will encode it in utf-8, not latin-1, causing a different value
this caused the subtle error where the IDTOKENs verification was failing.

I removed the encoding, fixed type notation, and
I added also a note in a comment to avoid future changes to repeat the error:
    master_key should be `bytes`. `str` could cause value changes if was decoded not using utf-8.
    The manual (https://pyjwt.readthedocs.io/en/stable/api.html) is incorrect to list `str` only.
    The source code (https://github.com/jpadilla/pyjwt/blob/72ad55f6d7041ae698dc0790a690804118be50fc/jwt/api_jws.py)
    shows `AllowedPrivateKeys | str | bytes` and if it is str, then it is encoded w/ utf-8:  value.encode("utf-8")
Copy link
Contributor

@namrathaurs namrathaurs left a comment

Choose a reason for hiding this comment

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

Changes look good, ready for merging!

@mambelli mambelli merged commit 69eea68 into glideinWMS:master Jul 24, 2023
8 checks passed
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