#321 #334

ayzu · 2020-01-08T20:54:22Z

Related issue

Closes #321

Proposed changes

Checklist

[X ] I have read the contributing guidelines
[X ] I have read the security policy
[X ] 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
[email protected]) from the maintainers to push
the changes.
[X ] I have added tests that prove my fix is effective or that my feature works
[X ] I have added necessary documentation within the code base (if appropriate)
[X ] I have documented my changes in the
developer guide (if appropriate)

Further comments

ayzu · 2020-01-08T21:01:24Z

@aeneasr probably, it would make sense to discuss the draft, to verify whether I am heading the right direction.

aeneasr · 2020-01-09T09:31:15Z

@ayzu yes, I will review it now. By the way, did you see the E-Mail from Jared?

ayzu · 2020-01-09T09:45:15Z

Yes, I replied to it recently.

…

On Thu, 9 Jan 2020, 10:31 hackerman, ***@***.***> wrote: @ayzu <https://github.com/ayzu> yes, I will review it now. By the way, did you see the E-Mail from Jared? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#334?email_source=notifications&email_token=AMAKVPFICD7GN5GZPBLNP4TQ43VGHA5CNFSM4KEONRJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIPTZDQ#issuecomment-572472462>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMAKVPALLE22EN2W25OLSADQ43VGHANCNFSM4KEONRJA> .

aeneasr

This looks really good! I just have some small nit picks but I think this is on track!

driver/configuration/provider.go

aeneasr · 2020-01-09T09:49:21Z

rule/fetcher_default.go

+
+	f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "entrypoint"})
+
+	viperx.AddWatcher(func(e fsnotify.Event) error {


Note to myself: Check if this will work as intended

rule/repository_memory.go

aeneasr · 2020-01-09T09:52:06Z

rule/rule.go

@@ -138,14 +140,6 @@ type Upstream struct {

 var _ json.Unmarshaler = new(Rule)

-func NewRule() *Rule {


A particular reason for removing this?

I didn't see usages of it.

rule/rule.go

rule/rule_test.go

ayzu · 2020-01-09T10:27:51Z

pipeline/authz/keto_engine_acp_ory.go

@@ -115,9 +110,19 @@ func (a *AuthorizerKetoEngineACPORY) Authorize(r *http.Request, session *authn.A

 	var b bytes.Buffer
 	u := fmt.Sprintf("%s://%s%s", r.URL.Scheme, r.URL.Host, r.URL.Path)
+
+	action, err := rule.Replace(u, cf.RequiredAction)


@aeneasr Do you intend to have this Replace functionality for regexp strategy only or for both regexp and glob?

Hm, that is a good question. I'm not sure if we can actually use this for glob, right?

I also think that we should really update this handler and make it more generic. So it would be fine for me to just support this specific handler for regexp, and throw an error if glob is used (for now).

What do you think?

Hm, that is a good question. I'm not sure if we can actually use this for glob, right?

I also think that we should really update this handler and make it more generic. So it would be fine for me to just support this specific handler for regexp, and throw an error if glob is used (for now).

What do you think?

I agree. I don't see the out-of-the-box replacement functionality for glob.

We can add a check for a flavour here: https://github.com/ory/oathkeeper/blob/master/pipeline/authz/keto_engine_acp_ory.go#L111 and return an error if it is not a blank string (defaults to regex), "regex" or "text". Otherwise, we invoke rule.ReplaceAllString and proceed as usual. We can rename this method to RegexpReplaceAll to show that currently only regexp replacement is supported. What do you think?

What's the difference, by the way, between "text" and "regex"? It seems like both flavours invoke regexp.ReplaceAllString.

return an error if it is not a blank string (defaults to regex), "regex" or "text". Otherwise, we invoke rule.ReplaceAllString and proceed as usual. We can rename this method to RegexpReplaceAll to show that currently only regexp replacement is supported. What do you think?

What's the difference, by the way, between "text" and "regex"? It seems like both flavours invoke regexp.ReplaceAllString.

So the flavor is actually only relevant for the upstream service (ORY Keto), not for ORY Oathkeeper itself. Therefore, the flavor does not matter regarding the logic here - we can simply decide based on the access_rule matching strategy!

ok, thank you, @aeneasr

As we discussed, I added a new interface MatchingEngine and its glob implementation.

I will some tests and update documentation later.

rule/rule.go

ayzu · 2020-01-12T22:43:33Z

@aeneasr one more general question, about copyright in newly created files. Is it supposed to be the same as in existing ones? How about the copyright years?

aeneasr · 2020-01-13T13:17:22Z

@aeneasr one more general question, about copyright in newly created files. Is it supposed to be the same as in existing ones? How about the copyright years?

Yeah we haven't really been super diligent with that. We definitely need something to automate this in the future. Maybe via the CI or something.

ayzu · 2020-01-15T16:09:00Z

@aeneasr I have created a corresponding PR in docs repo. I believe this PR now is review-ready:)

aeneasr

As discussed

api/decision_test.go

api/rule_test.go

rule/matcher_test.go

ayzu · 2020-02-02T20:27:01Z

Hi @aeneasr !

As discussed, I have merged the tests.

I have one check failing: https://circleci.com/gh/ory/oathkeeper/3381?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-checks-link

[198/323] pkg:golang/github.com/opencontainers/[email protected] [Vulnerable] 1 known vulnerabilities affecting installed version

This is an indirect dependency. As far as I understand 0.1.1 is the latest stable version (though they do have some newer release candidates).

What do you think?

aeneasr · 2020-02-03T08:32:31Z

This is an indirect dependency. As far as I understand 0.1.1 is the latest stable version (though they do have some newer release candidates).

The CVE affects everything below 1.0.0-rc.7 I believe. I've updated ory/x (which uses ory/dockertest, which uses runc) to bump runc -> dockertest -> x. If you update ory/x (which it is on master I believe so a merge/rebase should be enough) it should work :)

ayzu · 2020-02-03T11:05:28Z

Yes, ory/x -> dockertst -> runc ory/x v0.0.93 (latest tag) depends on runc 1.0.0-rc9 and we can use it. Some other dependencies in `ory/oathkeeper` require the previous versions of ory/x 0.0.88 and 0.0.85 (which in turn depend on a vulnerable version of runc): ``` go mod graph | grep github.com/ory/x github.com/ory/oathkeeper github.com/ory/[email protected] github.com/ory/sdk/[email protected] github.com/ory/[email protected] github.com/ory/[email protected] github.com/ory/[email protected]``` Do you want me to update them? An alternative would be to use `replace` statement in `ory/oathkeeper` for dependency `runc` and therefore force every other dependency to use the right version of runc.

…

On Mon, Feb 3, 2020 at 9:32 AM hackerman ***@***.***> wrote: This is an indirect dependency. As far as I understand 0.1.1 is the latest stable version (though they do have some newer release candidates). The CVE affects everything below 1.0.0-rc.7 I believe. I've updated ory/x (which uses ory/dockertest, which uses runc) to bump runc -> dockertest -> x. If you update ory/x (which it is on master I believe so a merge/rebase should be enough) it should work :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#334?email_source=notifications&email_token=AMAKVPH5EJZNQUQZKQUSP33RA7JB7A5CNFSM4KEONRJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKS54QI#issuecomment-581295681>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMAKVPDRWA3KQ2SGLOIQ5DTRA7JB7ANCNFSM4KEONRJA> .

aeneasr · 2020-02-03T11:38:51Z

Yeah, you have to update ory/x to 0.0.93 :) The library is very unstable, there might be build issues but they should be easy to resolve. If not, I can help

This update allows to use negative lookahead in regexp. Signed-off-by: Aynur Zulkarnaev <[email protected]>

Signed-off-by: Aynur Zulkarnaev <[email protected]>

…p object This change can help to unify regexp and glob matching strategies. Signed-off-by: Aynur Zulkarnaev <[email protected]>

Signed-off-by: Aynur Zulkarnaev <[email protected]>

… glob. Signed-off-by: Aynur Zulkarnaev <[email protected]>

Signed-off-by: Aynur Zulkarnaev <[email protected]>

ayzu · 2020-02-03T12:52:18Z

ok, @aeneasr

I did a rebase, so now it uses ory/x 0.0.93

aeneasr

Sweet! Just some minor things but I think this is good to merge after they have been addressed!

README.md

rule/engine_glob.py.go

rule/engine_regexp.go

rule/engine_glob.py.go

rule/matcher_test.go

Signed-off-by: Aynur Zulkarnaev <[email protected]>

ayzu · 2020-02-04T11:13:02Z

Sweet! Just some minor things but I think this is good to merge after they have been addressed!

Thank you for the comments, I have updated the code.

aeneasr · 2020-02-05T10:49:03Z

Awesome, thank you for your hard work!

See ory/oathkeeper#334 Signed-off-by: Aynur Zulkarnaev <[email protected]>

ayzu · 2020-02-05T10:55:20Z

Happy to commit and thank you for the reviews!

…

On Wed, Feb 5, 2020 at 11:49 AM hackerman ***@***.***> wrote: Awesome, thank you for your hard work! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#334?email_source=notifications&email_token=AMAKVPGHWFOC4ISLUWEE6WTRBKKR7A5CNFSM4KEONRJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK27HBI#issuecomment-582349701>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AMAKVPCMGC6JELYMB4UQDBLRBKKR7ANCNFSM4KEONRJA> .

aeneasr · 2020-02-05T11:18:41Z

No problem, will probably roll out a new release for this soon :)

aeneasr reviewed Jan 9, 2020

View reviewed changes

ayzu commented Jan 9, 2020

View reviewed changes

ayzu commented Jan 12, 2020

View reviewed changes

rule/rule.go Outdated Show resolved Hide resolved

ayzu mentioned this pull request Jan 15, 2020

oathkeeper: document matching strategy ory/docs#248

Merged

ayzu marked this pull request as ready for review January 15, 2020 15:42

aeneasr reviewed Jan 16, 2020

View reviewed changes

api/decision_test.go Outdated Show resolved Hide resolved

api/rule_test.go Outdated Show resolved Hide resolved

ayzu commented Feb 2, 2020

View reviewed changes

rule/matcher_test.go Show resolved Hide resolved

Aynur Zulkarnaev added 12 commits February 3, 2020 13:35

rule: update github.com/ory/ladon to v.1.1.0

d5bb95a

This update allows to use negative lookahead in regexp. Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: add MatchingStrategy parameter to configuration

643eecf

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: change pipeline.Rule interface so that it does not return regex…

71594cd

…p object This change can help to unify regexp and glob matching strategies. Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: added MatchingStrategy stub switch in rule.IsMatching

93772ff

Signed-off-by: Aynur Zulkarnaev <[email protected]>

docs: update API docs link

ce6da72

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: added MatchingEngine interface and it's implementation based on…

e906e27

… glob. Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: added tests for glob MatchingEngine

320981f

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: changed MatchingStrategy type from int to string

7288a4b

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: add tests for glob matching strategy

1c93b5d

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: add a test for glob matching strategy

1202198

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: add a test for reloading MatchingStrategy config parameter

e78681f

Signed-off-by: Aynur Zulkarnaev <[email protected]>

rule: configuration update for MatchingStrategy

c65dce5

Signed-off-by: Aynur Zulkarnaev <[email protected]>

Aynur Zulkarnaev added 2 commits February 3, 2020 13:35

refactor: merge glob and regexp tests

4154503

Signed-off-by: Aynur Zulkarnaev <[email protected]>

refactor: go mod tidy

7d153d4

Signed-off-by: Aynur Zulkarnaev <[email protected]>

aeneasr reviewed Feb 3, 2020

View reviewed changes

rule: fix code review comments

7de92eb

Signed-off-by: Aynur Zulkarnaev <[email protected]>

aeneasr mentioned this pull request Feb 5, 2020

Generalize the use of values extracted from the rule #292

Closed

aeneasr merged commit 5f983ab into ory:master Feb 5, 2020

aeneasr pushed a commit to ory/docs that referenced this pull request Feb 5, 2020

docs(oathkeeper): Document glob matching strategy (#248)

7910ac2

See ory/oathkeeper#334 Signed-off-by: Aynur Zulkarnaev <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#321 #334

#321 #334

ayzu commented Jan 8, 2020 •

edited

Loading

ayzu commented Jan 8, 2020

aeneasr commented Jan 9, 2020

ayzu commented Jan 9, 2020 via email •

edited

Loading

aeneasr left a comment

aeneasr Jan 9, 2020

aeneasr Jan 9, 2020

ayzu Jan 9, 2020

ayzu Jan 9, 2020

aeneasr Jan 10, 2020

ayzu Jan 10, 2020

aeneasr Jan 11, 2020

ayzu Jan 12, 2020

ayzu commented Jan 12, 2020

aeneasr commented Jan 13, 2020

ayzu commented Jan 15, 2020

aeneasr left a comment

ayzu commented Feb 2, 2020

aeneasr commented Feb 3, 2020

ayzu commented Feb 3, 2020 via email •

edited

Loading

aeneasr commented Feb 3, 2020

ayzu commented Feb 3, 2020

aeneasr left a comment

ayzu commented Feb 4, 2020

aeneasr commented Feb 5, 2020

ayzu commented Feb 5, 2020 via email

aeneasr commented Feb 5, 2020


		f.enqueueEvent(events, event{et: eventRepositoryConfigChanged, source: "entrypoint"})

		viperx.AddWatcher(func(e fsnotify.Event) error {

		@@ -138,14 +140,6 @@ type Upstream struct {

		var _ json.Unmarshaler = new(Rule)

		func NewRule() *Rule {

#321 #334

#321 #334

Conversation

ayzu commented Jan 8, 2020 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

ayzu commented Jan 8, 2020

aeneasr commented Jan 9, 2020

ayzu commented Jan 9, 2020 via email • edited Loading

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayzu commented Jan 12, 2020

aeneasr commented Jan 13, 2020

ayzu commented Jan 15, 2020

aeneasr left a comment

Choose a reason for hiding this comment

ayzu commented Feb 2, 2020

aeneasr commented Feb 3, 2020

ayzu commented Feb 3, 2020 via email • edited Loading

aeneasr commented Feb 3, 2020

ayzu commented Feb 3, 2020

aeneasr left a comment

Choose a reason for hiding this comment

ayzu commented Feb 4, 2020

aeneasr commented Feb 5, 2020

ayzu commented Feb 5, 2020 via email

aeneasr commented Feb 5, 2020

ayzu commented Jan 8, 2020 •

edited

Loading

ayzu commented Jan 9, 2020 via email •

edited

Loading

ayzu commented Feb 3, 2020 via email •

edited

Loading