Kuadrant / authorino

K8s-native AuthN/AuthZ service to protect your APIs.
Apache License 2.0
201 stars 32 forks source link

CEL Support #495

Closed alexsnaps closed 3 weeks ago

alexsnaps commented 1 month ago

Fixes #475

alexsnaps commented 1 month ago

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

@guicassolato am I missing something else?

alexsnaps commented 1 month ago

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

KevFan commented 1 month ago

@alexsnaps Sure, I'll take a look 👀

KevFan commented 1 month ago

@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 👍

alexsnaps commented 4 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

e2e tests now pass as well... 🎉

alexsnaps commented 3 weeks ago

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