From e6ef7419cb0eb56ae5b008e0467255df054bb60a Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 13 Nov 2024 14:16:12 -0500 Subject: [PATCH] Have only auth accessing actions happen after auth Signed-off-by: Alex Snaps --- .../envoy_gateway_extension_reconciler.go | 8 +- controllers/istio_extension_reconciler.go | 17 +++- pkg/wasm/types.go | 21 +++++ pkg/wasm/types_test.go | 90 +++++++++++++++++++ 4 files changed, 134 insertions(+), 2 deletions(-) diff --git a/controllers/envoy_gateway_extension_reconciler.go b/controllers/envoy_gateway_extension_reconciler.go index adf5d90a9..4a89c0313 100644 --- a/controllers/envoy_gateway_extension_reconciler.go +++ b/controllers/envoy_gateway_extension_reconciler.go @@ -185,7 +185,13 @@ func (r *EnvoyGatewayExtensionReconciler) buildWasmConfigs(ctx context.Context, // rate limit if effectivePolicy, ok := effectiveRateLimitPoliciesMap[pathID]; ok { - actions = append(actions, buildWasmActionsForRateLimit(effectivePolicy, state)...) + rlAction := buildWasmActionsForRateLimit(effectivePolicy, state) + if hasAuthAccess(rlAction) { + actions = append(actions, rlAction...) + } else { + // pre auth rate limiting + actions = append(rlAction, actions...) + } } if len(actions) == 0 { diff --git a/controllers/istio_extension_reconciler.go b/controllers/istio_extension_reconciler.go index 293793808..3b19ad8b3 100644 --- a/controllers/istio_extension_reconciler.go +++ b/controllers/istio_extension_reconciler.go @@ -188,7 +188,13 @@ func (r *IstioExtensionReconciler) buildWasmConfigs(ctx context.Context, state * // rate limit if effectivePolicy, ok := effectiveRateLimitPoliciesMap[pathID]; ok { - actions = append(actions, buildWasmActionsForRateLimit(effectivePolicy, state)...) + rlAction := buildWasmActionsForRateLimit(effectivePolicy, state) + if hasAuthAccess(rlAction) { + actions = append(actions, rlAction...) + } else { + // pre auth rate limiting + actions = append(rlAction, actions...) + } } if len(actions) == 0 { @@ -212,6 +218,15 @@ func (r *IstioExtensionReconciler) buildWasmConfigs(ctx context.Context, state * return wasmConfigs, nil } +func hasAuthAccess(actionSet []wasm.Action) bool { + for _, action := range actionSet { + if action.HasAuthAccess() { + return true + } + } + return false +} + // buildIstioWasmPluginForGateway builds a desired WasmPlugin custom resource for a given gateway and corresponding wasm config func buildIstioWasmPluginForGateway(gateway *machinery.Gateway, wasmConfig wasm.Config) *istioclientgoextensionv1alpha1.WasmPlugin { wasmPlugin := &istioclientgoextensionv1alpha1.WasmPlugin{ diff --git a/pkg/wasm/types.go b/pkg/wasm/types.go index c6316aa32..ceca23335 100644 --- a/pkg/wasm/types.go +++ b/pkg/wasm/types.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "strings" _struct "google.golang.org/protobuf/types/known/structpb" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -144,6 +145,26 @@ type Action struct { Data []DataType `json:"data,omitempty"` } +func (a *Action) HasAuthAccess() bool { + for _, predicate := range a.Predicates { + if strings.Contains(predicate, "auth.") { + return true + } + } + for _, data := range a.Data { + switch val := data.Value.(type) { + case *Static: + + continue + case *Expression: + if strings.Contains(val.ExpressionItem.Value, "auth.") { + return true + } + } + } + return false +} + func (a *Action) EqualTo(other Action) bool { if a.Scope != other.Scope || a.ServiceName != other.ServiceName || diff --git a/pkg/wasm/types_test.go b/pkg/wasm/types_test.go index 314d4e27c..d79a9dcc5 100644 --- a/pkg/wasm/types_test.go +++ b/pkg/wasm/types_test.go @@ -236,3 +236,93 @@ data: }) } } + +func TestAuthAccesses(t *testing.T) { + action := Action{ + ServiceName: "ratelimit-service", + Scope: "default/other", + Predicates: []string{ + "source.address != '127.0.0.1'", + }, + Data: []DataType{ + { + Value: &Static{ + Static: StaticSpec{ + Key: "limit.global__f63bec56", + Value: "1", + }, + }, + }, + }, + } + + if action.HasAuthAccess() { + t.Fatal("must not have auth access") + } + + action = Action{ + ServiceName: "ratelimit-service", + Scope: "default/other", + Predicates: []string{ + "auth.something != '127.0.0.1'", + }, + Data: []DataType{ + { + Value: &Static{ + Static: StaticSpec{ + Key: "limit.global__f63bec56", + Value: "1", + }, + }, + }, + }, + } + + if !action.HasAuthAccess() { + t.Fatal("must have auth access") + } + + action = Action{ + ServiceName: "ratelimit-service", + Scope: "default/other", + Predicates: []string{ + "source.address != '127.0.0.1'", + }, + Data: []DataType{ + { + Value: &Expression{ + ExpressionItem: ExpressionItem{ + Key: "limit.global__f63bec56", + Value: "auth.identity.anonymous", + }, + }, + }, + }, + } + + if !action.HasAuthAccess() { + t.Fatal("must have auth access") + } + + action = Action{ + ServiceName: "ratelimit-service", + Scope: "default/other", + Predicates: []string{ + "source.address != '127.0.0.1'", + }, + Data: []DataType{ + { + Value: &Static{ + Static: StaticSpec{ + Key: "auth.global__f63bec56", + Value: "auth", + }, + }, + }, + }, + } + + if action.HasAuthAccess() { + t.Fatal("must not have auth access") + } +}