Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Composite configurable mutator cache key per rule. #885

Draft
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

anderslauri
Copy link

@anderslauri anderslauri commented Dec 17, 2021

Enable support for configurable mutator cache key per unique rule.

Related issue(s)

None known.

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 green light (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

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Attention: Patch coverage is 72.91667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 62.69%. Comparing base (9910160) to head (4ab1904).
Report is 355 commits behind head on master.

Files with missing lines Patch % Lines
pipeline/mutate/mutator_hydrator.go 72.91% 8 Missing and 5 partials ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

pipeline/mutate/mutator_hydrator.go Outdated Show resolved Hide resolved
pipeline/mutate/mutator_hydrator_test.go Outdated Show resolved Hide resolved
.schema/config.schema.json Outdated Show resolved Hide resolved
break
case len(cfg.Cache.Key) > 0:
// Build a composite cache key with property from configuration.
if cacheSession, ok := a.hydrateFromCache(a.cacheKey(
Copy link
Author

@anderslauri anderslauri Dec 25, 2021

Choose a reason for hiding this comment

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

Key is a composite key based on configuration property + three distinct repeatable properties (URL for hydrator, rule id and subject of session).

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Author

@anderslauri anderslauri Dec 26, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@anderslauri anderslauri Jan 6, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

That's a brilliant idea! Exclusion is much safer (because explicit) :)

@anderslauri
Copy link
Author

anderslauri commented Dec 26, 2021

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

Thank you, please have another review. This includes some refactoring, introduction of sync.Pool and increased test-coverage of cache, both for custom key and also default behavior.

pipeline/mutate/mutator_hydrator.go Outdated Show resolved Hide resolved
pipeline/mutate/mutator_hydrator_test.go Outdated Show resolved Hide resolved
break
case len(cfg.Cache.Key) > 0:
// Build a composite cache key with property from configuration.
if cacheSession, ok := a.hydrateFromCache(a.cacheKey(
Copy link
Member

Choose a reason for hiding this comment

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

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

@aeneasr aeneasr marked this pull request as draft January 6, 2022 07:04
@aeneasr aeneasr added the rfc A request for comments to discuss and share ideas. label Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc A request for comments to discuss and share ideas.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants