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

Implement additional matching strategies and use negative lookahead regex #321

Closed
aeneasr opened this issue Dec 20, 2019 · 5 comments · Fixed by #334
Closed

Implement additional matching strategies and use negative lookahead regex #321

aeneasr opened this issue Dec 20, 2019 · 5 comments · Fixed by #334

Comments

@aeneasr
Copy link
Member

aeneasr commented Dec 20, 2019

Is your feature request related to a problem? Please describe.

At the moment, access rules will match based on a regular expression, as documented here (see bullet point match).

There are two problems related to that:

  1. The regex stdlib does not support negative lookahead, which can be cumbersome in real-world application as it's not possible to do a "match all except this one element".
  2. Many developers do not like writing regular expressions, but are used to glob pattern matching instead.

Describe the solution you'd like

For regex, we should pick a different library (research needed as to which library suits the best) - one that supports negative lookahead/behind and replace the stdlib regex with that one. There should not be any backwards incompatible changes here!

For glob matching, we should use gobwas/glob. Before implementing glob matching, we have to decide:

  1. How will the access rule decide if glob or regex should be used?
  2. What should the default behavior be glob or regex?

Once these decisions are made, we need to:

  1. If the default behavior is glob, we need to implement an access rule migrator that explicitly enables the "regex" matching strategy.
  2. An abstraction needs to be implemented in the matching algorithm so that we can easily add new pattern matchers easily and without a lot of work.
  3. These changes need to be documented.
@ayzu
Copy link
Contributor

ayzu commented Dec 22, 2019

Hi, @aeneasr

How about this package: regexp2?

It has MIT License and provides extensive functionality. The last commit was made 4 months ago. However, It does not have godoc documentation

Here is an example with negative lookahead.

As far as I understand, we will have to replace previous regexp package with the new one in ladon/complier/regex.go. The existing tests are OK with the new regexp package: ory/ladon#143

What do you think?

aeneasr pushed a commit to ory/ladon that referenced this issue Jan 8, 2020
…143)

Package "github.com/dlclark/regexp2" supports negative lookahead,
which is required by ory/oathkeeper#321

Signed-off-by: Aynur Zulkarnaev <[email protected]>
@ayzu
Copy link
Contributor

ayzu commented Jan 8, 2020

Hi @aeneasr. Regarding the glob matching:

  1. We can add an optional string field on to the global configuration to the section "access_rules". Let's say named "mathching_engine", with two possible values for now: "regexp" and "glob". We could also add this field to every rule configuration. Though, I am not sure whether it is necessary.

  2. We can create an interface Matcher which is responsible for matching and use it here: https://github.com/ory/oathkeeper/blob/master/rule/rule.go#L181

  3. If the field is not set in the configuration, we can use regexp matching by default. Therefore, it will be backwards-compatible.

What do you think?

By the way, there are some failing tests in the master branch. In particular, in file request_handler_test.go.

@aeneasr
Copy link
Member Author

aeneasr commented Jan 8, 2020

I really like that plan! I think matching_engine is a very good idea. We're using strategy as a naming convention, so matching_strategy would probably be a better name but apart form that I think its perfect.

By the way, there are some failing tests in the master branch. In particular, in file request_handler_test.go.

I'm not able to reproduce that - the CI is also passing?

@ayzu
Copy link
Contributor

ayzu commented Jan 8, 2020

I really like that plan! I think matching_engine is a very good idea. We're using strategy as a naming convention, so matching_strategy would probably be a better name but apart form that I think its perfect.

By the way, there are some failing tests in the master branch. In particular, in file request_handler_test.go.

I'm not able to reproduce that - the CI is also passing?

Well, actually it works fine:) The case is that I cloned the repo not to $GOPATH, but to another location. And tests didn't find configuration files. If I put the repo into $GOPATH, it works fine.

@ayzu ayzu mentioned this issue Jan 8, 2020
@aeneasr
Copy link
Member Author

aeneasr commented Jan 9, 2020

Ah I see - makes sense :)

aeneasr pushed a commit that referenced this issue Feb 5, 2020
This patch adds the ability to choose a matching strategy and adds a glob-based matching strategy to the available options (regex is still the default).

Closes #321
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 a pull request may close this issue.

2 participants