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

Misleading error message when validating audience of JWT against check_claims #239

Closed
dollarklavs opened this issue Sep 13, 2021 · 7 comments · Fixed by #240
Closed

Misleading error message when validating audience of JWT against check_claims #239

dollarklavs opened this issue Sep 13, 2021 · 7 comments · Fixed by #240

Comments

@dollarklavs
Copy link

dollarklavs commented Sep 13, 2021

Im confused by the error message when validating a JWT's aud claim against the check_claims parameter when initalizing a JWT.
Below is an example to clarify

from jwcrypto import jwt, jwk
claims = {"aud": "www.example.com"}
check_claims = {"aud": ["www.example.com", "account"]}
key = jwk.JWK(generate='oct', size=256)
token = jwt.JWT(header={"alg": "HS256"}, claims=claims)
verified_token = jwt.JWT(key=key, jwt=token.serialize(), check_claims=check_claims)

this throws jwcrypto.jwt.JWTInvalidClaimValue: Invalid 'aud' value. Expected 'www.example.com' to be in '['www.example.com', 'account']'

Which is a mystifying error as

'www.example.com' in ['www.example.com', 'account'] == True

I'm not sure if there is a problem in the code or if "I'm just doing it wrong"

@simo5 simo5 added the bug label Sep 14, 2021
@simo5
Copy link
Member

simo5 commented Sep 14, 2021

This sounds like a bug.
I do not see yet why this is happening as the code internally explicitly supports either a string or a list for 'aud' claims.

@simo5
Copy link
Member

simo5 commented Sep 14, 2021

What version of jwcrypto are you using ?

@dollarklavs
Copy link
Author

I'm on 1.0

@dollarklavs
Copy link
Author

This sounds like a bug.
I do not see yet why this is happening as the code internally explicitly supports either a string or a list for 'aud' claims.

Internally it looks as if there is a if branch if the aud claim on the the jwt is a list, but there's no check for the case that the aud value in check_claims is a list.
I don't even know if the aud value on check_claims should be allowed to be a list?

And if that is allowed how should it be handled?
Maybe each item in one list should be evaluated against all items in the other list?

@simo5
Copy link
Member

simo5 commented Sep 14, 2021

Ok, I thought this was a bug, but after creating a patch for it I realized it looks to me you are using this backwards.

The 'aud' claim in the token can be a list, because the token can be defined to be intended for multiple host/ervices/entities.
But when you do check the claim a list doesn't make a lot of sense.
In checking you want to check that the entity testing the claim is the actual audience so generally check_claims will contain only the name of the host/service/entity that is performing the verification.

What is the context in which this bug arose?
@dollarklavs Do you really have an entity that is impersonating multiple recipients?

@dollarklavs
Copy link
Author

dollarklavs commented Sep 14, 2021

I think you’re right and that I misused the aud in check_claims.
But how about a check on the check_claims aud to guarantee that it’s not at list then?

@simo5
Copy link
Member

simo5 commented Sep 14, 2021

I think you’re right and that I misused the aud in check_claims.
But how about a check on the check_claims aud to guarantee that it’s not at list then?

Yeah I guess we can check that the check_claims are strings as well, wherever appropriate.

simo5 added a commit to simo5/jwcrypto that referenced this issue Sep 14, 2021
Issue latchset#239 brought up that check_claims are not actually checked to be
of the right type and this can lead to confusion in some case, as well
as defer error reporting after potentially costly signature
computations.

Check for general claims type validity upfront where appropriate.

Resolves latchset#239

Signed-off-by: Simo Sorce <[email protected]>
@simo5 simo5 added enhancement and removed bug labels Sep 14, 2021
simo5 added a commit to simo5/jwcrypto that referenced this issue Sep 17, 2021
Issue latchset#239 brought up that check_claims are not actually checked to be
of the right type and this can lead to confusion in some case, as well
as defer error reporting after potentially costly signature
computations.

Check for general claims type validity upfront where appropriate.

Resolves latchset#239

Signed-off-by: Simo Sorce <[email protected]>
simo5 added a commit that referenced this issue Sep 17, 2021
Issue #239 brought up that check_claims are not actually checked to be
of the right type and this can lead to confusion in some case, as well
as defer error reporting after potentially costly signature
computations.

Check for general claims type validity upfront where appropriate.

Resolves #239

Signed-off-by: Simo Sorce <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants