feat: Add support for header in rule matching #967

m-guesnon-pvotal · 2022-05-18T17:17:59Z

This pull request aims at extending the matching rules of ory oathkeeper with support for header matching.
Header-based routing pattern are a very common when in comes to versioned APIs. However, we should also consider that new versions of an API may result in a need for different authorization rules.

As such, support for header based decision making is relevant. We used to solve this issue with a proxy between our gateway and authService (oathkeeper) but figured it would be a welcome addition to oathkeeper rule definitions.

This change should NOT be a breaking change since defining headers in the rule is not required.

Related issue(s)

Documentation pull request can be found here

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

I'm not quite positive on which files in the repository are generated through your CI. Should I update the schema definitions?

codecov · 2022-05-18T18:46:31Z

Codecov Report

Merging #967 (bec0e7e) into master (af5ce29) will increase coverage by 0.13%.
The diff coverage is 89.47%.

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

@@            Coverage Diff             @@
##           master     #967      +/-   ##
==========================================
+ Coverage   77.65%   77.78%   +0.13%     
==========================================
  Files          80       80              
  Lines        3965     3980      +15     
==========================================
+ Hits         3079     3096      +17     
+ Misses        607      606       -1     
+ Partials      279      278       -1

Impacted Files	Coverage Δ
rule/repository_memory.go	`85.52% <33.33%> (ø)`
rule/rule.go	`84.40% <93.54%> (+4.17%)`	⬆️
api/decision.go	`95.55% <100.00%> (ø)`
middleware/grpc_middleware.go	`72.41% <100.00%> (ø)`
proxy/proxy.go	`71.84% <100.00%> (ø)`

... and 1 file with indirect coverage changes

m-guesnon-pvotal · 2022-05-18T21:18:37Z

Formatting with npm run format to please the CI modified all the *.md file. This can be reverted if needed.

aeneasr

Could you please revert all changes not related to this PR? :)

m-guesnon-pvotal · 2022-05-31T09:41:46Z

Done. However, this brought back the linter fail for CI. (I suspect current master could not pass the CI)

a-manraj-pvotal · 2022-06-07T12:24:33Z

@aeneasr any news for linter and the content of the PR itself ?

a-manraj-pvotal · 2022-06-15T13:59:29Z

Outside linting @aeneasr any changes required or discussion to alter ?

aeneasr

Sorry for the super late review, my backlog is simply too big at the moment. I have a couple of questions for this PR and would be thankful if you could answer those

aeneasr · 2022-06-21T09:47:24Z

rule/rule.go

+	// A map of HTTP headers. When ORY Oathkeeper searches for rules
+	// to decide what to do with an incoming request to the proxy server, it compares the HTTP headers of the incoming
+	// request with the HTTP headers of each rules. If a match is found, the rule is considered a partial match.
+	// For headers with values in array format (e.g. User-Agent headers), the rule header value must match at least one
+	// of the request header values.
+	// If the matchesUrl and matchesMethods fields are satisfied as well, the rule is considered a full match.


Wha is the difference between a partial and a full match?

This is wording have taken from the comments on other fields.
My interpretation is that a partial match means you satisfied to one aspect of the rule. The incoming request has to be a (partial) match to all aspects of this rule in order to become a full match.

aeneasr · 2022-06-21T09:48:47Z

rule/rule.go

+	// For headers with values in array format (e.g. User-Agent headers), the rule header value must match at least one
+	// of the request header values.
+	// If the matchesUrl and matchesMethods fields are satisfied as well, the rule is considered a full match.
+	Headers map[string]string `json:"headers"`


As far as I understand, this is an AND match. So if all requirements are met from the rule, it will apply. Is that desired? Are there use cases for OR? And if so, how do we handle those? We can also only support AND for now but will nede to support OR most likely at some point, without breaking the config.

At this point, the implementation only supports AND.
You have to match all headers specified in the rule. IMHO, an OR implementation would be overcomplicating things. It should just be a different rule. Headers are seen just like URLs are (although, it is true that URLs support wildcards).

I think that using using OR is definitely a breaking change as it would potentially override normal oathkeeper rule matching and documentation with an optional routing header that was aimed at obtaining rule unicity per route with header (header : service aV1 to match a certain rule and header: service aV2 to match another rule and for both to live at the same time). Therefore i would recommend supporting only AND for now as it is meant only as an advanced configuration parameter to address multi service version routing.

Ok, that makes sense!

aeneasr · 2022-09-10T08:53:19Z

If you rebase on master, I think that the CI should pass!

aeneasr · 2022-09-10T08:53:28Z

I'm unfortunately not allowed to do so :(

vinckr · 2023-05-03T09:02:45Z

@m-guesnon-pvotal @a-manraj-pvotal
Apologies for the ping, but can you take another look at this so we can move it forward please?

# Conflicts: # api/decision.go # api/decision_test.go # proxy/proxy.go # rule/matcher.go # rule/matcher_test.go # rule/repository_memory.go # rule/rule.go # rule/rule_test.go

m-guesnon-pvotal · 2023-05-03T18:09:21Z

@vinckr Done. Sorry, I missed @aeneasr message and never followed up.

feat: Add support for header in rule matching

4b4144e

m-guesnon-pvotal mentioned this pull request May 18, 2022

docs: Add basic document on matching rules support for header fields ory/docs#819

Open

6 tasks

m-guesnon-pvotal added 2 commits May 18, 2022 13:44

Force ory-prettier-styles version to 1.2.0 to match CI

14eab8b

Trigger rebuild

a88a7c5

m-guesnon-pvotal marked this pull request as ready for review May 18, 2022 21:18

m-guesnon-pvotal requested a review from aeneasr as a code owner May 18, 2022 21:18

aeneasr requested changes May 26, 2022

View reviewed changes

m-guesnon-pvotal and others added 2 commits May 31, 2022 05:36

Revert formatting

ca4d80f

Merge branch 'master' into feat/matching-rule-header-support

29138bb

m-guesnon-pvotal requested a review from aeneasr May 31, 2022 09:41

aeneasr requested changes Jun 21, 2022

View reviewed changes

Merge branch 'master' into feat/matching-rule-header-support

c56898a

m-guesnon-pvotal requested a review from aeneasr August 23, 2022 18:29

m-guesnon-pvotal force-pushed the feat/matching-rule-header-support branch from 7688aa2 to c56898a Compare May 3, 2023 13:51

m-guesnon-pvotal added 3 commits May 3, 2023 10:05

Merge branch 'master' into feat/matching-rule-header-support

d52a51b

# Conflicts: # api/decision.go # api/decision_test.go # proxy/proxy.go # rule/matcher.go # rule/matcher_test.go # rule/repository_memory.go # rule/rule.go # rule/rule_test.go

fix: Unit tests. Run format

8c51cd5

fix: Rule wording. Full header match for consistency

2290ce1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for header in rule matching #967

feat: Add support for header in rule matching #967

m-guesnon-pvotal commented May 18, 2022 •

edited

Loading

codecov bot commented May 18, 2022 •

edited

Loading

m-guesnon-pvotal commented May 18, 2022

aeneasr left a comment

m-guesnon-pvotal commented May 31, 2022

a-manraj-pvotal commented Jun 7, 2022

a-manraj-pvotal commented Jun 15, 2022

aeneasr left a comment

aeneasr Jun 21, 2022

m-guesnon-pvotal Jul 4, 2022

aeneasr Jun 21, 2022

m-guesnon-pvotal Jul 4, 2022

a-manraj-pvotal Jul 13, 2022 •

edited

Loading

aeneasr Sep 10, 2022

aeneasr commented Sep 10, 2022

aeneasr commented Sep 10, 2022

vinckr commented May 3, 2023

m-guesnon-pvotal commented May 3, 2023

feat: Add support for header in rule matching #967

Are you sure you want to change the base?

feat: Add support for header in rule matching #967

Conversation

m-guesnon-pvotal commented May 18, 2022 • edited Loading

Related issue(s)

Checklist

Further Comments

codecov bot commented May 18, 2022 • edited Loading

Codecov Report

m-guesnon-pvotal commented May 18, 2022

aeneasr left a comment

Choose a reason for hiding this comment

m-guesnon-pvotal commented May 31, 2022

a-manraj-pvotal commented Jun 7, 2022

a-manraj-pvotal commented Jun 15, 2022

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jun 21, 2022

Choose a reason for hiding this comment

m-guesnon-pvotal Jul 4, 2022

Choose a reason for hiding this comment

aeneasr Jun 21, 2022

Choose a reason for hiding this comment

m-guesnon-pvotal Jul 4, 2022

Choose a reason for hiding this comment

a-manraj-pvotal Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

aeneasr Sep 10, 2022

Choose a reason for hiding this comment

aeneasr commented Sep 10, 2022

aeneasr commented Sep 10, 2022

vinckr commented May 3, 2023

m-guesnon-pvotal commented May 3, 2023

m-guesnon-pvotal commented May 18, 2022 •

edited

Loading

codecov bot commented May 18, 2022 •

edited

Loading

a-manraj-pvotal Jul 13, 2022 •

edited

Loading