-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Caching Responses of JWKS #384
Comments
How would you see this implemented, in an ideal world? Something like this? use Some\Psr\CacheItemPool;
use Firebase\JWT\JWK;
$cache = CacheItemPool();
$keys = JWK::parseKeySetFromUrl($jwkUrl, $cache); |
looking at the way this is implemented in aut0/node-jkws-rsa and considering the following scenarios would not be possible.
They are creating a client first that can then be used to fetch keys. The problem is that currently the Calling JWT:decode requires having a keyset already parsed / fetched via JWKS. use Psr\Http\Client;
use Psr\SimpleCache;
$jwksUri = 'https://appleid.apple.com/auth/keys';
$jwksClient = new JWKSClient([
$jwksUri,
new Client(),
new SimpleCache(),
[//additional options?]
]);
JWT::decode($jwt, $jwksCient);
|
I see. In that case, we could potentially do something like this: class JWK
{
// new method. for pulling keys from URL
// Provide a cache object to cache the results
public static function parseKeySetFromUrl(
string $jwkUri,
Psr\Http\Client\ClientInterface $http,
Psr\Cache\CacheItemPoolInterface $cache = null,
int|\DateInterval|null $expiresAfter = null
): array {
if ($cache) {
$item = $cache->getItem($jwkUri);
if ($item->isHit()) {
// item found! Return it
return $item->get();
}
}
// fetch the keys and save them to the cache
$jwks = $this->http->get($jwkUri);
$keySet = static::parseKeySet($jwks);
if ($cache) {
$item->set($keySet);
$item->expiresAfter($expiresAfter);
$cache->save($item);
}
return $keySet;
}
// ...
} And then this would be used in the code like so: use Firebase\JWT\JWT;
use Firebase\JWT\JWK;
$cache = new Some\Psr\CacheItemPool;
$http = new Some\Psr\HttpClient;
$jwkUri = 'https://appleid.apple.com/auth/keys';
$keySet = JWK::parseKeySetFromUri($jwkUri, $http, $cache, 300 /* 5 minutes expiration */);
$decoded = JWT::decode($key, $keySet); What do you think? Edit: To handle the case for DDOS attack, I am curious how this is handled in other libraries. We could have a "DDOS cache key" and we could throw an exception if it's been hit. I'm not sure the best way to implement that, so your suggestions here would be great. |
if JWK::parseKeySetFromUri is returning a (cached) keyset without knowning the kid questioned for JWT::decode it is not possible to auto invalidate the cache, is it? for the possibility of ddos such an auto invalidation could cause - auth0/node-jwks-rsa is doing it via a nodejs package called limiter which is making use of an async wait function: https://github.com/jhurliman/node-rate-limiter/blob/main/src/RateLimiter.ts#L75 i would have said having a cache key like |
@swiffer in my example the cache has no ability to auto-invalidate, as the JWT method only sees an array of keys. It's interesting though, I would expect a JWK provider would provide a little buffer time before signing JWTs with new keys, as a best practice. Another option would be something like this: class CachedKeySet implements ArrayAccess
{
public function __construct($jwkUri, $http, $cache, $expiresIn)
{
// fetches keys from cache or uri
$this->fetchFromCacheOrReload();
}
public function offsetGet($keyId)
{
if (!isset($this->keySet[$keyId])) {
// forces cache refresh
$this->reload();
}
return $this->keySet[$keyId];
}
//...
} Then we'd open up the Honestly I'm not super convinced that extra feature is worth the complexity, but I could be wrong. |
Sounds reasonable - it might only happen immediately when the private key gets compromised. Having auto-invalidation would also allow for way longer cache times. Regards complexity - this is why I thought about implementing it here directly and not in the projects dependent on php-jwt. A simpler read through caching is implemented fairly easy in most modern php frameworks. Really like your suggestion of having a |
@swiffer coming back to this two weeks later, I like the idea also. I'll see what I can do! |
@swiffer I'm looking to merge this soon, but I'm hoping to have you leave a review before I do! |
Hi @bshaffer - first sorry for late response and thanks a lot for initial implementation. Coming from CakePHP and trying to use the new functionality there isn't that easy. There are to PSR for Caching (6 + 16). CakePHP only bundles a SimpleCache (PSR 16) compatible cache. Any reason why you've choosen one over the other? Symfony provides a class of converting caches back and forth via Cache adapter classes so this could be a viable solution: https://symfony.com/doc/current/components/cache/psr6_psr16_adapters.html |
This has been merged into |
Hi @bshaffer I just found this project today and I would love to use the new cashed key functionality. Do you have an eta on that next release? |
@sublime392 thanks for the nudge! I'll try to get this out this week. In the meantime you can install and use the feature by requiring "dev-main" as the version in composer. |
Thanks @bshaffer, looks like I will be waiting because it turns out that Laravel/passport already requires this project with version ^6.0. |
Working with JWKS can have a serious impact on performance if the signing key is fetched on every request.
While fetching the Keys currently is left to the app specific implementation shouldn't fetching and caching the response from an external resource be something firebase/php-jwt should do?
Auth0 suggests to implement caching and .Net 6 seems to do it as well:
https://community.auth0.com/t/caching-jwks-signing-key/17654/2
https://github.com/auth0/node-jwks-rsa#caching
Taking all considerations into account and implement it here eliminates doing it over and over again.
Maybe (optionally) using https://packagist.org/packages/psr/simple-cache as a simple cache interface would be the way to go?
The text was updated successfully, but these errors were encountered: