Skip to content

fix: memory leak in id_token mutator cache#1209

Merged
aeneasr merged 1 commit intoory:masterfrom
David-Wobrock:fix/memory-leak-id-token-mutator-cache
Dec 28, 2024
Merged

fix: memory leak in id_token mutator cache#1209
aeneasr merged 1 commit intoory:masterfrom
David-Wobrock:fix/memory-leak-id-token-mutator-cache

Conversation

@David-Wobrock
Copy link
Contributor

@David-Wobrock David-Wobrock commented Dec 28, 2024

Set a TTL on the cached JWTs in the id_token mutator.
It fixes a memory leak in Oathkeeper 🙌

We are running internally a fork of Oathkeeper with this patch applied, and the resulting memory footprint:
image

Unsure how to properly test it properly in a unit test though, since the getFromCache logic also checks if the cached TTL value is not expired (and not only the new ristretto internal TTL).

Related issue(s)

Split from #1177.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@David-Wobrock David-Wobrock force-pushed the fix/memory-leak-id-token-mutator-cache branch from 1c7096d to ed72272 Compare December 28, 2024 09:01
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! This makes sense - one remark though. If you see high memory use the cache is probably misconfigured. Ristretto does evict items when the cache size limit has been reached so theoretically you don’t need TTL (but it makes sense because otherwise the cache might evict tokens that are still valid)

@aeneasr aeneasr merged commit 363ac04 into ory:master Dec 28, 2024
@David-Wobrock David-Wobrock deleted the fix/memory-leak-id-token-mutator-cache branch December 28, 2024 10:57
@David-Wobrock
Copy link
Contributor Author

Thank you! This makes sense - one remark though. If you see high memory use the cache is probably misconfigured. Ristretto does evict items when the cache size limit has been reached so theoretically you don’t need TTL (but it makes sense because otherwise the cache might evict tokens that are still valid)

Agreed 👍
There's an attempt to tackle this here: #1177
Which changes the default cache configuration, but also makes the id_token cache configurable.

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.

2 participants