RemoteJWKSet should proactively refresh its cache

[Josh Cummings]

RemoteJWKSet refreshes its cache whenever it can't find the indicated key in its cache, which I think handles most cases.

However, it doesn't handle the case when an OP expires a key only (meaning that there are no new keys to trigger the expiration). In this case, a JWT could be specified and accepted by Nimbus, even though the indicated key has been expired.

I think that the cachedJwkSet ought to expire after some kind of time since creation, perhaps configurably.

[desert wang]

thanks @jzheaux for your follow up.

i have seen key rotation description in Microsoft web site and have met a Google Id token which generated about a month ago is verified failure because of key missing.

Key rotation should be really happens in Google cloud.

[Josh Cummings]

@desert3 I think the issue is that if Microsoft or any key provider decides to remove a key (say they found that it was compromised) from the list, but does not, in that circumstance, replace the key with another, then there won't be any JWTs with new keys to trigger the cache invalidation.

For example, say that Azure has the following keys in its set:

{ 'kid-1', 'kid-2', 'kid-3' }

And a resource server has cached these in its RemoteJWKSet.cachedJwkSet reference.

The only JWTs that will come through will be those with one of the three kids listed.

Now, if Azure finds that one of the three keys needs to be decommissioned, the list would now look like this:

{ 'kid-1', 'kid-3' }

And since there is no new key, then all incoming JWTs will continue to have known keys, so the cache won't be cleared.

The result is that if a JWT comes in with 'kid-2', then, since it is still cached, that JWT will be accepted, even though Azure has decided that 'kid-2' is no longer valid.

[Josh Cummings]

@vdzhuvinov what do you think about this feature?

A first step could be to separate the caching from the retrieval. For example, RemoteJWKSet could be refactored to extend from a class that always retrieves the JWK Set. Then, folks could wrap their own caching mechanism around that.

[Vladimir Dzhuvinov]

@jzheaux I find the refresh absolutely necessary, given everything that was said.

I was thinking of adding a field to RemoteJWKSet to expire the cache after some given time, in ms, but your suggestion seems better, as it would allow people to use a cache like JCache.

Would you like to submit a PR?

[Josh Cummings]

Sure, @vdzhuvinov, will do.

[Vladimir Dzhuvinov]

Josh, how about allowing the RemoteJWKSet to be configured with a cache interface, so people can plug their own caching without having to extend the class?

The existing constructor could provide a default caching implementation that has the current behaviour, with the added ability to proactively refresh.

The API won't break that way.

[Vladimir Dzhuvinov]

Added a JWKSetCache interface, the default impl expires the JWK set in 5 minutes. Take a look: 6550608

If you think this is going to work well will release it as v6.3.

[Vladimir Dzhuvinov]

Merged and about to release as v6.3.

If there are anything needs to be added or updated, feel to reopen :)