Disallow empty AuthPolicies

Signed-off-by: Guilherme Cassolato <[email protected]>
Kuadrant · Nov 26, 2024 · 2784176 · 2784176
1 parent 1b336be
commit 2784176
Show file tree

Hide file tree

Showing 6 changed files with 137 additions and 13 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {