Skip to content
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

Closed
swiffer opened this issue Dec 26, 2021 · 14 comments
Closed

Caching Responses of JWKS #384

swiffer opened this issue Dec 26, 2021 · 14 comments

Comments

@swiffer
Copy link

swiffer commented Dec 26, 2021

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
Copy link
Collaborator

bshaffer commented Jan 5, 2022

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
Copy link
Author

swiffer commented Jan 22, 2022

looking at the way this is implemented in aut0/node-jkws-rsa and considering the following scenarios would not be possible.

  • Try to fetch new keys if you can’t find a kid referenced in a token in the cached JWKS
  • Put some protection to avoid trying to refresh the JWKS resource constantly (someone could try a DDOS attack by sending a token with a non-existing kid). E.g. “Wait at least 5 minutes before trying to get a new JWKS”.

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
Copy link
Collaborator

bshaffer commented Jan 24, 2022

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.

@bshaffer bshaffer added the v6.1 label Jan 24, 2022
@swiffer
Copy link
Author

swiffer commented Jan 24, 2022

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
Copy link
Collaborator

bshaffer commented Jan 25, 2022

@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
Copy link
Author

swiffer commented Jan 25, 2022

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
Copy link
Collaborator

bshaffer commented Feb 7, 2022

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

@bshaffer
Copy link
Collaborator

bshaffer commented Feb 9, 2022

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

@bshaffer
Copy link
Collaborator

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

@swiffer
Copy link
Author

swiffer commented Feb 17, 2022

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
Copy link
Collaborator

bshaffer commented May 2, 2022

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

@bshaffer bshaffer closed this as completed May 2, 2022
@sublime392
Copy link

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
Copy link
Collaborator

@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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants