CEL Support #495

alexsnaps · 2024-10-14T19:32:00Z

Fixes #475

guicassolato · 2024-10-15T13:27:16Z

install/rbac/role.yaml

+- apiGroups:
+  - operator.authorino.kuadrant.io
+  resources:
+  - authorinos


There's something wrong going on here. It's somehow bringing to the operand permissions that only belong to the operator.

Ah! Again... remember it happened to me already when I refactored the controller to v1beta2 🤔

Could you try and run it on your machine and push the changes to this branch please? I have no idea what's screwed up on my end 🤷

Done. 28e7fb9

❯ git st On branch cel Your branch is up to date with 'origin/cel'. nothing to commit, working tree clean ❯ make generate manifests go mod tidy go: downloading github.com/envoyproxy/go-control-plane v0.12.1-0.20240621013728-1eb8caab5155 go: downloading github.com/prometheus/client_golang v1.20.2 go: downloading github.com/open-policy-agent/opa v0.68.0 go: downloading google.golang.org/grpc v1.66.0 go: downloading go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.53.0 go: downloading github.com/google/cel-go v0.21.0 go: downloading google.golang.org/genproto/googleapis/rpc v0.0.0-20240701130421-f6361c86f094 go: downloading github.com/klauspost/compress v1.17.9 go: downloading golang.org/x/time v0.6.0 go: downloading github.com/stoewer/go-strcase v1.2.0 go: downloading cloud.google.com/go/compute/metadata v0.3.0 go: downloading github.com/golang/glog v1.2.1 go: downloading golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d go: downloading cloud.google.com/go/compute v1.23.0 go: downloading cloud.google.com/go v0.34.0 go mod vendor controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..." /Library/Developer/CommandLineTools/usr/bin/make fmt vet go fmt ./... go vet ./... controller-gen crd:crdVersions=v1 rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=install/crd output:rbac:artifacts:config=install/rbac && /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/bin/kustomize build install > /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml /Library/Developer/CommandLineTools/usr/bin/make patch-webhook envsubst \ < /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml \ > /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml.tmp && \ mv /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml.tmp /Users/guilherme.cassolato/Workspace/go/src/github.com/kuadrant/authorino/install/manifests.yaml ❯ git st On branch cel Your branch is up to date with 'origin/cel'. Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) modified: install/manifests.yaml modified: install/rbac/role.yaml no changes added to commit (use "git add" and/or "git commit -a") ❯ grep "controller-gen@" Makefile $(call go-get-tool,$(CONTROLLER_GEN),sigs.k8s.io/controller-tools/cmd/[email protected]) ❯ ./bin/controller-gen --version Version: v0.15.0

alexsnaps · 2024-10-15T14:40:58Z

🤦 This should in v1beta3 ...
I think we still miss

Some "CEL enabled" HttpEndpointSpec.Url alternative
PlainIdentitySpec.CelExpression
Delete response.DynamicCEL
... and moving this to v1beta3

@guicassolato am I missing something else?

alexsnaps · 2024-10-15T14:42:59Z

pkg/evaluators/authorization/kubernetes_authz.go

-		return fmt.Sprintf("%s", value.ResolveFor(authJSON))
+	jsonValueToStr := func(value expressions.Value) (string, error) {
+		if value == nil {
+			return "", nil


Ugh... Now that it is a pointer, we could get nil indeed... I don't know whether all these cases are now covered, hard to tell what can or not be nil in those specs

guicassolato · 2024-10-16T08:18:29Z

api/v1beta1/zz_generated.deepcopy.go

@@ -1,5 +1,4 @@
 //go:build !ignore_autogenerated
-// +build !ignore_autogenerated


The // +build syntax has been deprecated since Go v1.17.

https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md

guicassolato · 2024-10-16T08:23:52Z

api/v1beta3/auth_config_types.go

+// AuthConfig is the schema for Authorino's AuthConfig API
+// +kubebuilder:object:root=true
+// +kubebuilder:subresource:status
+// +kubebuilder:storageversion


Shall we remove it from v1beta2?

One and only one version must be marked as the storage version.

https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#specify-multiple-versions

guicassolato · 2024-10-16T08:25:20Z

api/v1beta3/auth_config_conversion.go

+package v1beta3
+
+// Hub marks this version as a conversion hub.
+func (a *AuthConfig) Hub() {}


I don't know for sure how this works, but can we have 2 versions marked as conversion hub? I believe right now v1beta2 is also marked as hub, right?

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps · 2024-10-29T16:20:55Z

e2e tests now pass as well... 🎉

alexsnaps · 2024-10-29T16:22:25Z

This below would let us get rid of all the remainder selector et al stuff:

diff --git a/pkg/expressions/cel/expressions.go b/pkg/expressions/cel/expressions.go
index 18ba5728..40fd95b9 100644
--- a/pkg/expressions/cel/expressions.go
+++ b/pkg/expressions/cel/expressions.go
@@ -9,6 +9,7 @@ import (
 	"github.com/google/cel-go/cel"
 	"github.com/google/cel-go/checker/decls"
 	"github.com/google/cel-go/common/types/ref"
+	"github.com/google/cel-go/ext"
 	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/structpb"
@@ -130,6 +131,7 @@ func Compile(expression string, expectedType *cel.Type, opts ...cel.EnvOption) (
 		decls.NewConst(RootDestinationBinding, decls.NewObjectType("google.protobuf.Struct"), nil),
 		decls.NewConst(RootAuthBinding, decls.NewObjectType("google.protobuf.Struct"), nil),
 	)}, opts...)
+	envOpts = append(envOpts, ext.Strings())
 	env, env_err := cel.NewEnv(envOpts...)
 	if env_err != nil {
 		return nil, env_err
diff --git a/pkg/expressions/cel/expressions_test.go b/pkg/expressions/cel/expressions_test.go
index 587ac4cd..6154d020 100644
--- a/pkg/expressions/cel/expressions_test.go
+++ b/pkg/expressions/cel/expressions_test.go
@@ -42,3 +42,14 @@ func TestPredicate(t *testing.T) {
 	assert.NilError(t, err)
 	assert.Equal(t, response, true)
 }
+
+func TestExpression(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	expression, err := NewStringExpression(`"hello hello".replace("", "_")`)
+	assert.NilError(t, err)
+	resolveFor, err := expression.ResolveFor("{}")
+	assert.NilError(t, err)
+	assert.Equal(t, resolveFor, "_h_e_l_l_o_ _h_e_l_l_o_")
+}
diff --git a/tests/v1beta3/authconfig.yaml b/tests/v1beta3/authconfig.yaml
index 509b93fe..554f2f62 100644
--- a/tests/v1beta3/authconfig.yaml
+++ b/tests/v1beta3/authconfig.yaml
@@ -77,10 +77,10 @@ spec:
           Accept:
             value: application/json
         method: GET
-        url: http://ip-location.authorino.svc.cluster.local:3000/{context.request.http.headers.x-forwarded-for.@extract:{"sep":","}}
+        urlExpression: "http://ip-location.authorino.svc.cluster.local:3000/" + request.headers["x-forwarded-for"].split(",")[0]
       cache:
         key:
-          selector: request.http.headers.x-forwarded-for.@extract:{"sep":","}
+          expresssion: request.headers["x-forwarded-for"].split(",")[0]
     user-info:
       userInfo:
         identitySource: keycloak
@@ -179,7 +179,7 @@ spec:
               uri:
                 expression: request.path
               scope:
-                selector: request.http.method.@case:lower
+                expression: request.http.method.lowerAscii()
             signingKeyRefs:
             - name: wristband-signing-key
               algorithm: ES256

pkg/expressions/cel/expressions.go

guicassolato · 2024-10-30T06:46:00Z

controllers/auth_config_controller.go

+func stringValueFrom(user *api.ValueOrSelector) (expressions.Value, error) {
+	var strValue expressions.Value
+	var err error
+	if user.Expression != "" {
+		if strValue, err = cel.NewStringExpression(string(user.Expression)); err != nil {
+			return nil, err
+		}
+	} else {
+		strValue = &json.JSONValue{Static: user.Value, Pattern: user.Selector}
+	}
+	return strValue, nil
+}
+
+func valueFrom(user *api.ValueOrSelector) (expressions.Value, error) {
+	var strValue expressions.Value
+	var err error
+	if user.Expression != "" {
+		if strValue, err = cel.NewStringExpression(string(user.Expression)); err != nil {
+			return nil, err
+		}
+	} else {
+		strValue = &json.JSONValue{Static: user.Value, Pattern: user.Selector}
+	}
+	return strValue, nil
+}


Is there any difference between these two funcs?

Typo! This the latter should create a NewExpression! Tho I wonder if we should keep that one altogether... (by the current state of things, I'd say no!)

guicassolato · 2024-10-30T07:00:15Z

controllers/auth_config_controller.go

@@ -272,7 +284,15 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf
 			}

 		case api.PlainIdentityAuthentication:
-			translatedIdentity.Plain = &identity_evaluators.Plain{Pattern: identity.Plain.Selector}
+			if identity.Plain.Expression != "" {
+				expression, err := cel.NewStringExpression(string(identity.Plain.Expression))


Not sure if understand NewStringExpression here. Does this mean plain.expression will always resolve to a string?

With JSONValue, I think plain.selector can resolve to any type, including returning complex objects to set auth.identity, no?

So, what would be plain.selector value's type for auth.identity if one would be to select that? In Golang's type system that is...

from gjson

func (t Result) Value() interface{} { if t.Type == String { return t.Str } switch t.Type { default: return nil case False: return false case Number: return t.Num case JSON: r := t.arrayOrMap(0, true) if r.vc == '{' { return r.oi } else if r.vc == '[' { return r.ai } return nil case True: return true } }

So a map[string]interface{}, where interface is one of the above recursively I guess...

One more question tho... what is that good for tho? I realize I took a shortcut here, and even left a note to make this yet even (according to me) better, but... we're just marshaling json back and forth, right? This only ever gets used thru AuthPipeline.GetAuthorizationJSON, no? i.e. as JSON?

I think this is where the mismatch happens:

ResolveFor(jsonData string) (interface{}, error)

ResolveFor is the thing that only ever get to interact with these payloads, is that correct? So we end up spending all our CPU cycles marshaling that data back and forth, are we not? Am I missing something?

So, what would be plain.selector value's type for auth.identity if one would be to select that? In Golang's type system that is...

Not sure if I follow. plain.selector is not for selecting from auth.identity; it selects for setting the value of auth.identity. What it selects can be of any type.

So, what would be plain.selector value's type for auth.identity if one would be to select that? In Golang's type system that is...

Not sure if I follow. plain.selector is not for selecting from auth.identity; it selects for setting the value of auth.identity. What it selects can be of any type.

Not going to elaborate on this... as it's orthogonal and I found the answer, as per this answer. But the follow up questions remain. I mean I can go thru the whole process of remapping to golang native types... to only marshal them back into JSON literals later... but it makes no sense, other than something needing to use that in golang. Tho even then I'd argue it might be better to optimize for the most frequent data access, which seems to be JSON based of my scanning of the code base.

I do think that, if we ever 🔥 the gjson stuff, as per my comment in the code, ideally the pipeline stores all that data as protobuf... which would make it much lighter weight to inject it into the CEL interpreter.

So I can have the resulting value of a CEL expression be returned from ResolveFor, be expressed in golang's type system, but I really think this isn't sensible... as, again afaict! but I could be missing something, nothing actually needs that, other than to re marshal it back into JSON...

Is my reasoning sensible? Am I missing something that renders it inapplicable to the problem space?

Also, I think ResolveFor(jsonData string) (interface{}, error) should probably not expect json in and golang out, but have a clear contract, i.e. json string in & out or interface{} in (tho probably the proper type in, not "any") & out... It's not really symmetrical. But not arguing to fix this as part of this PR or even now neither

For future us... Not sensible for this PR I think

Thinking out loud further...

ResolveFor should probably be ResolveFor(pipeline *AuthPipeline) (interface{}, error).
That now would let us control what the expression takes in as argument format. gjson stuff can call GetAuthorizationJSON(), while CEL could bypass it go straight to the protobuf format. I'd still argue for storing things in protobuf tho... So in order to remain "compatible", the result would be... "undefined" which probably isn't good... 🤔

Let me show what I mean with an example...

Requisite:

Add the omitempty suggested at CEL Support #495 (comment)

Run the service:

export LOG_LEVEL=debug; make cluster install run

Create a couple of AuthConfigs expected to behave exactly the same:

kubectl apply -f -<<EOF apiVersion: authorino.kuadrant.io/v1beta3 kind: AuthConfig metadata: name: gjson spec: hosts: ["gjson"] authentication: "envoy-verified-jwt": plain: selector: metadata.filter_metadata.envoy\.filters\.http\.jwt_authn|verified_jwt authorization: "must_be_admin": patternMatching: patterns: - selector: auth.identity.roles operator: incl value: admin --- apiVersion: authorino.kuadrant.io/v1beta3 kind: AuthConfig metadata: name: cel spec: hosts: ["cel"] authentication: "envoy-verified-jwt": plain: expression: | metadata.filter_metadata["envoy.filters.http.jwt_authn"].verified_jwt authorization: "must_be_admin": patternMatching: patterns: - predicate: | "admin" in auth.identity.roles EOF

Send a request to the AuthConfig using plain.selector:

grpcurl -plaintext -d @ localhost:50051 envoy.service.auth.v3.Authorization.Check <<EOF { "attributes": { "request": { "http": { "host": "gjson" } }, "metadata_context": { "filter_metadata": { "envoy.filters.http.jwt_authn": { "verified_jwt": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3", "roles": ["admin"] } } } } } } EOF

{ "status": {}, "okResponse": {}, "dynamicMetadata": {} }

Now send a request to the AuthConfig using plain.expression:

grpcurl -plaintext -d @ localhost:50051 envoy.service.auth.v3.Authorization.Check <<EOF { "attributes": { "request": { "http": { "host": "cel" } }, "metadata_context": { "filter_metadata": { "envoy.filters.http.jwt_authn": { "verified_jwt": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3", "roles": ["admin"] } } } } } } EOF

{ "status": { "code": 7 }, "deniedResponse": { "status": { "code": "Forbidden" }, "headers": [ { "header": { "key": "X-Ext-Auth-Reason", "value": "no such key: roles" } } ] } }

If you check the logs, both requests succeed in the authentication, however...

The first request evaluates authorisation based on the following Authorization JSON:

{ "auth": { "identity": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "roles": [ "admin" ], "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3" } }, "context": { "metadata_context": { "filter_metadata": { "envoy.filters.http.jwt_authn": { "verified_jwt": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "roles": [ "admin" ], "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3" } } } }, "request": { "http": { "host": "gjson", "id": "b59827c5-ec62-422c-972c-b86f1d29ec52" } } }, "destination": {}, "metadata": { "filter_metadata": { "envoy.filters.http.jwt_authn": { "verified_jwt": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "roles": [ "admin" ], "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3" } } } }, "request": { "host": "gjson", "id": "b59827c5-ec62-422c-972c-b86f1d29ec52" }, "source": {} }

The second request evaluates authorisation based on the following Authorization JSON:

{ "auth": { "identity": "{\"aud\":\"toystore\", \"exp\":1730284979, \"iat\":1730371379, \"iss\":\"auth-server.io\", \"roles\":[\"admin\"], \"sub\":\"55474041-c8c6-48bb-9398-34c2dbfd88b3\"}" }, "context": { "metadata_context": { "filter_metadata": { "envoy.filters.http.jwt_authn": { "verified_jwt": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "roles": [ "admin" ], "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3" } } } }, "request": { "http": { "host": "cel", "id": "04eb5f71-ec3c-40f1-a941-a7a5f0b3390b" } } }, "destination": {}, "metadata": { "filter_metadata": { "envoy.filters.http.jwt_authn": { "verified_jwt": { "aud": "toystore", "exp": 1730284979, "iat": 1730371379, "iss": "auth-server.io", "roles": [ "admin" ], "sub": "55474041-c8c6-48bb-9398-34c2dbfd88b3" } } } }, "request": { "host": "cel", "id": "04eb5f71-ec3c-40f1-a941-a7a5f0b3390b" }, "source": {} }

guicassolato · 2024-10-30T07:09:10Z

controllers/auth_config_controller.go

-				Static:  property.Value,
-				Pattern: property.Selector,
-			}, false))
+			if value, err := stringValueFrom(&property); err != nil {


Same as https://github.com/Kuadrant/authorino/pull/495/files#r1821989655. The value of an extended property can be of any type.

Also, just to be clear (and we may want to rename that function), this does return a "JSON literal", but for String returns a plain string... i.e. w/o the quotes.

Signed-off-by: Alex Snaps <[email protected]>

api/v1beta3/auth_config_types.go

Signed-off-by: Guilherme Cassolato <[email protected]>

Signed-off-by: Alex Snaps <[email protected]>

guicassolato

Amazing work, @alexsnaps!

I've manually tried this against a couple use cases myself and it seems to work like a charm.

alexsnaps force-pushed the cel branch 2 times, most recently from fc65fbc to dc50681 Compare October 14, 2024 20:05

alexsnaps changed the title ~~First seam: DynamicCEL~~ CEL Support Oct 14, 2024

alexsnaps force-pushed the cel branch 3 times, most recently from 00949f1 to 7f17d95 Compare October 15, 2024 11:19

guicassolato reviewed Oct 15, 2024

View reviewed changes

alexsnaps force-pushed the cel branch from 6147481 to 5a229aa Compare October 15, 2024 14:29

alexsnaps commented Oct 15, 2024

View reviewed changes

alexsnaps force-pushed the cel branch 2 times, most recently from 901996d to 067f83d Compare October 15, 2024 15:35

alexsnaps requested a review from guicassolato October 15, 2024 17:06

alexsnaps marked this pull request as ready for review October 15, 2024 17:06

alexsnaps force-pushed the cel branch from b0b2967 to 41caa3b Compare October 15, 2024 17:18

guicassolato reviewed Oct 16, 2024

View reviewed changes

alexsnaps force-pushed the cel branch 2 times, most recently from dc2777a to a25343f Compare October 18, 2024 12:06

alexsnaps added 10 commits October 18, 2024 16:37

First seam: DynamicCEL

3d79881

Signed-off-by: Alex Snaps <[email protected]>

Refactored Cel into own pkg, removed CelResponse

79882b3

Signed-off-by: Alex Snaps <[email protected]>

Adding the new seams

95d3bfa

Signed-off-by: Alex Snaps <[email protected]>

Cel Predicates

e138990

Signed-off-by: Alex Snaps <[email protected]>

Add context to Cel's context

6087b3c

Signed-off-by: Alex Snaps <[email protected]>

Cel Expressions

e469c17

Signed-off-by: Alex Snaps <[email protected]>

Abstracted expressions.Value away somewhat

d6dc884

Signed-off-by: Alex Snaps <[email protected]>

Fix imports

cd1b510

Signed-off-by: Alex Snaps <[email protected]>

Wired cel.Expression in the AuthConfig

5431fed

Signed-off-by: Alex Snaps <[email protected]>

Fix imports

69d0613

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps added 4 commits October 28, 2024 15:11

Like I need to understand this crap

2f5d43e

Signed-off-by: Alex Snaps <[email protected]>

All the things...

3d1e1b8

Signed-off-by: Alex Snaps <[email protected]>

Dunno where this comes from

5f94c7c

Signed-off-by: Alex Snaps <[email protected]>

Neither where this is supposed to come from

b7cc27f

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps force-pushed the cel branch from bc55987 to b7cc27f Compare October 29, 2024 13:49

Exploring while I can

44052a6

Signed-off-by: Alex Snaps <[email protected]>

alexsnaps force-pushed the cel branch from fd1d837 to 821e220 Compare October 29, 2024 16:10

alexsnaps requested review from guicassolato and KevFan October 29, 2024 16:21