firebase / php-jwt

PHP package for JWT
BSD 3-Clause "New" or "Revised" License
9.37k stars 1.27k forks source link

Caching Responses of JWKS #384

Closed swiffer closed 2 years ago

swiffer commented 2 years ago

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?

bshaffer commented 2 years ago

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);
swiffer commented 2 years ago

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. fetching keys required handing over the kid (to see if the specific kid is already cached).

The problem is that currently the kid used in the JWT is only extracted as part of JWT::decode.

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);

JWT::decode() would need to call $jwksClient->getKeyMaterial($kid) in case a jwksClient is passed.

bshaffer commented 2 years ago

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.

swiffer commented 2 years ago

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 md5($jwksUri) . '_rate_limit' set to the time last fetched would be fine, but definitly not an expert regards best practices here!

bshaffer commented 2 years ago

@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 decode function to accept this class also.

Honestly I'm not super convinced that extra feature is worth the complexity, but I could be wrong.

swiffer commented 2 years ago

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.

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 CachedKeySet.

bshaffer commented 2 years ago

@swiffer coming back to this two weeks later, I like the idea also. I'll see what I can do!

bshaffer commented 2 years ago

@swiffer Please see #397 and tell me what you think!

bshaffer commented 2 years ago

@swiffer I'm looking to merge this soon, but I'm hoping to have you leave a review before I do!

swiffer commented 2 years ago

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

bshaffer commented 2 years ago

This has been merged into main and will be in the next release

sublime392 commented 2 years ago

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?

bshaffer commented 2 years ago

@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.

sublime392 commented 2 years ago

Thanks @bshaffer, looks like I will be waiting because it turns out that Laravel/passport already requires this project with version ^6.0.