feat: Cache OAuth2 introspection client #482
Conversation
ptescher
changed the title
There was a problem hiding this comment.
I think this is a great idea! Regarding implementation I have some ideas how to improve the correctness of the code. It is also imperative to add tests which, at least:
- Check if the cache is used in general
- Check that there are no collisions (e.g. the same cache used for two different configs)
- Check that the cache is invalidated when requests fail
To check if the cache is used, the best idea would be to mock a pre-auth endpoint and count how often that endpoint was called while doing things against the cache.
Keep in mind that, if you use ristretto, this counter will be off sometimes because ristretto does not always cache all items which would make the test flaky. If you want to use ristretto, doing multiple requests (e.g. 10 or 20) and counting if the total number of actual HTTP requests is lower (e.g. 2 or 3) would be the way to approach this.
| Scopes: c.PreAuth.Scope, | ||
| TokenURL: c.PreAuth.TokenURL, | ||
| }).Client(context.Background()).Transport | ||
| rt = a.preAuthCache.Client(context.Background(), c.PreAuth).Transport |
There was a problem hiding this comment.
c.PreAuth is a parameter which may change per route, but the authenticator is instantiated only once with one global cache, which will cause errors when more than one PreAuth are configured. Instead, this should probably be cached using the c.PreAuth contents as the key (e.g. crc32ing a string representation of c.PreAuth) .
Another issue is that the cache is never invalidated, even if requests start failing. This is problematic when, for example, the configuration is changed and subsequently reloaded.
|
@aeneasr Is this feature resolved in any other way? For me it seems the pre_authorization is not usable without this in production. Thanks |
Related issue
#293
#424
Proposed changes
#424 Cached the responses of OAuth2 introspection, but for repeated calls to similar requests with pre_authorization enabled Oathkeeper will fetch a new access token per request. This can generate a massive number of access tokens!
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
Although the configuration for
authenticators. oauth2_introspection.config. pre_authorizationis not likely to change, the way the authenticators are built the configuration is not available on initialization. In order to get around this limitation this patch introduces a "cache" for the client. That cache uses a mutex which could be quite heavy for this use case. If there is a lighter solution that would work in this case it might be preferred.