feat: JWT cache implementation based on sieve algorithm - #4084
Conversation
|
This PR contains:
|
|
Very interesting! 👀 👀
Maybe we could expose those as metrics. That would help with passing code coverage too, since it looks like it's detecting those functions as dead code:
I also see the JWT loadtest failing on CI https://github.com/PostgREST/postgrest/actions/runs/15030111955?pr=4084:
Not sure what's going on there, but you should be able to reproduce it locally with |
|
Not sure what the problem is - tried to find typos in my changes to env variables settings, main (0b3c8c9) has the same issue on my machine. |
Weird, it's like you're using and old version of the nix tools and it's ignoring the $ postgrest-loadtest -k jwt
Created 50000 targets in ./test/load/gen_targets.http (0.79s)
...
## this is the ouput you're getting
$ postgrest-loadtest -k mixed # same as just "postgrest-loadtest"
delaying data to/from postgres by 0ms Maybe try going out of |
|
Yeah, that was it, thanks. I have serious doubts about Nevertheless there indeed seems to be an issue with implementation in this PR: all percentiles (ie. median up to 99th) show better results than |
Agree, we can change that.
Note that the jwt loadtest actually generates unique JWTs , it was mainly done to test the jwt decoding perf while also ensuring the JWT cache purging doesn't slow things down (#4034). The "mixed" loadtest does have hardcoded JWTs, and the perf looks more or less maintained (or slightly better). |
|
@steve-chavez @wolfgangwalther @taimoorzaeem I am not sure what to do with outstanding test coverage issues. This is about The question is: how to handle invalid JWTs? There are several options:
I've decided option three makes sense as it also speeds up repeated invalid token requests. OTOH - it opens up possibility of cache filling attacks using randomly generated tokens. It is disputable if that is a bigger problem than overloading CPU with eg. JWTs signed with random key. It looks to me option 4 would be the best compromise because it would prevent filling up cache with random garbage but would offload signature validation. I haven't implemented it as I don't know how to split parsing and signature validation with JOSE. WDYT? |
Parsing a JWT is really simple. Split on The parser that is used for But do we really need to? Parsing a JWT is really simple - but creating parseable tokens, with invalid signatures is just as easy. So I don't really see the additional value of option 4 vs option 3. If somebody wants to fill the cache, they can do so easily. |
Done. Three new counters are provided:
|
Indeed - any random bytes are a signature that consumes CPU to validate. So that leaves us with the choice between caching negative results or caching only valid JWTs. I've made cache implementation polymorphic over value computation monad so it is easy to change strategies. The latest commits introduce possibility of selecting one of two variants in PostgREST.Auth.JwtCache - it is a matter of changing |
I wonder whether we can cache all valid JWTs, but including expired ones? Aka when validation fails because of expiry, still cache. When validation fails because of something else, don't. The assumption is, that:
|
That’s exactly option 2. Which is currently implemented as notCachingErrors variant (switchable in JwtCache.init). As described in the description of the PR (first comment) - we don’t cache AuthResults but raw parsed claims that are re-validated for each request. |
Well, not exactly, but close enough I agree. We'd still cache those that fail, for example, and audience check or so. But that's totally fine, yes. |
Right - it is not exactly the same. The reason I decided to leave claims checking until after cache lookup are two-fold:
And, of course, claims checking is very fast, so there is little sense in caching it. |
Yes, this makese a lot of sense! So the only change that requires a reset is the change of secret, right? |
That, and - of course - turning off caching alltogether (ie. setting |
|
@steve-chavez @wolfgangwalther @taimoorzaeem After some more work on this PR I think it is now in a mergeable state (pending documentation changes, changelog adjustments etc. - and of course code review). I am pretty confident it is working fine as I've added JWT cache behavior tests that verify hits/misses and evictions using metrics. Please, let me know if there are any adjustments / changes required (or if you think the whole idea is wrong). |
| let auth = genToken [json|{"exp": 9999999999, "role": "postgrest_test_author", "id": "jdoe1"}|] | ||
|
|
||
| expectCounters | ||
| [ | ||
| requests (+ 1) | ||
| , hits (+ 0) | ||
| ] $ |
There was a problem hiding this comment.
This is a really elegant way to test the metrics 💯 So easy to read!
| |## Enables JWT Cache and sets its max size, disables caching with 0 | ||
| |# jwt-cache-max-size = 0 |
There was a problem hiding this comment.
I think we should make the default 1000. That means the cache will be enabled by default for next major.
There was a problem hiding this comment.
1000 is a rough estimation, mentioned before on #3802 (comment)
There was a problem hiding this comment.
I think we should make the default 1000. That means the cache will be enabled by default for next major.
@mkleczek Awesome! How about a gauge for number of cached JWTs? I'm currently load testing the feature and that would help me ensure the cache size is maxed and that it goes down. This might be good for tests too? I noticed that the previous JWT purge had a memory usage problem (#3889 (comment)) and this is now gone 🚀 I've recorded a video using postgrest-benchmark's OPTIONSUniqueJWT.js (this has the same logic as our Screencast.from.05-27-2025.09.56.23.PM.webmSharing the run results here for completeness: Also the metrics after the run (counters are high because I did some previous runs): @mkleczek I've noticed that the jwt loadtest results show a perf drop compared to main and latest version. Is that expected? |
Our load test is the worst possible case for this (and I would say: any bounded) cache: all JWTs are different so no caching but:
In other words - cache thrashing at its best :) What's more: in case of symmetric JWT keys I don't think cache lookup is faster than simply performing JWT verification. |
c282e74 to
43f6d76
Compare
43f6d76 to
ac155a9
Compare
Changes: 1. Refactoring and some cleanup of JWT handling code: * Instead of caching AuthResult cache decoded claims (which signature was verified). Validating claims and determining role is done after cache lookup * Cleaned up API so that usage of it is simplified: lookupJwtCache cache key >>= parseClaims configJwtAud time * Handling of JwtCacheState initialization and updates of configuration is encapsulated in Auth.JwtCache module 2. Generic high performance (hopefully) scalable, dynamically resizeable cache implementation based on stm, stm-hamt and sieve algorithm. It also integrates with PostgREST measurements infrastructure providing usage stats (ie. hit ratio, evictions count)
docs: reorganize sections to better explain caching
|
🙌 We're now included in the "Adoption" section of https://cachemon.github.io/SIEVE-website/ 🙌 |
This should reduce setup time for build process. - cache: introduced in PostgREST#2928, defunct since PostgREST#4084 - clock: introduced in PostgREST#2928, defunct since PostgREST#4084 - heredoc: introduced in PostgREST#714, defunct since PostgREST#4390 - iproute: introduced in PostgREST#3560, defunct since PostgREST#4288 Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
This should reduce setup time for build process. - cache: introduced in #2928, defunct since #4084 - clock: introduced in #2928, defunct since #4084 - heredoc: introduced in #714, defunct since #4390 - iproute: introduced in #3560, defunct since #4288 Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
This should reduce setup time for build process. - cache: introduced in #2928, defunct since #4084 - clock: introduced in #2928, defunct since #4084 - heredoc: introduced in #714, defunct since #4390 - iproute: introduced in #3560, defunct since #4288 Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com> (cherry picked from commit 9921743)
This should reduce setup time for build process. - cache: introduced in #2928, defunct since #4084 - clock: introduced in #2928, defunct since #4084 - heredoc: introduced in #714, defunct since #4390 - iproute: introduced in #3560, defunct since #4288 Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com> (cherry picked from commit 9921743)
Removes a test related to jwt cache which is stale since PostgREST#4084. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
Removes a test related to jwt cache which is stale since #4084. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
`PGRST_JWT_CACHE_MAX_LIFETIME` is defunct since PostgREST#4084 is merged. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
`PGRST_JWT_CACHE_MAX_LIFETIME` is defunct since PostgREST#4084 is merged. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
`PGRST_JWT_CACHE_MAX_LIFETIME` is defunct since #4084 is merged. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
This should reduce setup time for build process. - cache: introduced in PostgREST#2928, defunct since PostgREST#4084 - clock: introduced in PostgREST#2928, defunct since PostgREST#4084 - heredoc: introduced in PostgREST#714, defunct since PostgREST#4390 - iproute: introduced in PostgREST#3560, defunct since PostgREST#4288 Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
Removes a test related to jwt cache which is stale since PostgREST#4084. Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
Draft version of JWT cache implementation based on https://cachemon.github.io/SIEVE-website/blog/2023/12/17/sieve-is-simpler-than-lru/ algorithm