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: allow for easily matching rules using path prefixes #1073

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

davidspek
Copy link
Contributor

@davidspek davidspek commented Mar 1, 2023

Currently rules are matched to an incoming request based on regex or glob pattern matching. Since only a single rule is allowed to match a request, the regex or glob patterns must be very precise and become increasingly complex as different paths need to be matched. While the used regex library supports negative lookahead, creating rules where requests are matched based on path prefixes is still difficult since you'd need to provide negative matches for all other rule regexes with the same base path. As glob doesn't support negative lookahead doing path prefix based routing is not possible. Even when using regex patterns with negative lookahead, the issue that arises is that the end user ends up needing to manage system with the state of all rules to then be able to create rules with the appropriate regexes including negative lookahead patterns for all other rules. Since the most common type of routing people likely would want to implement is prefixed based routing, the fact that doing so with oathkeeper is difficult and error prone is in my views its biggest drawback.

This PR introduces a system that allows for matching rules based on the longest matching path prefix between a request URL and the paths of all the rules using a Trie. With this trie, oathkeeper does not need to range over each rule when performing the matching which I expect will decrease latency.

Note, this implementation is likely incomplete and requires some further work which I would like to implement after some further discussion here. Some things that come to mind are:

  • a way to be able to define multiple schemes in a single rule (currently only a single scheme per rule is possible since the URL needs to be parsable by url.Parse())
  • allow for regex or glob matching groups since these can be used downstream in authenticators, authorisers and mutators.

Update:
I've made the prefix matching a separate config from the matching strategy. This way, if multiple rules are found based on the path prefix those rules are then further filtered using the matching pattern. Matching patterns can only be used in the path of the URL and are not added into the Trie (since they would break the Trie). Thus, 2 rules with the same path prefix but different matching patterns will be added to the same node in the Trie. Then if a request comes in that matches those rules it will be matched using the pattern. Note that the intension for pattern matching combined with prefix matching is for use of the patterns in downstream handlers. It is not intended to be used for determining which rule an incoming request should match against, but it will fall back to doing so. In any case, it is much easier to create a negative lookahead for a single path section rather than all paths known to oathkeeper.

Related issue(s)

#1073
#441

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 the approval (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 Mar 1, 2023

Codecov Report

Merging #1073 (eb4a176) into master (3a716f2) will decrease coverage by 0.44%.
The diff coverage is 60.86%.

❗ Current head eb4a176 differs from pull request most recent head 5542209. Consider uploading reports for the commit 5542209 to get more accurate results

@@            Coverage Diff             @@
##           master    #1073      +/-   ##
==========================================
- Coverage   78.17%   77.73%   -0.44%     
==========================================
  Files          80       81       +1     
  Lines        3853     3979     +126     
==========================================
+ Hits         3012     3093      +81     
- Misses        566      603      +37     
- Partials      275      283       +8     
Impacted Files Coverage Δ
driver/configuration/provider_koanf.go 88.13% <0.00%> (-0.76%) ⬇️
rule/repository_memory.go 75.92% <45.45%> (-9.61%) ⬇️
rule/trie.go 69.56% <69.56%> (ø)

@davidspek davidspek force-pushed the prefix-trie-engine branch 2 times, most recently from cc114e8 to bab7632 Compare March 20, 2023 12:39
@aeneasr
Copy link
Member

aeneasr commented Mar 22, 2023

Hey David, thank you for this PR. We are also struggling ourselves with this problem (increasingly complex rules that are hard to read) but do not have a clear idea how to address these issues properly. Do you have some examples for the rule matching you're suggestion? I'm not quite sure if I understand the approach correctly.

In the future may I suggest to first create a design document to exchange ideas and follow up with an implementation afterwards? :) I know that we are not as responsive as in other repositories but design documents are a good way to work on topics such as this one.

Thanks!

@davidspek
Copy link
Contributor Author

Hi Aeneasr. I can create an issue with a design document for what I’ve done in this PR tomorrow. However, I find that it usually helps to have a (brief) synchronous discussion to get on the same page before creating long write-ups. I think this would be particularly helpful since this codebase is mostly foreign to me. Do you think you’d have time to have a short chat some day soon?

Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants