refactor: splitting authorization rule expression into match and resp…

…onse fields (#256)

Signed-off-by: Frank Jogeleit <[email protected]>

.crds/envoy.kyverno.io_authorizationpolicies.yaml

@@ -46,9 +46,13 @@ spec:
                 items:
                   description: Authorization defines an authorization policy rule
                   properties:
-                    expression:
+                    match:
+                      description: Match represents the match condition which will
+                        be evaluated by CEL. Must evaluate to bool.
+                      type: string
+                    response:
                       description: |-
-                        Expression represents the expression which will be evaluated by CEL.
+                        Response represents the response expression which will be evaluated by CEL.
                         ref: https://github.com/google/cel-spec
                         CEL expressions have access to CEL variables as well as some other useful variables:
 
@@ -57,7 +61,7 @@ spec:
                         CEL expressions are expected to return an envoy CheckResponse (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkresponse).
                       type: string
                   required:
-                  - expression
+                  - response
                   type: object
                 type: array
                 x-kubernetes-list-type: atomic

.manifests/policies/demo-policy.example.com.yaml

@@ -13,26 +13,25 @@ spec:
     expression: '{"my-new-metadata": "my-new-value"}'
   authorizations:
     # if force_unauthenticated -> 401
-  - expression: >
-      variables.force_unauthenticated
-        ? envoy
-            .Denied(401)
-            .WithBody("Authentication Failed")
-            .Response()
-        : null
+  - match: variables.force_unauthenticated
+    response: >
+      envoy
+        .Denied(401)
+        .WithBody("Authentication Failed")
+        .Response()
     # if force_authorized -> 200
-  - expression: >
-      variables.force_authorized
-        ? envoy
-            .Allowed()
-            .WithHeader("x-validated-by", "my-security-checkpoint")
-            .WithoutHeader("x-force-authorized")
-            .WithResponseHeader("x-add-custom-response-header", "added")
-            .Response()
-            .WithMetadata(variables.metadata)
-        : null
+  - match: variables.force_authorized
+    response: >
+      envoy
+        .Allowed()
+        .WithHeader("x-validated-by", "my-security-checkpoint")
+        .WithoutHeader("x-force-authorized")
+        .WithResponseHeader("x-add-custom-response-header", "added")
+        .Response()
+        .WithMetadata(variables.metadata)
     # else -> 403
-  - expression: >
+  - match: 'true'
+    response: >
       envoy
         .Denied(403)
         .WithBody("Unauthorized Request")

.schemas/json/authorizationpolicy-envoy-v1alpha1.json

@@ -340,11 +340,18 @@
               "null"
             ],
             "required": [
-              "expression"
+              "response"
             ],
             "properties": {
-              "expression": {
-                "description": "Expression represents the expression which will be evaluated by CEL.\nref: https://github.com/google/cel-spec\nCEL expressions have access to CEL variables as well as some other useful variables:\n\n- 'object' - The object from the incoming request. (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkrequest)\n\nCEL expressions are expected to return an envoy CheckResponse (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkresponse).",
+              "match": {
+                "description": "Match represents the match condition which will be evaluated by CEL. Must evaluate to bool.",
+                "type": [
+                  "string",
+                  "null"
+                ]
+              },
+              "response": {
+                "description": "Response represents the response expression which will be evaluated by CEL.\nref: https://github.com/google/cel-spec\nCEL expressions have access to CEL variables as well as some other useful variables:\n\n- 'object' - The object from the incoming request. (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkrequest)\n\nCEL expressions are expected to return an envoy CheckResponse (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkresponse).",
                 "type": "string"
               }
             },

apis/v1alpha1/types.go

@@ -76,15 +76,18 @@ func (s *AuthorizationPolicySpec) GetFailurePolicy() admissionregistrationv1.Fai
 
 // Authorization defines an authorization policy rule
 type Authorization struct {
-	// Expression represents the expression which will be evaluated by CEL.
+	// Match represents the match condition which will be evaluated by CEL. Must evaluate to bool.
+	// +optional
+	Match string `json:"match,omitempty"`
+	// Response represents the response expression which will be evaluated by CEL.
 	// ref: https://github.com/google/cel-spec
 	// CEL expressions have access to CEL variables as well as some other useful variables:
 	//
 	// - 'object' - The object from the incoming request. (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkrequest)
 	//
 	// CEL expressions are expected to return an envoy CheckResponse (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkresponse).
 	// +required
-	Expression string `json:"expression"`
+	Response string `json:"response"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

charts/kyverno-authz-server/templates/crds.yaml

@@ -55,9 +55,13 @@ spec:
                 items:
                   description: Authorization defines an authorization policy rule
                   properties:
-                    expression:
+                    match:
+                      description: Match represents the match condition which will
+                        be evaluated by CEL. Must evaluate to bool.
+                      type: string
+                    response:
                       description: |-
-                        Expression represents the expression which will be evaluated by CEL.
+                        Response represents the response expression which will be evaluated by CEL.
                         ref: https://github.com/google/cel-spec
                         CEL expressions have access to CEL variables as well as some other useful variables:
 
@@ -66,7 +70,7 @@ spec:
                         CEL expressions are expected to return an envoy CheckResponse (https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/auth/v3/external_auth.proto#service-auth-v3-checkresponse).
                       type: string
                   required:
-                  - expression
+                  - response
                   type: object
                 type: array
                 x-kubernetes-list-type: atomic

pkg/authz/cel/env_test.go → pkg/authz/cel/libs/envoy/lib_test.go

@@ -1,11 +1,13 @@
-package cel
+package envoy_test
 
 import (
 	"reflect"
 	"testing"
 
 	authv3 "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3"
+	"github.com/google/cel-go/cel"
 	"github.com/google/cel-go/interpreter"
+	"github.com/kyverno/kyverno-envoy-plugin/pkg/authz/cel/libs/envoy"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -19,7 +21,7 @@ envoy
 	.WithMetadata({"my-new-metadata": "my-new-value"})
 	.WithMessage("hello")
 `
-	env, err := NewEnv()
+	env, err := cel.NewEnv(envoy.Lib())
 	assert.NoError(t, err)
 	ast, issues := env.Compile(source)
 	assert.Nil(t, issues)

pkg/policy/compiler.go

@@ -32,6 +32,11 @@ func NewCompiler() Compiler {
 type compiler struct{}
 
 func (c *compiler) Compile(policy *v1alpha1.AuthorizationPolicy) (PolicyFunc, field.ErrorList) {
+	type authorizationPrograms struct {
+		Match    cel.Program
+		Response cel.Program
+	}
+
 	var allErrs field.ErrorList
 	base, err := engine.NewEnv()
 	if err != nil {
@@ -51,17 +56,17 @@ func (c *compiler) Compile(policy *v1alpha1.AuthorizationPolicy) (PolicyFunc, fi
 	{
 		path := path.Child("matchConditions")
 		for i, matchCondition := range policy.Spec.MatchConditions {
-			path := path.Index(i)
+			path := path.Index(i).Child("expression")
 			ast, issues := env.Compile(matchCondition.Expression)
 			if err := issues.Err(); err != nil {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), matchCondition.Expression, err.Error()))
+				return nil, append(allErrs, field.Invalid(path, matchCondition.Expression, err.Error()))
 			}
 			if !ast.OutputType().IsExactType(types.BoolType) {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), matchCondition.Expression, "matchCondition output is expected to be of type bool"))
+				return nil, append(allErrs, field.Invalid(path, matchCondition.Expression, "matchCondition output is expected to be of type bool"))
 			}
 			prog, err := env.Program(ast)
 			if err != nil {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), matchCondition.Expression, err.Error()))
+				return nil, append(allErrs, field.Invalid(path, matchCondition.Expression, err.Error()))
 			}
 			matchConditions = append(matchConditions, prog)
 		}
@@ -70,36 +75,59 @@ func (c *compiler) Compile(policy *v1alpha1.AuthorizationPolicy) (PolicyFunc, fi
 	{
 		path := path.Child("variables")
 		for i, variable := range policy.Spec.Variables {
-			path := path.Index(i)
+			path := path.Index(i).Child("expression")
 			ast, issues := env.Compile(variable.Expression)
 			if err := issues.Err(); err != nil {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), variable.Expression, err.Error()))
+				return nil, append(allErrs, field.Invalid(path, variable.Expression, err.Error()))
 			}
 			provider.RegisterField(variable.Name, ast.OutputType())
 			prog, err := env.Program(ast)
 			if err != nil {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), variable.Expression, err.Error()))
+				return nil, append(allErrs, field.Invalid(path, variable.Expression, err.Error()))
 			}
 			variables[variable.Name] = prog
 		}
 	}
-	var authorizations []cel.Program
+	var authorizations []authorizationPrograms
 	{
 		path := path.Child("authorizations")
 		for i, rule := range policy.Spec.Authorizations {
 			path := path.Index(i)
-			ast, issues := env.Compile(rule.Expression)
-			if err := issues.Err(); err != nil {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), rule.Expression, err.Error()))
-			}
-			if !ast.OutputType().IsExactType(envoy.CheckResponse) {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), rule.Expression, "rule output is expected to be of type envoy.service.auth.v3.CheckResponse"))
+			program := authorizationPrograms{}
+
+			if rule.Match != "" {
+				path := path.Child("match")
+				ast, issues := env.Compile(rule.Match)
+				if err := issues.Err(); err != nil {
+					return nil, append(allErrs, field.Invalid(path, rule.Match, err.Error()))
+				}
+				if !ast.OutputType().IsExactType(types.BoolType) {
+					return nil, append(allErrs, field.Invalid(path, rule.Match, "rule match output is expected to be of type bool"))
+				}
+				prog, err := env.Program(ast)
+				if err != nil {
+					return nil, append(allErrs, field.Invalid(path, rule.Match, err.Error()))
+				}
+				program.Match = prog
 			}
-			prog, err := env.Program(ast)
-			if err != nil {
-				return nil, append(allErrs, field.Invalid(path.Child("expression"), rule.Expression, err.Error()))
+
+			{
+				path := path.Child("response")
+				ast, issues := env.Compile(rule.Response)
+				if err := issues.Err(); err != nil {
+					return nil, append(allErrs, field.Invalid(path, rule.Response, err.Error()))
+				}
+				if !ast.OutputType().IsExactType(envoy.CheckResponse) {
+					return nil, append(allErrs, field.Invalid(path, rule.Response, "rule response output is expected to be of type envoy.service.auth.v3.CheckResponse"))
+				}
+				prog, err := env.Program(ast)
+				if err != nil {
+					return nil, append(allErrs, field.Invalid(path, rule.Response, err.Error()))
+				}
+				program.Response = prog
 			}
-			authorizations = append(authorizations, prog)
+
+			authorizations = append(authorizations, program)
 		}
 	}
 	eval := func(r *authv3.CheckRequest) (*authv3.CheckResponse, error) {
@@ -139,8 +167,25 @@ func (c *compiler) Compile(policy *v1alpha1.AuthorizationPolicy) (PolicyFunc, fi
 			})
 		}
 		for _, rule := range authorizations {
+			if rule.Match != nil {
+				// evaluate rule match condition
+				out, _, err := rule.Match.Eval(data)
+				if err != nil {
+					return nil, err
+				}
+				// try to convert to a match result
+				matched, err := utils.ConvertToNative[bool](out)
+				if err != nil {
+					return nil, err
+				}
+				// if condition is false, continue
+				if !matched {
+					continue
+				}
+			}
+
 			// evaluate the rule
-			out, _, err := rule.Eval(data)
+			out, _, err := rule.Response.Eval(data)
 			// check error
 			if err != nil {
 				return nil, err