Skip to content

Cache loading of JWK object from OIDC private key #1273

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

Conversation

cakemanny
Copy link
Contributor

@cakemanny cakemanny commented May 18, 2023

Fixes #1263

Description of the Change

This adds caching so that the OIDC private key is only loaded once.

Testing via the repro instructions in the referenced issue, on my machine the request time for the code-for-token exchange reduces from 1.5s to 150ms from the second request onwards (the first request take 820ms due to needing to load from the PEM the first time).

I'm not super happy with adding a utils module but I was unable to determine a good place to put the cached function.

The implementation uses functools.lru_cache - which seems like a good option with the limitation of being restricted to what is available in python 3.7.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

Since there is no functional change, I feel the existing unit tests already cover the changes sufficiently but I'm happy to hear suggestions. now tested

@cakemanny cakemanny marked this pull request as ready for review May 18, 2023 10:30
dopry
dopry previously requested changes May 20, 2023
Copy link
Member

@dopry dopry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cakemanny thanks for contributing this PR. This is a valuable performance feature. Please add a test that confirms that jwk_from_pem is caching. It'll save us from a mistake where someone accidentally removes the @functools.lru_cache() decorator.

@codecov
Copy link

codecov bot commented May 20, 2023

Codecov Report

Merging #1273 (c31bd1e) into master (13a6143) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1273   +/-   ##
=======================================
  Coverage   97.29%   97.30%           
=======================================
  Files          31       32    +1     
  Lines        1996     2003    +7     
=======================================
+ Hits         1942     1949    +7     
  Misses         54       54           
Impacted Files Coverage Δ
oauth2_provider/models.py 98.55% <100.00%> (+<0.01%) ⬆️
oauth2_provider/utils.py 100.00% <100.00%> (ø)
oauth2_provider/views/oidc.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cakemanny cakemanny force-pushed the fix-repeat-private-key-parsing-via-caching branch from 8be7daf to 5dc58b5 Compare May 21, 2023 10:38
@cakemanny
Copy link
Contributor Author

Good suggestion. Added in 5dc58b5
I thought about whether to base the test on accessing the private key setting, i.e. below, but after looking at it, I decided that it seemed strange that the test depend on the settings module despite the implementation in utils being agnostic of the project so far.

@pytest.mark.oauth2_settings(presets.OIDC_SETTINGS_RW)
def test_jwk_from_pem_caches_jwk(oauth2_settings):

    # For the same private key we expect the same object to be returned

    jwk1 = utils.jwk_from_pem(oauth2_settings.OIDC_RSA_PRIVATE_KEY)
    jwk2 = utils.jwk_from_pem(oauth2_settings.OIDC_RSA_PRIVATE_KEY)

    assert jwk1 is jwk2

    # But for a different key, a different object
    jwk3 = utils.jwk_from_pem(oauth2_settings.OIDC_RSA_PRIVATE_KEYS_INACTIVE[0])

    assert jwk3 is not jwk1

So I decided to change it to use its own little keys

@cakemanny cakemanny requested a review from dopry May 21, 2023 10:49
@cakemanny cakemanny force-pushed the fix-repeat-private-key-parsing-via-caching branch from 5dc58b5 to c31bd1e Compare June 2, 2023 10:01
@cakemanny
Copy link
Contributor Author

Hi @dopry, did you have any further feedback following the adding of the tests?

@cakemanny
Copy link
Contributor Author

I can imagine that dopry is on holiday or otherwise busy for a while, @tonial and @n2ygk , I saw both of you commented on the issue that this is addressing, would it be ok to ask one of you two for a review in his stead?

@tonial
Copy link
Contributor

tonial commented Jun 8, 2023

Hi @cakemanny .
It've read your PR and I think it's great, but I'm not part of DOT team (https://jazzband.co/projects/django-oauth-toolkit)

@n2ygk
Copy link
Contributor

n2ygk commented Jun 8, 2023

Hi @cakemanny . It've read your PR and I think it's great, but I'm not part of DOT team (https://jazzband.co/projects/django-oauth-toolkit)

@tonial It would be great to have you join!

@n2ygk
Copy link
Contributor

n2ygk commented Jun 8, 2023

@cakemanny Did you add this test? It's still listed as an open review item. It looks like you did. I'll work on approving this:

@cakemanny thanks for contributing this PR. This is a valuable performance feature. Please add a test that confirms that jwk_from_pem is caching. It'll save us from a mistake where someone accidentally removes the @functools.lru_cache() decorator.

@n2ygk n2ygk dismissed dopry’s stale review June 8, 2023 14:18

The requested change was made.

@n2ygk n2ygk merged commit 2f3dd45 into django-oauth:master Jun 8, 2023
@n2ygk n2ygk added this to the Future milestone Jun 8, 2023
@cakemanny cakemanny deleted the fix-repeat-private-key-parsing-via-caching branch February 5, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jwk_key is reparsed 3x per login (and it's expensive)
4 participants