jwtk / jjwt

Java JWT: JSON Web Token for Java and Android
Apache License 2.0
10.33k stars 1.34k forks source link

SignatureException could include the header and claims #969

Open mnylensc opened 2 months ago

mnylensc commented 2 months ago

Is your feature request related to a problem? Please describe. If JwtParser#parseSignedClaims call results into SignatureException, it's not possible to parse the JWS header or claims from the exception. This would be useful for collecting metrics per key id and audit logging, when you could log the already parsed header and claims.

Describe the solution you'd like Similar to ExpiredJwtException#getHeader() and #getClaims(), SignatureException could also have those methods.

I realize the methods are missing probably because this could be quite dangerous if used wrong, but maybe the methods could be named with dangerously prefix to signify the inherent danger in using the return values...?

Describe alternatives you've considered Decoding the JWS parts myself, but this seems like a waste, when the work to parse the token has already been done by JJWT.

Additional context JJWT version 0.12.6

kesrishubham2510 commented 1 month ago

Hi @mnylensc , is someone looking into it ?? If not can I take it up ?

bdemers commented 1 month ago

Personally, I'd be very cautious about adding this to JJWT, as mentioned above it is potentially dangerous. All that said, it is a commonly requested bit of functionality, we would either have to mark the method in a very obvious way (as @mnylensc mentioned)

Or maybe, make it possible for a user to do the dangerous bits themselves easier. I'm guessing @mnylensc's solution does something like: substring, Base64URLDecode, parse json ?

Again, personally I'm reluctant, but I'd still be interested in other thoughts on where a dangerouslyParseTheInvalidJwsBody() method would live (and how users would find it) 😰

kesrishubham2510 commented 4 weeks ago

Hi @bdemers, consider the below case,

Is this understanding (regarding the danger associated with this feature) correct ? or I'm thinking in the wrong direction 🤔💭

Hey @mnylen, I want to understand how could a header be useful in logging and secondly the developer team would be knowing the header details and it would probably be same for all the granted/generated tokens. Coming to the payload, what if we could add some logic to log only selective claims instead of all of the claims 🫥.

mnylensc commented 4 weeks ago

I think the danger here is that someone would catch the SignatureException and use the claims and/or header contained in the exception in a way that leads to the token being accepted.

Consider the following:

public boolean isValidToken(final String token) {
  try {
    jws = parser.parseSignedClaims(token);
    return isValidClaims(jws.getPayload());
  } catch (SignatureException e) {
    return isValidClaims(jws.getPayload());
  }
}

private boolean isValidClaims(Claims claims) {
  return claims.getAudience().equals("https://myserver");
}

Nobody should write code like this, but it's still a mistake that can be made, and someone will eventually end up making it...

However, similar mistake can be already be made with ExpiredJwtException which contains the claims and the header, although arguably the security impact isn't as high due to the signature at least being correct.

Edit: And of course, there are myriad other ways that using claims from an unverified tokens can cause. Some other possible scenarios that popped into my head:

mnylensc commented 3 weeks ago

However, these dangers I listed above are imo more about the claims. I don't know why I suggested it originally, because the implementation should not even try parsing the claims if the signature verification failed.

For the header, I don't see similar risks (although, arguably, one can include custom non-standard header fields). The parser anyway needs to parse the header to verify the JWT signature, because the header is needed for locating the key. Having SignatureException#getHeader() method would allow users to audit log and collect metrics for verification failures by key id for example. Beyond obvious attack scenarios, these verification failures can happen due to simple configuration mistakes: using incorrect kid for example, or in case the application is juggling multiple public/private key pairs, using the wrong key.

kesrishubham2510 commented 3 weeks ago

@mnylensc the use case regarding metrics collection with 'kid' sounds a good measure to track the SignatureExceptions in case we have multiple options for kid. If I'm not mistaken, this 'kid' identifier can be different in same environment because of specific implementation or be different because of different environment specific keys. Please add if I'm missing something here.

Thanks

nikitocheck commented 2 weeks ago

Actually, this feature makes sense. In my case jws token is provided by identity & access management proxy server. It is completely safe and there is no need for further verification