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

Tokens can be resolved by multiple keys #666

Closed
zhangan12138 opened this issue Jun 6, 2021 · 8 comments
Closed

Tokens can be resolved by multiple keys #666

zhangan12138 opened this issue Jun 6, 2021 · 8 comments
Labels
stale Stale issues pending deletion due to inactivity

Comments

@zhangan12138
Copy link

Hello, I was confused when I used JJWT in the test. When I used 'XXXX' as the key to get the token, I used 'XXXXX ',' XXXXXX' and 'XXXXXXX' to successfully resolve the token. I wonder if I made a mistake in the operation

question.txt

@bdemers
Copy link
Member

bdemers commented Jun 7, 2021

Hi @zhangan12138!

Your attached doesn't reproduce the issue you are describing, there are a few problems:

  1. Your keys xxxx and xxxxxxx are not valid, when running this example you should see output similar to:

The JWT JWA Specification (RFC 7518, Section 3.2) states that keys used with HS256 MUST have a size >= 256 bits (the key size must be greater than or equal to the hash output size). Consider using the io.jsonwebtoken.security.Keys class's 'secretKeyFor(SignatureAlgorithm.HS256)' method to create a key guaranteed to be secure enough for HS256. See https://tools.ietf.org/html/rfc7518#section-3.2 for more information.

  1. Your test uses a pre-generated JWS, and while that is sufficient if you know how the JWS was created, just reading this code it isn't clear. My suggestion is to merge your two test methods together, use the output from the first as the input for the second. Or if you are looking to use a static input for the second test, document how that JWT was created.

  2. The example uses deprecated methods (this should still work, but makes the example a little hard to follow)

  3. They key in the method you are using should be base64 encoded.

@Marcono1234
Copy link
Contributor

@zhangan12138 it looks like you fell for the misleading parse(...) method:

Claims claim = (Claims) Jwts.parser()
        .setSigningKey("xxxxxxx")
        .parse(token)
        .getBody();

Sadly the documentation does not make it very clear, but this method does not verify the signature of the token, see #212.
You need to use either parseClaimsJws or parsePlaintextJws (the name is somewhat misleading, "plaintext" refers here to the payload being non-JSON, but signature verification is performed).

@bdemers
Copy link
Member

bdemers commented Jun 16, 2021

Changing the method functionality is a breaking change, but it's something that we are looking into for 1.0

I think most folks confuse assume a JWT is always JWS, so the default parser needs a little work. We are also working on supporting JWE's support which will help flush how to clarify the difference between the different types of JWTs

@Marcono1234
Copy link
Contributor

@bdemers, there is probably still some work left until version 1.0 will be released and from what I have read it will be a backward incompatible release, so users will likely be hesitant upgrading their dependency.
Do you think it would make sense to already perform the method renaming before 1.0 to make users aware of their potentially unsafe usage as soon as possible?
#658 only renames the methods, their existing behavior is kept. The only possible incompatibility could arise for users implementing the JwtParser interface.

@bdemers
Copy link
Member

bdemers commented Jun 21, 2021

Hey @Marcono1234,

I'm not sure I've following 100%. Renaming methods is a breaking change and isn't backward compatible.

@Marcono1234
Copy link
Contributor

Sorry for being imprecise there, the pull request is not actually "renaming" the methods, instead it annotates the old methods with @Deprecated and adds new methods with more expressive names to which the deprecated methods delegate.
So existing code is not breaking due to this (unless it implements JwtParser, which is unlikely?), but it will cause deprecation warnings.

@stale stale bot added the stale Stale issues pending deletion due to inactivity label Aug 21, 2021
@jwtk jwtk deleted a comment from stale bot Aug 21, 2021
@stale stale bot removed the stale Stale issues pending deletion due to inactivity label Aug 21, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale due to inactivity for 60 or more days. It will be closed in 7 days if no further activity occurs.

@stale stale bot added the stale Stale issues pending deletion due to inactivity label Apr 16, 2022
@zhangan12138
Copy link
Author

thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issues pending deletion due to inactivity
Projects
None yet
Development

No branches or pull requests

3 participants