authorino icon indicating copy to clipboard operation
authorino copied to clipboard

CEL Support

Open alexsnaps opened this issue 1 year ago • 5 comments

Fixes #475

alexsnaps avatar Oct 14 '24 19:10 alexsnaps

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

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

@guicassolato am I missing something else?

alexsnaps avatar Oct 15 '24 14:10 alexsnaps

@KevFan could you have a look at this wrt the v1beta3 changes... my environment is acting up on these generated changes... 🙏

alexsnaps avatar Oct 22 '24 11:10 alexsnaps

@alexsnaps Sure, I'll take a look 👀

KevFan avatar Oct 22 '24 11:10 KevFan

@alexsnaps when running make manifests generate, I got a diff compared to yours. I've a PR at https://github.com/Kuadrant/authorino/pull/499 to merge to this PR if you want to merge it into this or take the changes or if you want me to push directly to this PR 🤔 Whichever works best for you anyway 👍

KevFan avatar Oct 22 '24 13:10 KevFan

@KevFan Thanks for your help fixing the generated stuff... but I fear I could still use either your or @guicassolato 's help. Integration tests fail still:

Test failed [#5]:
  GET   http://talker-api.127.0.0.1.nip.io:8000/greetings/1
  Authorization: Bearer eyJhbGciOiJSUzI1NiIsImtpZCI6InpTZThzUGx3YXIwOVhCc2RITTd4cU45YU1iY09uNEdMRnoyWlRUckFfMkkifQ.eyJhdWQiOlsiaHR0cHM6Ly9rdWJlcm5ldGVzLmRlZmF1bHQuc3ZjLmNsdXN0ZXIubG9jYWwiXSwiZXhwIjoxNzI5ODY3NjE3LCJpYXQiOjE3Mjk4NjcwMTcsImlzcyI6Imh0dHBzOi8va3ViZXJuZXRlcy5kZWZhdWx0LnN2Yy5jbHVzdGVyLmxvY2FsIiwia3ViZXJuZXRlcy5pbyI6eyJuYW1lc3BhY2UiOiJhdXRob3Jpbm8iLCJzZXJ2aWNlYWNjb3VudCI6eyJuYW1lIjoiYXBwLTEtc2EiLCJ1aWQiOiIwNDRmNWU4Mi01MDQxLTQwOWMtYjdlNy02YjkyZTY1YmNmYTQifX0sIm5iZiI6MTcyOTg2NzAxNywic3ViIjoic3lzdGVtOnNlcnZpY2VhY2NvdW50OmF1dGhvcmlubzphcHAtMS1zYSJ9.oCZ8tqz2mo0QbxwGulyhPz0WKdghsoWOYv3-awh_mtBbcL0ERpxfHg4OyoVnTLj2TvLGDx9ntKt-ag17zHYm8mUgdCQNA7qFFcb1DkN4CFnHkNMn7rwx7Bnv4CwyCbjNM79R5GL3jJu7X55I9PgskStFsXw_LKdV7ttn1Gw2TexYM3QLHjSc8ZXTDFAFoBc8H-19WWFsHRE3zC8erpvAqXzneCBICa9xrbEBOri62teA-yP9bcDp32ROa3MukaYq5JmH79X83Yj03ApG4lD77AiCrokPu2Px4DEUAZRMUSrFwJj7-NcS1ZXGwOh1svSN47HaTOb03j8a8Pn9qksDmg
  X-Forwarded-For: 109.69.200.56

  Expected: 403
  Actual: 200


FAIL
make: *** [Makefile:162: e2e] Error 1

But if I then rerun it:

$  ./tests/e2e-test.sh
deployment.apps/coredns condition met
deployment.apps/ip-location unchanged
service/ip-location unchanged
deployment.apps/authorino condition met
deployment.apps/envoy condition met
deployment.apps/ip-location condition met
deployment.apps/keycloak condition met
deployment.apps/talker-api condition met
waiting keycloak ready condition met
waiting oidc config ready. condition met
testing invalid authconfig [SUCCESS]
testing valid authconfig
authconfig.authorino.kuadrant.io/e2e-test configured
secret/oauth2-token-introspection-credentials-keycloak configured
secret/talker-api-uma-credentials configured
secret/wristband-signing-key configured
secret/bob-api-key configured
secret/alice-api-key configured
serviceaccount/app-1-sa unchanged
serviceaccount/app-2-sa unchanged
clusterrole.rbac.authorization.k8s.io/talker-api-admin unchanged
clusterrolebinding.rbac.authorization.k8s.io/talker-api-admins configured
waiting authconfig ready. condition met
sending multiple requests
.................................................

SUCCESS

They succeed. I guess this some caching issue somewhere... I'll keep on investigating, but a second set of eyes would be appreciated. This is much deeper into the inner working of Authorino than I had hoped for...

alexsnaps avatar Oct 25 '24 14:10 alexsnaps

After much headache, I think I found the issue: None of the expressions are being evaluated and I get these warnings about them:

authorino.KubeAPIWarningLogger  unknown field "spec.authentication.anonymous.defaults.username.Expression"
authorino.KubeAPIWarningLogger  unknown field "spec.authentication.api-key.defaults.kubernetes-rbac.Expression"

So pretty sure there is something I didn't do to declare these fields and most importantly have them evaluated! @guicassolato @KevFan Or anyone with k8s magic could help me with this?

alexsnaps avatar Oct 28 '24 16:10 alexsnaps

e2e tests now pass as well... 🎉

alexsnaps avatar Oct 29 '24 16:10 alexsnaps

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

alexsnaps avatar Oct 29 '24 16:10 alexsnaps