-
-
Notifications
You must be signed in to change notification settings - Fork 676
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
Cache signing keys #611
Cache signing keys #611
Conversation
@makusu2 thanks for working on this one.
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
I'm fine only keeping 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 |
It looks like it works! Good catch 👍
Done
16 feels reasonable. The're just strings, so it's low enough that it makes no discernible impact on memory usage.
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. |
jwt/jwks_client.py
Outdated
if cache_keys: | ||
# Cache signing keys | ||
# Ignore mypy (https://github.com/python/mypy/issues/2427) | ||
self.get_signing_key = lru_cache(maxsize=16)(self.get_signing_key) # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@makusu2 I think we should also make maxsize configurable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@makusu2 thanks! mind adding an entry to the changelog?
Added! (I assume my name will get tagged at the next major release, under the |
A check failed and I can't see why... interesting that it started failing now 🤔 |
@jpadilla Could you please check to see if that's a legit failure? |
Co-authored-by: José Padilla <jpadilla@webapplicate.com>
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 |
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. |
I agree that we should handle TTL, but 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. |
I gotcha. |
Hmm, indeed, it might be better to disable it by default. |
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. |
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 withthe same client andthe samekid
.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: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 usefunctools.lru_cache
, but there are known issues with usingfunctools.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 thatmocked_response
only allowsurllib.request.urlopen
to be called once, so technicallytest_get_signing_key_caches_result
will fail with an exception instead ofnetwork_response.call_count == 1
, but it still fails if the caching isn't working.