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

jwt.encode() overrides the algorithm specified by the caller #725

Closed
hiranya911 opened this issue Mar 26, 2021 · 4 comments · Fixed by #729
Closed

jwt.encode() overrides the algorithm specified by the caller #725

hiranya911 opened this issue Mar 26, 2021 · 4 comments · Fixed by #729
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@hiranya911
Copy link
Contributor

The jwt.encode() function always set the alg JWT header to either ES256 or RS256, even if the caller had specified an explicit algorithm:

if es256 is not None and isinstance(signer, es256.ES256Signer):
header.update({"alg": "ES256"})
else:
header.update({"alg": "RS256"})

This function should only set the alg header when the caller hasn't specified one. This is required to support some use cases in Firebase emulator, where we need the ability to mint JWTs with alg header values like none.

@hiranya911
Copy link
Contributor Author

It might also make sense to read the exact alg header to use from the google.auth.crypt.Signer interface, since those concepts are close related. Something like:

if 'alg' not in header:
    if signer.alg:
        header['alg'] = signer.alg
    elif es256 is not None and isinstance(signer, es256.ES256Signer): 
       header.update({"alg": "ES256"}) 
    else: 
       header.update({"alg": "RS256"})

Let me know if this makes sense, and I'd be happy to provide a PR to this effect.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Mar 27, 2021
@busunkim96
Copy link
Contributor

@arithmetic1728 @silvolu PTAL

@busunkim96 busunkim96 removed the triage me I really want to be triaged. label Mar 29, 2021
@hiranya911
Copy link
Contributor Author

Also adding @yuchenshi and @samtstern from Firebase for visibility.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Mar 30, 2021
@busunkim96 busunkim96 added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Mar 30, 2021
@arithmetic1728
Copy link
Contributor

@hiranya911 I created a PR to use alg from header if provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants