feat: Cache requests for JWKS on JWT verification #228
Conversation
|
Please note that I'm planning to look into ways of inserting a "clock" for replacing The main concern is to figure out an API for a clock that does not depend on external libraries. I have some ideas, but I consider them out of scope for this PR. |
| // Caching store. | ||
| type cache struct { | ||
| mu sync.RWMutex | ||
| entries map[string]*cacheEntry |
There was a problem hiding this comment.
🙃 Without specific metrics in mind, shouldn't we consider offering a way to prune expired entries? Just in case this gets heavily used and starts to fill up the memory.
There was a problem hiding this comment.
Good idea. We can optimistically prune every time we force fetch. I can add this as an enhancement in a later PR.
There was a problem hiding this comment.
As far as I understand we currently store always one entry here, isn't that right?
|
|
||
| // Try the cache first. Make sure we have a non-expired entry and | ||
| // that the value is a valid JWKS. | ||
| entry, ok := getCache().Get(cacheKey) |
There was a problem hiding this comment.
🔧 I think the cache must have the KID (instance ID) as key (or part of it). You can then quickly lookup by KID since that's the requirement from line 114. This way you can also have a multi-instance scenario handled.
There was a problem hiding this comment.
Good thinking. I'm just a bit reluctant to change the existing behavior. 🤔
As far as I understand the cache stores the whole json web key set response from the server. It was like this before and I'm a bit hesitant to change it now.
Regarding multi-instance scenarios, indeed, caching per instance is not possible with this setup, but I don't think it's needed for now.
I'll toy a bit with your proposal and open a separate PR if it doesn't break anything. Assuming that's alright, since you added a 🔧 comment. 😄
| // Caching store. | ||
| type cache struct { | ||
| mu sync.RWMutex | ||
| entries map[string]*cacheEntry |
There was a problem hiding this comment.
As far as I understand we currently store always one entry here, isn't that right?
| func (c *cache) Get(key string) (*cacheEntry, bool) { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
| entry, ok := c.entries[key] |
There was a problem hiding this comment.
❓ If the entry is expired, I think you can ignore it here and don't return it?
There was a problem hiding this comment.
Was debating this as well. The entry is still there technically, but I guess users of a cache would expect to not get back expired entries?
There was a problem hiding this comment.
If it's going to be replaced, sounds good to me to not return it. My suggestion just transfers the concern to the cache layer. Not a big deal.
The jwt.Verify method needs to fetch the JSON Web Key Set from the API in order to verify the session JWT's validity. The jwt.Verify method is used in the http.WithHeaderAuthorization middleware, which means that in an HTTP server context, the method will executed for every request. We're adding a caching layer for the JWKS when we verify the session JWT. This way we can cache the JWKS response from the API for 1 hour.
gkats
force-pushed
the
core-1639-jwks-cache
branch
from
cb8971d to
4f362db
Compare
The jwt.Verify method needs to fetch the JSON Web Key Set from the API in order to verify the session JWT's validity.
The jwt.Verify method is used in the http.WithHeaderAuthorization middleware, which means that in an HTTP server context, the method will executed for every request.
We're adding a caching layer for the JWKS when we verify the session JWT. This way we can cache the JWKS response from the API for 1 hour.