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

JwtParser throws wrong exception when signing key is HMAC but token signed with RSA #438

Closed
mbarkley opened this issue Feb 21, 2019 · 8 comments · Fixed by #811
Closed
Milestone

Comments

@mbarkley
Copy link

mbarkley commented Feb 21, 2019

Given:

  • A parser with an HS256 signing key.
  • Key was specified using JwtParser.setSigningKey(String base64Encoded).
  • Token being parsed is signed with algorithm RS256.
  • Using the parseClaimsJws(String) method.
  • Using jjwt-impl version 0.10.5

Expected:

  • A JwtException of some kind.

Observed:

  • An IllegalArgumentException with the following message: "Key bytes can only be specified for HMAC signatures. Please specify a PublicKey or PrivateKey instance."
@lhazlewood
Copy link
Contributor

lhazlewood commented Mar 9, 2019

I think I understand what you're saying - let me reformulate it to see if we're on the same page:

The current exception indicates that the jws specified is an illegal argument because of how the parser was configured. But this isn't obvious because the internal parser key is expecting a different type of JWS, so that should be made more obvious/explicit with a more concrete JwtException - that explains not just that the argument was illegal, but why it was illegal. Is that correct?

If so (and I think that's what you're saying), I agree with you, but we can't change runtime exception types until a major version increase (i.e. 1.0). This is because any existing logic expecting to catch IllegalArgumentException today wouldn't trigger at all if we change the exception type.

As such, I can assign this to a v 1.0 tag. Please confirm.

@lhazlewood lhazlewood added this to the 1.0 milestone Mar 9, 2019
@mbarkley
Copy link
Author

But this isn't obvious because the internal parser key is expecting a different type of JWS, so that should be made more obvious/explicit with a more concrete JwtException - that explains not just that the argument was illegal, but why it was illegal. Is that correct?

Yes, that's correct.

I agree with you, but we can't change runtime exception types until a major version increase (i.e. 1.0). This is because any existing logic expecting to catch IllegalArgumentException today wouldn't trigger at all if we change the exception type.

Understood. Thanks for following up.

@lhazlewood
Copy link
Contributor

Thanks for the clarification!

@NagaAtLv
Copy link

@mbarkley Hey, how did you get away with this exception, may i know how did you resolve it?

@mbarkley
Copy link
Author

@NagaAtLv we catch the IllegalArgumentException and treat it as an authentication error (i.e. the token has the wrong kind of signature). It's not ideal because it could potentially obscure other non security errors that throw that exception type, so we've tried to put the catch at the lowest level possible to avoid catching other errors accidentally.

@NagaAtLv
Copy link

@mbarkley Thanks for the prompt response, it is rather surprising, when i tried to parse the token generated through Azure AD, i can view the json object on jwt.io and other parsing websites... so i just wondering how can i get past this one.

@KaustubhJha-TUL-UpperFunnel

Did you find a solution ? Creating a new HmacKey by jeo4j gives the exception below that one and creation of new public/private key also gives error

@Tejshri47
Copy link

@NagaAtLv have you got solution on this I am also facing same exception while working on AzureAD

lhazlewood added a commit that referenced this issue Sep 5, 2023
@lhazlewood lhazlewood modified the milestones: 1.0, 0.12.0 Sep 5, 2023
lhazlewood added a commit that referenced this issue Sep 5, 2023
lhazlewood added a commit that referenced this issue Sep 5, 2023
* Added test case for #438
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 a pull request may close this issue.

5 participants