Cache signing keys

[stevenpitts]

The function get_signing_key indirectly makes a network call to get the public key. For a project that authenticates requests extremely often, this introduces a lot of delay.

Cache the result of get_signing_key so that it gets reused if called with the same client and the same kid.

This should not break any projects that are following JWT standards, as while the RFC is not entirely explicit, common practice dictates that the kid must be unique:

kid: is the unique identifier for the key

Therefore, unless there is somehow a new key added and an old key removed which both somehow have the same kid (which I think is a fair assumption), this is not a breaking change.

I initially wanted to use functools.lru_cache, but there are known issues with using functools.lru_cache with instance methods.

The _known_signing_key key values include the URI, so as not to break any existing workflows that change an instance of PyJWKClient's uri value (for whatever reason). I'm honestly fine with getting rid of the URI part so that it's just kid -> key, but I figure you guys should have the say on that decision 🤷

I've also added a test to check that the caching is working, and a test to make sure that we're not overwriting keys from different URIs. During testing, I noticed that mocked_response only allows urllib.request.urlopen to be called once, so technically test_get_signing_key_caches_result will fail with an exception instead of network_response.call_count == 1, but it still fails if the caching isn't working.

[jpadilla]

@makusu2 thanks for working on this one.

I initially wanted to use functools.lru_cache, but there are known issues with using functools.lru_cache with instance methods.

I haven't looked too much at it, but I wonder if we could just do something like what's proposed here: https://stackoverflow.com/questions/14946264/python-lru-cache-decorator-per-instance/14946506#14946506

The _known_signing_key key values include the URI, so as not to break any existing workflows that change an instance of PyJWKClient's uri value (for whatever reason).

I'm fine only keeping kid in there.

Regarding caching here, ideally we could set some sensible defaults for number of cached items and max age. A must is way to opt-out of caching behavior. Maybe a mixin for PyJWKClient or CachedPyJWKClient?

[stevenpitts]

I haven't looked too much at it, but I wonder if we could just do something like what's proposed here: https://stackoverflow.com/questions/14946264/python-lru-cache-decorator-per-instance/14946506#14946506

It looks like it works! Good catch 👍

The _known_signing_key key values include the URI, so as not to break any existing workflows that change an instance of PyJWKClient's uri value (for whatever reason).

I'm fine only keeping kid in there.

Done

Regarding caching here, ideally we could set some sensible defaults for number of cached items and max age.

16 feels reasonable. The're just strings, so it's low enough that it makes no discernible impact on memory usage.

A must is way to opt-out of caching behavior. Maybe a mixin for PyJWKClient or CachedPyJWKClient?

Sure, but a constructor input makes more sense to me for this, would you be opposed to that?

Also, would you prefer opt-out or opt-in to caching? Opt-out makes more sense to me, since it really shouldn't be intrusive.

You're the boss, let me know what you think with the new changes.

[jpadilla]

@makusu2 I think we should also make maxsize configurable here.

[stevenpitts]

Good call, done 👍

[jpadilla]

@makusu2 thanks! mind adding an entry to the changelog?

[stevenpitts]

@makusu2 thanks! mind adding an entry to the changelog?

Added!

(I assume my name will get tagged at the next major release, under the Pull Requests section? Not sure how that works)

[stevenpitts]

A check failed and I can't see why... interesting that it started failing now 🤔

[stevenpitts]

@jpadilla Could you please check to see if that's a legit failure?

[luigibertaco]

Hey @jpadilla any idea when a release containing this change will be available? I am currently developing a custom solution to do exactly the same but I'd prefer to use it from here.

Thanks

[dajiaji]

I think this change is a little bit dangerous because the cache implementation does not have TTL. If a JWK's private key is compromised, I think this implementation will allow the JWK to remain in the cache for a long time unintentionally.

It seems to me that we need to either support TTL or revert this change.

[stevenpitts]

It seems to me that we need to either support TTL or revert this change.

I agree that we should handle TTL, but caching can be disabled, so I don't think we need to revert this change

[dajiaji]

caching can be disabled, so I don't think we need to revert this change

OK, I agree. What I wanted to say is that it is better to support TTL as soon as possible.

[stevenpitts]

OK, I agree. What I wanted to say is that it is better to support TTL as soon as possible.

I gotcha.
Actually, the more I think about it, we might want to make caching disabled by default... what do you think?

[dajiaji]

Hmm, indeed, it might be better to disable it by default.

[luigibertaco]

Another improvement could be caching the get_jwk_set instead of the single key used. It would avoid unnecessary fetches even if the key was not used yet.