FISH-868 JWT public key is cached for a configurable amount of time.

[ghunteranderson]

Description

This is a PR for feature request #5027. With this change, the loaded JWT public key is cached for subsequent requests to use. While the PR is aimed at helping with remote public keys, it handles key caching from any source the same.

I original wrote the feature request with HTTP cache headers in mind but since it is much simpler to have a default cache TTL configurable by payara-mp-jwt.properties, I thought that would be a better place to start. A new configuration publicKey.cache.ttl sets the TTL in seconds and defaults to 5 minutes (probably want to discuss what the default could be). If set to 0 or less, the cache is disabled and public key loads are no longer synchronized.

Last, I omitted a feature I entertained in #5027 where receiving an unrecognized JWT key ID would hint to the server that the public key cache was stale and needed to be refreshed. If that's something we think would be helpful, I can definitely add it.

Testing

New tests

I held off on writing tests because I'm not sure how tests are being handled for this module. I do not see any unit test. For what it's worth, I did manually test the feature with embedded and remote public keys.

Notes for Reviewers

The PR looks a bit bigger than it actually is. With the extra complexity to loading public keys, it seemed like a good time to move that responsibility out of SignedJWTIdentityStore and into its own class, JwtPublicKeyStore. Most of the "read MP public key" methods were copied verbatim except they now return the public key as a string instead of parsing it first.

I went back and forth on whether to cache the public key before or after parsing. I landed on before because it was the least disruptive to how key IDs, PEMs and JWKS is working right now but I also recognize that each request then has to re-parse the public key. I thought it was an ok tradeoff but I could be easily swayed.

I really wanted to use org.glassfish.jersey.internal.guava.LoadingCache but it does not seem to support expireAfterWrite. I added a simple double checking synchronized cache wrapper but I'd rather delete it if there's a better option already available in the project.

[AlanRoth]

jenkins test please

[svendiedrichsen]

There is a org.glassfish.jersey.internal.guava.CacheBuilder which allows to create LocalCache instances and allows CacheLoader instances to be provided. It also supports a setting of expireAfterWriteNanos. Maybe this is something worth looking at for the actual cache implementation.
Nice work btw.

[ghunteranderson]

@svendiedrichsen Thank you! I thought I checked those cache classes for a "expire after write" option and didn't see one. I might have just missed it. I'll check again to make sure.

[svendiedrichsen]

As for the CacheBuilder from Jersey framework. It doesn't seem to be fully developed. I couldn't find expireAfterWriteNanos being used at all. Maybe it isn't a good idea to use Jersey internal classes after all.

[fturizo]

Changes needed, with clarification over the scope of HTTP cache headers consideration for the TTL of the public key.

[ghunteranderson]

jenkins test please

[lprimak]

jenkins test

[svendiedrichsen]

@ghunteranderson Wouldn't it be nice to have some unit tests?

[fturizo]

@ghunteranderson, before merging the PR, would you be kind enough to document this improvement and submit a PR to the official Payara Community Documentation repository: https://github.com/payara/Payara-Community-Documentation? Let us know if you have any questions

[fturizo]

@ghunteranderson, to help you merge this contribution, I've documented it on the Payara Community Edition repository here: payara/Payara-Community-Documentation#127. Once this PR is approved and merged, I'll proceed to merge and close this issue.

[ghunteranderson]

Thanks for the help on this @fturizo! Finding time has gotten a little more difficult. I really appreciate it 🙂