feat: OR operator #427

guicassolato · 2023-09-20T23:15:43Z

Adds support for logical disjunction (aka "OR" operator) in 'when' conditions and pattern-matching authorization rules.

Closes #424.

Introduces a new field any: Array<expression> to the AuthConfig API to express logical disjunction, along with all: Array<expression> to enforce the AND operation (default if omitted). Where expression can be any of the following:

a pattern composed of selector, operator and value;
a reference to a named pattern (patternRef: String);
a nested any field;
a nested all field.

For backward compatibility, the AND operator is assumed for all lists of patterns not grouped under an explicit any field declaration. I.e., it is equivalent to grouping the patterns under a single all field declaration.

Examples

Example 1. To allow anonymous access (no authentication required) for all requests to path /test/* OR method GET (ocasionally both):

spec:
  authentication:
    anonymous-request:
      when:
      - any:
        - selector: context.request.http.path
          operator: matches
          value: ^/test/.*
        - selector: context.request.http.method
          operator: eq
          method: GET
      anonymous: {}

Example 2. In a more complex condition, with nested logical operators, to express host == 'foo.apis.io' AND ((path =~ '/test*' AND (method == 'POST' OR method == 'PUT')) OR method == 'GET'):

spec:
  authentication:
    anonymous-request:
      when:
      - selector: context.request.http.host
        operator: eq
        value: foo.apis.io
      - any:
        - all:
          - selector: context.request.http.path
            operator: matches
            value: ^/test/.*
          - any:
            - selector: context.request.http.method
              operator: eq
              value: POST
            - selector: context.request.http.method
              operator: eq
              value: PUT
        - selector: context.request.http.method
          operator: eq
          value: GET
      anonymous: {}

Verification steps

Deploy

make local-setup FF=1
kubectl port-forward deployment/envoy 8000:8000 2>&1 >/dev/null &

Try the new feature

① Implicit AND (i.e. the existing syntax, which continues to be supported):

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta2
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api-authorino.127.0.0.1.nip.io
  authorization:
    deny-specific-endpoints:
      when:
      - selector: context.request.http.method
        operator: eq
        value: POST
      - selector: context.request.http.path
        operator: eq
        value: /foo
      opa:
        rego: "allow = false"
EOF

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X POST -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X GET -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/bar -i
# HTTP/1.1 200 OK

② Same as above with explicit AND operator (all):

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta2
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api-authorino.127.0.0.1.nip.io
  authorization:
    deny-specific-endpoints:
      when:
      - all:
        - selector: context.request.http.method
          operator: eq
          value: POST
        - selector: context.request.http.path
          operator: eq
          value: /foo
      opa:
        rego: "allow = false"
EOF

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X POST -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X GET -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/bar -i
# HTTP/1.1 200 OK

③ Logical disjunction (any):

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta2
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api-authorino.127.0.0.1.nip.io
  authorization:
    deny-specific-endpoints:
      when:
      - any:
        - selector: context.request.http.method
          operator: eq
          value: POST
        - selector: context.request.http.method
          operator: eq
          value: PUT
      opa:
        rego: "allow = false"
EOF

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X POST -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X GET -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/bar -X PUT -i
# HTTP/1.1 403 Forbidden

④ Nested OR and AND expression:

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta2
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api-authorino.127.0.0.1.nip.io
  authorization:
    deny-specific-endpoints:
      when:
      - any:
        - all:
          - selector: context.request.http.method
            operator: eq
            value: POST
          - selector: context.request.http.path
            operator: eq
            value: /foo
        - all:
          - selector: context.request.http.method
            operator: eq
            value: PUT
          - selector: context.request.http.path
            operator: eq
            value: /foo
      opa:
        rego: "allow = false"
EOF

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X POST -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X PUT -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X GET -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/bar -X PUT -i
# HTTP/1.1 200 OK

⑤ Same as above with implicit AND:

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta2
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api-authorino.127.0.0.1.nip.io
  authorization:
    deny-specific-endpoints:
      when:
      - selector: context.request.http.path
        operator: eq
        value: /foo
      - any:
        - selector: context.request.http.method
          operator: eq
          value: POST
        - selector: context.request.http.method
          operator: eq
          value: PUT
      opa:
        rego: "allow = false"
EOF

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X POST -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X PUT -i
# HTTP/1.1 403 Forbidden

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X GET -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/bar -X PUT -i
# HTTP/1.1 200 OK

⑥ Edge case: Invalid condition:

kubectl apply -f -<<EOF
apiVersion: authorino.kuadrant.io/v1beta2
kind: AuthConfig
metadata:
  name: talker-api-protection
spec:
  hosts:
  - talker-api-authorino.127.0.0.1.nip.io
  authorization:
    foo:
      when:
      - all:
        - invalid: object
      opa:
        rego: "allow = false"
EOF

Because CRDs defined containing recursive types (e.g. an object/struct that defines a field whose type is the same type as the object itself) result in invalid OAS, the new fields all and any have to be marked with x-kubernetes-preserve-unknown-fields. Therefore, Kube API won't validate the scheme of these field, effectively accepting any array. The AuthConfig controller should not fail nonetheless, but just ignore the invalid condition instead. In the future, this can be improved with further validation.

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X POST -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/foo -X PUT -i
# HTTP/1.1 200 OK

curl http://talker-api-authorino.127.0.0.1.nip.io:8000/bar -X PUT -i
# HTTP/1.1 200 OK

…tching authorization) Adds support for logical disjunction (aka "OR" operator) in 'when' conditions and pattern-matching authorization rules. A new field `any: Array<expression>` was introduced to the AuthConfig API to express logical disjunction, along with `all: Array<expression>` to enforce the default AND operation, where `expression` can be any of the following: - a pattern composed of `selector`, `operator` and `value`; - a reference to a named pattern (`patternRef: String`); - a nested `any` field; - a nested `all` field. For backward compatibility, the AND operator is assumed for all lists of patterns not grouped under an explicit `any` field declaration. I.e., it is equivalent to grouping the patterns under a single `all` field declaration. Example 1) To allow anonymous access (no authentication required) for all request to path `/test/*` OR method `GET` (ocasionally both): ```yaml spec: authentication: anonymous-request: when: - any: - selector: context.request.http.path operator: matches value: ^/test/.* - selector: context.request.http.method operator: eq method: GET anonymous: {} ``` In a more complex condition, with nested logical operators, to express `host == 'foo.apis.io' AND ((path =~ '/test*' AND (method == 'POST' OR method == 'PUT')) OR method == 'GET')`: ```yaml spec: authentication: anonymous-request: when: - selector: context.request.http.host operator: eq value: foo.apis.io - any: - all: - selector: context.request.http.path operator: matches value: ^/test/.* - any: - selector: context.request.http.method operator: eq value: POST - selector: context.request.http.method operator: eq value: PUT - selector: context.request.http.method operator: eq value: GET anonymous: {} ```

pkg/evaluators/authorization/json_test.go

pkg/jsonexp/expressions.go

maleck13 · 2023-09-21T12:16:56Z

pkg/jsonexp/expressions.go

+	}
+}
+
+func Any(expressions ...Expression) Expression {


so all will default to true if no experessions passed and any will default to false if no expressions passed? Is that what is intended?

This has surprised more than one, but yes... See https://en.wikipedia.org/wiki/Vacuous_truth

maleck13 · 2023-09-21T12:22:58Z

pkg/jsonexp/expressions_test.go

+	assert.Check(t, !ok)
+}
+
+func TestEmptyOr(t *testing.T) {


perhaps being stupid, but not getting why an empty or returns false but an empty and returns true?

This is because every expression has 2 sides, left and right, and sometimes we instantiate expressions where the right-hand side is another empty nested expression. This is the case of expressions initialized with All()
and Any(), where we don't want the empty nested expressions to affect the result of the outer one.

A more obvious (and possibly easier to read) alternative would be defining trivial True and False expressions to use instead of the empty ones.

Additionally, see #427 (comment)

maleck13

The changes and test all look good to me, caveat I am not an expert on Authorino. One or two small questions but generally it is a looks good to me

maleck13 · 2023-09-25T08:36:11Z

docs/features.md

+spec:
+  when: # auth enforced only on requests with HTTP method equals to POST or PUT
+  - any:
+    - selector: context.request.http.method


side point: Have you considered and in operator as this would (at least from an API perspective) improve the verbosity.

- selector: context.request.http.method operator: in values: ["PUT","POST"]

I guess you could also do this with regexp and the matches operator?

maleck13 · 2023-09-25T08:43:04Z

docs/features.md

+    operator: matches
+    value: ^/resources/.*
+  - any:
+    - selector: context.request.http.method


nit: is this the best example? Seems it could be achieved also with a matches operator? without too complex of a regex?

Indeed, many cases of disjunction can be solved with the matches operator, and this is definitely true for patterns limited to finite sets (and many other infinite ones as well!), such as the values of the HTTP method. For other cases, users may want/have to go with any.

So I wanted to present any also as an alternative to matches.

Click here if you want to learn more about the limitations of the matches operator.

Because Golang regexp package (behind our matches operator) is a strict implementation of regular expressions, in the theoretical computer science sense of the term (i.e., the kind that defines a regular language), some patterns often supported by other programming languages are simply not available in Go by default.

For details, see https://github.com/google/re2/wiki/Syntax.

But that was not the only reason though. There are yet other cases where the disjunction involves patterns with different selectors. E.g.: If (method == <X>) OR (path =~ <Y>.*).

I could have used any one of those more complex cases here. I was trying to reduce the cognitive complexity of the examples by going with something simple and straightforward. I guess I didn't account for the more skilled readers, like you, who often knows a little bit more about the suject and therefore also other ways to solve the problem 🙂

maleck13 · 2023-09-25T09:14:03Z

docs/features.md

+        - any:
+          - selector: context.request.http.method
+            operator: eq
+            value: POST


just a note I do find myself wanting to write this as a matches operator :)

docs/features.md

adam-cattermole

I've gone through the verification and code and the changes here look good and make sense from my point of view

guicassolato self-assigned this Sep 20, 2023

guicassolato changed the title ~~feat: OR operator for the pattern-matching (conditions and pattern-matching authorization)~~ feat: OR operator Sep 20, 2023

guicassolato requested a review from a team September 21, 2023 08:38

maleck13 reviewed Sep 21, 2023

View reviewed changes

pkg/evaluators/authorization/json_test.go Show resolved Hide resolved

maleck13 reviewed Sep 21, 2023

View reviewed changes

pkg/jsonexp/expressions.go Outdated Show resolved Hide resolved

maleck13 reviewed Sep 21, 2023

View reviewed changes

pkg/jsonexp/expressions.go Show resolved Hide resolved

maleck13 reviewed Sep 21, 2023

View reviewed changes

refactor: avoid else branch in the evaluation of jsonexp.RegexOperator

70b2b1f

maleck13 previously approved these changes Sep 21, 2023

View reviewed changes

docs: AND and OR conditions

302e420

guicassolato dismissed maleck13’s stale review via 302e420 September 22, 2023 09:50

guicassolato requested a review from a team September 22, 2023 09:54

maleck13 reviewed Sep 25, 2023

View reviewed changes

docs/features.md Outdated Show resolved Hide resolved

docs: fix typo at: AND and OR conditions

3ec4f6b

adam-cattermole approved these changes Sep 26, 2023

View reviewed changes

maleck13 approved these changes Sep 26, 2023

View reviewed changes

guicassolato merged commit d6111da into main Sep 26, 2023
9 checks passed

guicassolato deleted the or-conditions branch September 26, 2023 14:01

This was referenced Sep 26, 2023

Update Authorino manifests Kuadrant/authorino-operator#140

Merged

fix: missing one-of constraint of top-level when conditions #432

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: OR operator #427

feat: OR operator #427

guicassolato commented Sep 20, 2023 •

edited

Loading

maleck13 Sep 21, 2023

alexsnaps Sep 21, 2023

maleck13 Sep 21, 2023

guicassolato Sep 21, 2023

maleck13 left a comment

maleck13 Sep 25, 2023 •

edited

Loading

maleck13 Sep 25, 2023 •

edited

Loading

guicassolato Sep 26, 2023

maleck13 Sep 25, 2023

guicassolato Sep 26, 2023 •

edited

Loading

maleck13 Sep 25, 2023

adam-cattermole left a comment

feat: OR operator #427

feat: OR operator #427

Conversation

guicassolato commented Sep 20, 2023 • edited Loading

Examples

Verification steps

Deploy

Try the new feature

maleck13 Sep 21, 2023

Choose a reason for hiding this comment

alexsnaps Sep 21, 2023

Choose a reason for hiding this comment

maleck13 Sep 21, 2023

Choose a reason for hiding this comment

guicassolato Sep 21, 2023

Choose a reason for hiding this comment

maleck13 left a comment

Choose a reason for hiding this comment

maleck13 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

maleck13 Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

guicassolato Sep 26, 2023

Choose a reason for hiding this comment

maleck13 Sep 25, 2023

Choose a reason for hiding this comment

guicassolato Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

maleck13 Sep 25, 2023

Choose a reason for hiding this comment

adam-cattermole left a comment

Choose a reason for hiding this comment

guicassolato commented Sep 20, 2023 •

edited

Loading

maleck13 Sep 25, 2023 •

edited

Loading

maleck13 Sep 25, 2023 •

edited

Loading

guicassolato Sep 26, 2023 •

edited

Loading