-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
base: master
Are you sure you want to change the base?
Conversation
bfbf92f
to
97192c9
Compare
Codecov Report
@@ 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
|
cc114e8
to
bab7632
Compare
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! |
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? |
bab7632
to
fbcbeeb
Compare
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>
fbcbeeb
to
5542209
Compare
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:
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
introduces a new feature.
contributing code guidelines.
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.
works.
Further Comments