feat: Composite configurable mutator cache key per rule. #885
Conversation
Codecov Report
@@ Coverage Diff @@
## master #885 +/- ##
==========================================
+ Coverage 62.20% 62.69% +0.48%
==========================================
Files 102 102
Lines 4837 4892 +55
==========================================
+ Hits 3009 3067 +58
+ Misses 1554 1547 -7
- Partials 274 278 +4
|
| break | ||
| case len(cfg.Cache.Key) > 0: | ||
| // Build a composite cache key with property from configuration. | ||
| if cacheSession, ok := a.hydrateFromCache(a.cacheKey( |
There was a problem hiding this comment.
Key is a composite key based on configuration property + three distinct repeatable properties (URL for hydrator, rule id and subject of session).
There was a problem hiding this comment.
I am not sure if that is enough! What about extra properties of the session, such as e.g. "scope" or "permissions"? These could all influence the hydrator response and could lead to eventual security vulnerabilities down the road. I think we need to take the full session in a serialized form if we want to be sure that the cache can actually be reused!
There was a problem hiding this comment.
If using complete AuthenticationSession one is not benefiting given the volatiltity of this structure - using subject, rule id, hydrator URL as part of the composition is enabling flexibility while ensuring a certain level of constraint. The user is also free given flexibility to set a key which can include all of the above (i.e. JWT-claims) using templating.
There was a problem hiding this comment.
Thank you. I am unsure though wether I can agree here. I think it points to a deeper issue though which is manifesting in this pr: It is not possible to define the cache key reliably. Actually, one would probably like to define the cache key themselves (e.g. subject + scope) to make the system more efficient. For example, the AuthSession might contain vital info such as a "permissions" array. Yet, it may also contain a counter or timestamp which is changing for most of the requests - invalidating the cache if included.
We are still not there yet with a new concept for Ory Oathkeeper, but I think this is a very interesting problem that could very well warrant a realignment on Ory Oathkeeper which would be to make access control at the reverse proxy as efficient and flexible as possible.
I think this too could benefit from JsonNet, as JsonNet is typable, lintable, and can produce errors (go templating fulfills none of these properties).
Unfortunately, for this PR in particular, I don't think the current implementation can be accepted because it still bears too many risks and this particular type of issue has already caused a CVE in Ory Oathkeeper which is why I am so hyper-sensible about this topic: GHSA-qvp4-rpmr-xwrr
There was a problem hiding this comment.
Thank you. Let us inverse the solution. If you feel a user can't define a cache key freely (given cause of concerns as mentioned) - what about allowing the user to define exclusion of request/response headers (I also believe I saw an issue regarding this)? If the whole AuthenticationSession is used as a cache key headers represents a volatile part (i.e. subject to middleware enrichment which is hard to predict and control). extra is defined as part of previous steps in the pipeline and can be more controlled. What is your thoughts?
There was a problem hiding this comment.
That's a brilliant idea! Exclusion is much safer (because explicit) :)
Thank you, please have another review. This includes some refactoring, introduction of |
| break | ||
| case len(cfg.Cache.Key) > 0: | ||
| // Build a composite cache key with property from configuration. | ||
| if cacheSession, ok := a.hydrateFromCache(a.cacheKey( |
There was a problem hiding this comment.
Thank you. I am unsure though wether I can agree here. I think it points to a deeper issue though which is manifesting in this pr: It is not possible to define the cache key reliably. Actually, one would probably like to define the cache key themselves (e.g. subject + scope) to make the system more efficient. For example, the AuthSession might contain vital info such as a "permissions" array. Yet, it may also contain a counter or timestamp which is changing for most of the requests - invalidating the cache if included.
We are still not there yet with a new concept for Ory Oathkeeper, but I think this is a very interesting problem that could very well warrant a realignment on Ory Oathkeeper which would be to make access control at the reverse proxy as efficient and flexible as possible.
I think this too could benefit from JsonNet, as JsonNet is typable, lintable, and can produce errors (go templating fulfills none of these properties).
Unfortunately, for this PR in particular, I don't think the current implementation can be accepted because it still bears too many risks and this particular type of issue has already caused a CVE in Ory Oathkeeper which is why I am so hyper-sensible about this topic: GHSA-qvp4-rpmr-xwrr
Enable support for configurable mutator cache key per unique rule.
Related issue(s)
None known.
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments