From 2784176997f54e203149928f248ab5a687d2d5ac Mon Sep 17 00:00:00 2001 From: Guilherme Cassolato Date: Tue, 19 Nov 2024 14:36:16 +0100 Subject: [PATCH] Disallow empty AuthPolicies Signed-off-by: Guilherme Cassolato --- api/v1/authpolicy_types.go | 3 + ...adrant-operator.clusterserviceversion.yaml | 2 +- .../manifests/kuadrant.io_authpolicies.yaml | 9 ++ .../templates/manifests.yaml | 9 ++ .../crd/bases/kuadrant.io_authpolicies.yaml | 9 ++ .../authpolicy/authpolicy_controller_test.go | 118 ++++++++++++++++-- 6 files changed, 137 insertions(+), 13 deletions(-) diff --git a/api/v1/authpolicy_types.go b/api/v1/authpolicy_types.go index d1878d63e..edb2ba2c5 100644 --- a/api/v1/authpolicy_types.go +++ b/api/v1/authpolicy_types.go @@ -292,6 +292,9 @@ func (p *AuthPolicy) Kind() string { // +kubebuilder:validation:XValidation:rule="!(has(self.defaults) && (has(self.patterns) || has(self.when) || has(self.rules)))",message="Implicit and explicit defaults are mutually exclusive" // +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && (has(self.patterns) || has(self.when) || has(self.rules)))",message="Implicit defaults and explicit overrides are mutually exclusive" // +kubebuilder:validation:XValidation:rule="!(has(self.overrides) && has(self.defaults))",message="Explicit overrides and explicit defaults are mutually exclusive" +// +kubebuilder:validation:XValidation:rule="!(has(self.overrides) || has(self.defaults)) ? has(self.rules) && size(self.rules.authentication) > 0 : true",message="At least one spec.rules.authentication must be defined" +// +kubebuilder:validation:XValidation:rule="has(self.overrides) ? has(self.overrides.rules) && size(self.overrides.rules.authentication) > 0 : true",message="At least one spec.overrides.rules.authentication must be defined" +// +kubebuilder:validation:XValidation:rule="has(self.defaults) ? has(self.defaults.rules) && size(self.defaults.rules.authentication) > 0 : true",message="At least one spec.defaults.rules.authentication must be defined" type AuthPolicySpec struct { // Reference to the object to which this policy applies. // +kubebuilder:validation:XValidation:rule="self.group == 'gateway.networking.k8s.io'",message="Invalid targetRef.group. The only supported value is 'gateway.networking.k8s.io'" diff --git a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml index 1900a00f0..40ef14571 100644 --- a/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml +++ b/bundle/manifests/kuadrant-operator.clusterserviceversion.yaml @@ -109,7 +109,7 @@ metadata: capabilities: Basic Install categories: Integration & Delivery containerImage: quay.io/kuadrant/kuadrant-operator:latest - createdAt: "2024-11-25T09:30:08Z" + createdAt: "2024-11-26T11:04:32Z" description: A Kubernetes Operator to manage the lifecycle of the Kuadrant system operators.operatorframework.io/builder: operator-sdk-v1.32.0 operators.operatorframework.io/project_layout: go.kubebuilder.io/v3 diff --git a/bundle/manifests/kuadrant.io_authpolicies.yaml b/bundle/manifests/kuadrant.io_authpolicies.yaml index 2d67baec7..7044b036c 100644 --- a/bundle/manifests/kuadrant.io_authpolicies.yaml +++ b/bundle/manifests/kuadrant.io_authpolicies.yaml @@ -6888,6 +6888,15 @@ spec: || has(self.rules)))' - message: Explicit overrides and explicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.defaults))' + - message: At least one spec.rules.authentication must be defined + rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.rules) + && size(self.rules.authentication) > 0 : true' + - message: At least one spec.overrides.rules.authentication must be defined + rule: 'has(self.overrides) ? has(self.overrides.rules) && size(self.overrides.rules.authentication) + > 0 : true' + - message: At least one spec.defaults.rules.authentication must be defined + rule: 'has(self.defaults) ? has(self.defaults.rules) && size(self.defaults.rules.authentication) + > 0 : true' status: properties: conditions: diff --git a/charts/kuadrant-operator/templates/manifests.yaml b/charts/kuadrant-operator/templates/manifests.yaml index 6cb653ddc..434b09a88 100644 --- a/charts/kuadrant-operator/templates/manifests.yaml +++ b/charts/kuadrant-operator/templates/manifests.yaml @@ -6888,6 +6888,15 @@ spec: || has(self.rules)))' - message: Explicit overrides and explicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.defaults))' + - message: At least one spec.rules.authentication must be defined + rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.rules) + && size(self.rules.authentication) > 0 : true' + - message: At least one spec.overrides.rules.authentication must be defined + rule: 'has(self.overrides) ? has(self.overrides.rules) && size(self.overrides.rules.authentication) + > 0 : true' + - message: At least one spec.defaults.rules.authentication must be defined + rule: 'has(self.defaults) ? has(self.defaults.rules) && size(self.defaults.rules.authentication) + > 0 : true' status: properties: conditions: diff --git a/config/crd/bases/kuadrant.io_authpolicies.yaml b/config/crd/bases/kuadrant.io_authpolicies.yaml index 354999897..06a41a5e9 100644 --- a/config/crd/bases/kuadrant.io_authpolicies.yaml +++ b/config/crd/bases/kuadrant.io_authpolicies.yaml @@ -6887,6 +6887,15 @@ spec: || has(self.rules)))' - message: Explicit overrides and explicit defaults are mutually exclusive rule: '!(has(self.overrides) && has(self.defaults))' + - message: At least one spec.rules.authentication must be defined + rule: '!(has(self.overrides) || has(self.defaults)) ? has(self.rules) + && size(self.rules.authentication) > 0 : true' + - message: At least one spec.overrides.rules.authentication must be defined + rule: 'has(self.overrides) ? has(self.overrides.rules) && size(self.overrides.rules.authentication) + > 0 : true' + - message: At least one spec.defaults.rules.authentication must be defined + rule: 'has(self.defaults) ? has(self.defaults.rules) && size(self.defaults.rules.authentication) + > 0 : true' status: properties: conditions: diff --git a/tests/common/authpolicy/authpolicy_controller_test.go b/tests/common/authpolicy/authpolicy_controller_test.go index eeb7b4817..88fdc1862 100644 --- a/tests/common/authpolicy/authpolicy_controller_test.go +++ b/tests/common/authpolicy/authpolicy_controller_test.go @@ -481,18 +481,6 @@ var _ = Describe("AuthPolicy controller", func() { authConfigSpecAsJSON, _ := json.Marshal(authConfig.Spec) Expect(string(authConfigSpecAsJSON)).To(Equal(fmt.Sprintf(`{"hosts":["%s"],"patterns":{"authz-and-rl-required":[{"selector":"source.ip","operator":"neq","value":"192.168.0.10"}],"internal-source":[{"selector":"source.ip","operator":"matches","value":"192\\.168\\..*"}]},"authentication":{"jwt":{"when":[{"selector":"filter_metadata.envoy\\.filters\\.http\\.jwt_authn|verified_jwt","operator":"neq"}],"credentials":{},"plain":{"selector":"filter_metadata.envoy\\.filters\\.http\\.jwt_authn|verified_jwt"}}},"metadata":{"user-groups":{"when":[{"selector":"auth.identity.admin","operator":"neq","value":"true"}],"http":{"url":"http://user-groups/username={auth.identity.username}","method":"GET","contentType":"application/x-www-form-urlencoded","credentials":{}}}},"authorization":{"admin-or-privileged":{"when":[{"patternRef":"authz-and-rl-required"}],"patternMatching":{"patterns":[{"any":[{"selector":"auth.identity.admin","operator":"eq","value":"true"},{"selector":"auth.metadata.user-groups","operator":"incl","value":"privileged"}]}]}}},"response":{"unauthenticated":{"message":{"value":"Missing verified JWT injected by the gateway"}},"unauthorized":{"message":{"value":"User must be admin or member of privileged group"}},"success":{"headers":{"x-username":{"when":[{"selector":"request.headers.x-propagate-username.@case:lower","operator":"matches","value":"1|yes|true"}],"plain":{"value":null,"selector":"auth.identity.username"}}},"dynamicMetadata":{"x-auth-data":{"when":[{"patternRef":"authz-and-rl-required"}],"json":{"properties":{"groups":{"value":null,"selector":"auth.metadata.user-groups"},"username":{"value":null,"selector":"auth.identity.username"}}}}}}},"callbacks":{"unauthorized-attempt":{"when":[{"patternRef":"authz-and-rl-required"},{"selector":"auth.authorization.admin-or-privileged","operator":"neq","value":"true"}],"http":{"url":"http://events/unauthorized","method":"POST","body":{"value":null,"selector":"\\{\"identity\":{auth.identity},\"request-id\":{request.id}\\}"},"contentType":"application/json","credentials":{}}}}}`, authConfig.GetName()))) }, testTimeOut) - - It("Succeeds when AuthScheme is not defined", func(ctx SpecContext) { - policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { - policy.Spec.Proper().AuthScheme = nil - }) - - err := k8sClient.Create(ctx, policy) - logf.Log.V(1).Info("Creating AuthPolicy", "key", client.ObjectKeyFromObject(policy).String(), "error", err) - Expect(err).ToNot(HaveOccurred()) - - Eventually(tests.IsAuthPolicyAcceptedAndEnforced(ctx, testClient(), policy)).WithContext(ctx).Should(BeTrue()) - }, testTimeOut) }) Context("Complex HTTPRoute with multiple rules and hostnames", func() { @@ -935,6 +923,19 @@ var _ = Describe("AuthPolicy CEL Validations", func() { Name: "my-target", }, }, + AuthPolicySpecProper: kuadrantv1.AuthPolicySpecProper{ + AuthScheme: &kuadrantv1.AuthSchemeSpec{ + Authentication: map[string]kuadrantv1.MergeableAuthenticationSpec{ + "anonymous": { + AuthenticationSpec: authorinov1beta3.AuthenticationSpec{ + AuthenticationMethodSpec: authorinov1beta3.AuthenticationMethodSpec{ + AnonymousAccess: &authorinov1beta3.AnonymousAccessSpec{}, + }, + }, + }, + }, + }, + }, }, } @@ -979,6 +980,99 @@ var _ = Describe("AuthPolicy CEL Validations", func() { }) }) + Context("Rules missing from configuration", func() { + It("Missing rules object", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = nil + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.rules.authentication must be defined")).To(BeTrue()) + }) + + It("Empty rules object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = &kuadrantv1.AuthSchemeSpec{} + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.rules.authentication must be defined")).To(BeTrue()) + }) + + It("Empty rules.authentication object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = &kuadrantv1.AuthSchemeSpec{ + Authentication: map[string]kuadrantv1.MergeableAuthenticationSpec{}, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.rules.authentication must be defined")).To(BeTrue()) + }) + + It("Missing defaults.rules.authentication object", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = nil + policy.Spec.Defaults = &kuadrantv1.MergeableAuthPolicySpec{ + AuthPolicySpecProper: kuadrantv1.AuthPolicySpecProper{ + AuthScheme: nil, + }, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.defaults.rules.authentication must be defined")).To(BeTrue()) + }) + + It("Empty defaults.rules.authentication object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = nil + policy.Spec.Defaults = &kuadrantv1.MergeableAuthPolicySpec{ + AuthPolicySpecProper: kuadrantv1.AuthPolicySpecProper{ + AuthScheme: &kuadrantv1.AuthSchemeSpec{ + Authentication: map[string]kuadrantv1.MergeableAuthenticationSpec{}, + }, + }, + } + + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.defaults.rules.authentication must be defined")).To(BeTrue()) + }) + + It("Missing overrides.rules.authentication object", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = nil + policy.Spec.Overrides = &kuadrantv1.MergeableAuthPolicySpec{ + AuthPolicySpecProper: kuadrantv1.AuthPolicySpecProper{ + AuthScheme: nil, + }, + } + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.overrides.rules.authentication must be defined")).To(BeTrue()) + }) + + It("Empty overrides.rules.authentication object created", func(ctx SpecContext) { + policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) { + policy.Spec.AuthScheme = nil + policy.Spec.Overrides = &kuadrantv1.MergeableAuthPolicySpec{ + AuthPolicySpecProper: kuadrantv1.AuthPolicySpecProper{ + AuthScheme: &kuadrantv1.AuthSchemeSpec{ + Authentication: map[string]kuadrantv1.MergeableAuthenticationSpec{}, + }, + }, + } + + }) + err := k8sClient.Create(ctx, policy) + Expect(err).To(Not(BeNil())) + Expect(strings.Contains(err.Error(), "At least one spec.overrides.rules.authentication must be defined")).To(BeTrue()) + }) + }) + Context("Defaults mutual exclusivity validation", func() { It("Valid when only implicit defaults are used", func(ctx SpecContext) { policy := policyFactory(func(policy *kuadrantv1.AuthPolicy) {