com.auth0.jwk.Jwk#getPublicKey recreates PublicKey object on every access

[adrian-skybaker]

Even when using a cached JwkProvider, the com.auth0.jwk.Jwk#getPublicKey method will reconstruct a public key object in memory on every call.

Although the caching prevents remote calls, at high volumes (eg thousands requests/sec, each requiring JWT verification) this amount of object allocation creates a measureable overhead.

[jimmyjames]

Hey @adrian-skybaker, thanks for raising. We'll look into this and see if there's something we can do to minimize the overhead.

[jimmyjames]

@adrian-skybaker you are correct that while the Jwk itself can be cached, the public key is computed on every invocation. I'm curious (and for others potentially wondering the same), are you working around this by caching the public key, and if so how?

We can't move the calculation of the key to the constructor and then just return the computed value because of the potential exception handling. Perhaps we could implement a simple in-memory lazy loading by computing the key the first time and then storing the value:

public class Jwk {
    ...
    // added field
    private PublicKey publicKey;

    ...

    public synchronized PublicKey getPublicKey() throws InvalidPublicKeyException {
        if (Objects.isNull(this.publicKey)) {
            this.publicKey = computeKey();
        }
        return this.publicKey;
    }

    private synchronized PublicKey computeKey() throws InvalidPublicKeyException {
        // calculate and return public key
    }
}

Need to think about it a bit more but interested in others thoughts.

[jimmyjames]

We're not going to implement any caching of the the public key at this time, as the introduction of caching could bring about complexities through synchronization/eviction/rotation, etc. Other Auth0 libraries also do not cache the public key. We will monitor this issue for additional interest, but in the meantime it's recommended that clients that demonstrate high usage load here cache the key locally.

[adrian-skybaker]

Perhaps the issue is misunderstood. The issue is that the PublicKey object is reconstructed from existing in-memory data every call to the same com.auth0.jwk.Jwk instance. There is no need for eviction/rotation, just memoization of the constructed object.

[adrian-skybaker]

Perhaps we could implement a simple in-memory lazy loading by computing the key the first time and then storing the value:

This solution looks fine to me.

but in the meantime it's recommended that clients that demonstrate high usage load here cache the key locally.

That's possible, but quite awkward if you want to still leverage the built-in caching and refresh logic etc.

Here's our solution:

/** Workaround https://github.com/auth0/jwks-rsa-java/issues/150, avoid recreating the PublicKey object every request */
static class FastJwk extends Jwk {
    private PublicKey publicKey;
    @Builder
    public FastJwk(
            String id,
            String type,
            String algorithm,
            String usage,
            List<String> operations,
            String certificateUrl,
           List<String> certificateChain,
            String certificateThumbprint,
            Map<String, Object> additionalAttributes) {
        super(id, type, algorithm, usage, operations, certificateUrl, certificateChain, certificateThumbprint, additionalAttributes);
    }

    @Override
    public PublicKey getPublicKey() throws InvalidPublicKeyException {
        // We don't care about double checked locking, its harmless if we create twice in race conditions
        if(this.publicKey == null) {
            this.publicKey = super.getPublicKey();
        }
        return this.publicKey;
    }
}

@RequiredArgsConstructor
/** Provides {@link FastJwk} instances */
static class FastJwtProvider implements JwkProvider {

    private final UrlJwkProvider delegate;

    @Override
    public Jwk get(String keyId) throws JwkException {
        Jwk jwt = delegate.get(keyId);
        return FastJwk.builder()
                .id(jwt.getId())
                .type(jwt.getType())
                .algorithm(jwt.getAlgorithm())
                .usage(jwt.getUsage())
                .operations(jwt.getOperationsAsList())
                .certificateUrl(jwt.getCertificateUrl())
                .certificateChain(jwt.getCertificateChain())
                .certificateThumbprint(jwt.getCertificateThumbprint())
                .additionalAttributes(jwt.getAdditionalAttributes())
                .build();
    }
}

UrlJwkProvider urlJwkProvider = new UrlJwkProvider("...domain...");
final JwkProvider jwtProvider = new FastJwtProvider(urlJwkProvider);

[adrian-skybaker]

Tagging @jimmyjames in case these comments aren't visible. I believe if the rationale for closing this was to avoid "synchronization/eviction/rotation", then the change is misunderstood, none of these aspects are required.

[jimmyjames]

👋 hey @adrian-skybaker, thanks for all the info and your implementation notes. We will add this feature to our backlog; that said, if you (or anyone else) would like to provide a PR before we get to it (I think the snippet I provided above will work), I'm happy to review!