jwtk/jjwt

parseSignedClaims(JWE) calls key locator

laurids opened this issue · 2 comments

Is your feature request related to a problem? Please describe.
The JavaDoc for parseSignedClaims says that it throws an exception if the input string is not actually a JWS: https://github.com/jwtk/jjwt/blob/0.12.3/api/src/main/java/io/jsonwebtoken/JwtParser.java#L324
But if the input string is a JWE, the execution ends up calling the key locator instead of throwing an exception:
https://github.com/jwtk/jjwt/blob/0.12.3/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParser.java#L540
In our key locator, to lookup the key, we are using the key ID and certificate thumbprint from the JwsHeader. However, now we cannot just cast the Header to JwsHeader, if we want to avoid running into a generic ClassCastException.

Describe the solution you'd like
That jjwt throws exception before calling locate on the key locator when you use parseSignedClaims on a JWE.

Describe alternatives you've considered
To avoid a generic ClassCastException, we have put a type check in our key locator:

    if (!(header instanceof final JwsHeader jwsHeader)) {
      // throwing custom exception...
    }
    final var keyId = jwsHeader.getKeyId();
    final var certificateThumbprint = jwsHeader.getX509Sha256Thumbprint();

Additional context
N/A

This is expected behavior.

.parseSignedClaims will still throw an exception before returning, it's just that your Locator is being called before it has the ability to throw the exception.

Per the parseSignedClaims JavaDoc:

This is a convenience method logically equivalent to the following:

parse(jws).accept(Jws.CLAIMS);

So it is fully parsed/verified first (via the .parse(jws) call) and then asserted that is the expected type (via the .accept(Jws.CLAIMS) call. Verification (during .parse) requires calling the key Locator if available, so that's why it is invoked before the type assertion occurs.

The JwtParserBuilder's .keyLocator method JavaDoc shows in the code example that the base .locate method implementation doesn't know what type of Header instance will be passed, and uses instanceof to determine the type:

* Jws<Claims> jws = Jwts.parser().keyLocator(new Locator<Key>() {
* @Override
* public Key locate(Header<?> header) {
* if (header instanceof JwsHeader) {
* return getSignatureVerificationKey((JwsHeader)header); // implement me
* } else {
* return getDecryptionKey((JweHeader)header); // implement me
* }
* }})
* .build()
* .parseSignedClaims(compact);
* </pre>

Even so, if you only ever need to support JWSs, you can simply subclass the LocatorAdapter class as showin in the https://github.com/jwtk/jjwt#key-locator documentation:

public class MyKeyLocator extends LocatorAdapter<Key> {
    
    @Override
    public Key locate(ProtectedHeader<?> header) { // a JwsHeader or JweHeader
        // implement me
    }
}

But instead of overriding locate(ProtectedHeader), override locate(JwsHeader) instead:

public class MyKeyLocator extends LocatorAdapter<Key> {
    
    @Override
    public Key locate(JwsHeader header) { // only ever called for JWSs
        // implement me
    }
}

The locator will still be called when parsing a JWE, but it will always return null to indicate no key is found for such a header instance and will not be decrypted (i.e. it's a 'no op'), and an exception thrown.

Based on this, there doesn't appear to be a need to change JJWT's code, so I'm closing the issue, but feel free to continue to discuss if you need any clarification.

Ok, thanks for the hint. It is a little more suitable to extend from LocatorAdapter since we only need to support JWS :)