Misuse of Jackson ObjectMapper

[arlampin]

I noticed java-jwt doesn't use Jackson's ObjectMappers in the most optimal way. They are meant to be fully thread-safe (after configuration) and reusable, as their construction is a relative heavy process. Instead, java-jwt seems to create a new one for almost any operation, including JWTParser and JWTCreator construction. Even some getter-type parsing methods create one just to parse a single value.

In practice, they could be replaced with a single static instance.

[mhriemers]

I don't know what Jackson says about this but when these ObjectMappers are created, they are configured differently across this codebase. Re-using (and thus re-configuring) an ObjectMapper doesn't seem thread-safe to me at all. Could you provide some more insight and possibly a PR?

Adding to that, I think most applications either create or verify a JWT, which would only result in one ObjectMapper instance being created.

[arlampin]

As far as I can tell, every call to JWTVerifier.verify() creates a new JWTDecoder, which creates a new JWTParser, which creates a new ObjectMapper. So even if application only verifies tokens, each verification creates a separate ObjectMapper.

Furthermore, at least JsonNodeClaim creates a single-use ObjectMapper for asArray, asList and as methods, as does PayloadDeserializer.getStringOrArray. I think most, if not all, of these could be merged, as I don't think their configurations clash with each other.

For reference, http://stackoverflow.com/a/3909846/434429 (answer by Jackson's developer)

[derandreasberger]

Hello!
For our microservice applications we have implemented a ResourceServerTokenServices to do local JWT OAuth2 token validation using the JwtVerifier-class.
Recently we figured out that roughly 10% of our requests fail due to JsonParseExceptions. I was able to verify the underlying usage of JWTVerifier was causing the problem. Rough description: We have one service-class (singleton) which is responsible for the token-validation, this service-class utilizes JWTVerifier.class' method verify(token).
Since the constructor-call looks like this: "new JWTVerifier(this.getPublicKeyFromCertificate(), this.getAudience(), this.getIssuer());", I assumed we could use this object thread-safe for multiple verify()-calls.
Turns out this does not work very well. Only when creating a new instance of JWTVerifier each time we want to verify a token, everything's ok. I'd like to point out that this is not much more expensive in terms of calculation power (roughly 5%), but please add a bold hint / javadoc, so everyone can see how this class-method should be invoked!

com.auth0.jwt.internal.com.fasterxml.jackson.core.JsonParseException: Illegal character ((CTRL-CHAR, code 0)): only regular white space (\r, \n, \t) is allowed between tokens at [Source: java.io.StringReader@2c28b43; line: 1, column: 2] at com.auth0.jwt.internal.com.fasterxml.jackson.core.JsonParser._constructError(JsonParser.java:1284) at com.auth0.jwt.internal.com.fasterxml.jackson.core.base.ParserMinimalBase._reportError(ParserMinimalBase.java:588) at com.auth0.jwt.internal.com.fasterxml.jackson.core.base.ParserMinimalBase._throwInvalidSpace(ParserMinimalBase.java:534) at com.auth0.jwt.internal.com.fasterxml.jackson.core.json.ReaderBasedJsonParser._skipWSOrEnd(ReaderBasedJsonParser.java:1493) at com.auth0.jwt.internal.com.fasterxml.jackson.core.json.ReaderBasedJsonParser.nextToken(ReaderBasedJsonParser.java:380) at com.auth0.jwt.internal.com.fasterxml.jackson.databind.ObjectMapper._initForReading(ObjectMapper.java:2601) at com.auth0.jwt.internal.com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:2548) at com.auth0.jwt.internal.com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:1805) at com.auth0.jwt.JWTVerifier.decodeAndParse(JWTVerifier.java:236) at com.auth0.jwt.JWTVerifier.verify(JWTVerifier.java:109)

[hzalaz]

@derandreasberger the verifier is meant to be reusable, can you provide more info since the exception can be caused by something unrelated to the lib?

[derandreasberger]

@hzalaz I think i can not provide the whole bunch of code, but I'd like to display the problematic piece of Constructor/method invocation.
The original class had a private variable holding the JWTVerifier (method this.setJwtVerifier(...), respectively this.getJwtVerifier()). This needed to be replaced with a new class initiation.

private boolean verifyTokenAndSignature(String token) throws InvalidKeyException {

        boolean tokenVerificationOk = false;

        try {

/*DDS-1751: Concurrency issue fix (JWTVerifier is not thread-safe, checkout LocalTokenVerificationSingletonLoadTest for testing)
            if (this.getJwtVerifier() == null) {
                this.initTokenValidator();
            }
            Map<String, Object> map = this.getJwtVerifier().verify(token);
*/
            
JWTVerifier jwtVerifier = new JWTVerifier(this.getPublicKeyFromCertificate(), this.getAudience(), this.getIssuer());
            
            Map<String, Object> map = jwtVerifier.verify(token);

            LocalTokenValidation.logger
                    .debug("JwtVerifier return keys: " + Arrays.deepToString(map.keySet().toArray()));

            if (map != null && !map.isEmpty()) {
                LocalTokenValidation.logger.info("No Exception thrown, verification ok!");
                tokenVerificationOk = true;
            }

        } catch (InvalidKeyException | SignatureException | JWTVerifyException e) {
            
            LocalTokenValidation.logger.error("InvalidKeyException check if parameters are correct (aud: " + this.getAudience() + ", iss: " + this.getIssuer() + ", certificate: " + this.getCertifcateFileName() + ")", e);
            throw new InvalidKeyException(e);
            
        } catch (NoSuchAlgorithmException | IllegalStateException | IOException e) {

            LocalTokenValidation.logger.error("InvalidKeyException caused by ", e);
            throw new InvalidKeyException(e);

        }

        return tokenVerificationOk;
    }
    
    public void initTokenValidator() {

        LocalTokenValidationWithSingletonVerifier.logger.debug("Initializing TokenValidator");

        this.setJwtVerifier(
                new JWTVerifier(this.getPublicKeyFromCertificate(), this.getAudience(), this.getIssuer()));

    }```


+ Adding a JUnit-snippet part so you can see roughly how I tested the verification in terms of concurrency. If you implement some RunVerificationHelper-class which either uses a singleton or a new instance of JWTVerifier, it should be possible to reproduce the issue.

```    private List<RunVerificationHelper> startParallelTokenVerification() {
        int numberOfParallelVerificationThreads = 1000;
        
        List<RunVerificationHelper> verificators = new ArrayList<RunVerificationHelper>();

        for (int i = 0; i < numberOfParallelVerificationThreads; i++) {
            verificators.add(new RunVerificationHelper(i));
        }

        List<RunVerificationHelper> verificatorRejects = verificators.parallelStream()
                .map(p -> p.runValidation(this.token, this.localTokenValidation))
                .filter(p -> !((RunVerificationHelper)p).isVerificationSuccess())
                .collect(Collectors.toList());
        
        if (verificatorRejects.size() > 0) {
            System.out.println(" ### ERROR ### count=" + verificatorRejects.size() + ", fail ratio: "
                               + (float) numberOfParallelVerificationThreads / verificatorRejects.size());
            verificatorRejects.stream().forEach(p -> p.printInfo());
        }
        return verificatorRejects;
    }

[hzalaz]

@derandreasberger I see no sign of a threading issue in the decoding code and the exception you got can be caused by a corrupted input that ends up with a malformed token passed to java-jwt. I said this since we only receive String not Streams so it's unlikely we are adding or modifying the string in some way to add these unexpected chars. We'll review this once we tackle the original issue reported here.

@arlampin its not the recommended way but I don't want to have a static singleton with the object mapper or keep it in a static variable in some already existing class. However, there are room for improvements like passing a single mapper to all who need to use them or use the recommended reader and writers (I am more inclined to do this) created from a single mapper that is stored in the verifier/creator since they are meant to be shared.
In order to make this clear, I'll close this one and create a new one stating this improvement