fix: add auth check to the admission controller for generate policies (#10963)

* fix: add auth check to the admission controller for generate policies Signed-off-by: ShutingZhao <shuting@nirmata.com> * fix: enable auth check if sync=true Signed-off-by: ShutingZhao <shuting@nirmata.com> * fix: restict to list/get permissions Signed-off-by: ShutingZhao <shuting@nirmata.com> * fix: aggregate clusterrole to admission controller Signed-off-by: ShutingZhao <shuting@nirmata.com> * fix: aggregate clusterrole to admission controller Signed-off-by: ShutingZhao <shuting@nirmata.com> * fix: aggregate clusterrole to admission controller Signed-off-by: ShutingZhao <shuting@nirmata.com> * fix: aggregate clusterrole to admission controller Signed-off-by: ShutingZhao <shuting@nirmata.com> --------- Signed-off-by: ShutingZhao <shuting@nirmata.com>
2025-04-15 00:36:28 +00:00 · 2024-09-04 19:26:24 +08:00 · 2024-09-04 19:26:24 +08:00 · 01cc42e78a
commit 01cc42e78a
parent c0d6eaddb3
14 changed files with 138 additions and 35 deletions
--- a/pkg/policy/generate/validate.go
+++ b/pkg/policy/generate/validate.go
@ -36,7 +36,7 @@ func NewGenerateFactory(client dclient.Interface, rule kyvernov1.Generation, use
 }

 // Validate validates the 'generate' rule
-func (g *Generate) Validate(ctx context.Context) (warnings []string, path string, err error) {
+func (g *Generate) Validate(ctx context.Context, verbs []string) (warnings []string, path string, err error) {
 	rule := g.rule
 	if rule.CloneList.Selector != nil {
 		if wildcard.ContainsWildcard(rule.CloneList.Selector.String()) {
@ -59,40 +59,42 @@ func (g *Generate) Validate(ctx context.Context) (warnings []string, path string
 	// If kind and namespace contain variables, then we cannot resolve then so we skip the processing
 	if rule.ForEachGeneration != nil {
 		for _, forEach := range rule.ForEachGeneration {
-			if err := g.validateAuth(ctx, forEach.GeneratePattern); err != nil {
+			if err := g.validateAuth(ctx, verbs, forEach.GeneratePattern); err != nil {
 				return nil, "foreach", err
 			}
 		}
 	} else {
-		if err := g.validateAuth(ctx, rule.GeneratePattern); err != nil {
+		if err := g.validateAuth(ctx, verbs, rule.GeneratePattern); err != nil {
 			return nil, "", err
 		}
 	}
 	return nil, "", nil
 }

-func (g *Generate) validateAuth(ctx context.Context, generate kyvernov1.GeneratePattern) error {
+func (g *Generate) validateAuth(ctx context.Context, verbs []string, generate kyvernov1.GeneratePattern) error {
 	if len(generate.CloneList.Kinds) != 0 {
 		for _, kind := range generate.CloneList.Kinds {
 			gvk, sub := parseCloneKind(kind)
-			return g.canIGenerate(ctx, gvk, generate.Namespace, sub)
+			return g.canIGenerate(ctx, verbs, gvk, generate.Namespace, sub)
 		}
 	} else {
 		k, sub := kubeutils.SplitSubresource(generate.Kind)
-		return g.canIGenerate(ctx, strings.Join([]string{generate.APIVersion, k}, "/"), generate.Namespace, sub)
+		return g.canIGenerate(ctx, verbs, strings.Join([]string{generate.APIVersion, k}, "/"), generate.Namespace, sub)
 	}
 	return nil
 }

-func (g *Generate) canIGenerate(ctx context.Context, gvk, namespace, subresource string) error {
+func (g *Generate) canIGenerate(ctx context.Context, verbs []string, gvk, namespace, subresource string) error {
 	if regex.IsVariable(gvk) {
 		g.log.V(2).Info("resource Kind uses variables; skipping authorization checks.")
 		return nil
 	}

-	verbs := []string{"get", "create"}
-	if g.rule.Synchronize {
-		verbs = []string{"get", "create", "update", "delete"}
+	if verbs == nil {
+		verbs = []string{"get", "create"}
+		if g.rule.Synchronize {
+			verbs = []string{"get", "create", "update", "delete"}
+		}
 	}

 	ok, msg, err := g.authChecker.CanI(ctx, verbs, gvk, namespace, "", subresource)
--- a/pkg/policy/generate/validate_test.go
+++ b/pkg/policy/generate/validate_test.go
@ -36,7 +36,7 @@ func Test_Validate_Generate_HasAnchors(t *testing.T) {
 	err = json.Unmarshal(rawGenerate, &genRule)
 	assert.NilError(t, err)
 	checker := NewFakeGenerate(genRule)
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}

@ -53,7 +53,7 @@ func Test_Validate_Generate_HasAnchors(t *testing.T) {
 	err = json.Unmarshal(rawGenerate, &genRule)
 	assert.NilError(t, err)
 	checker = NewFakeGenerate(genRule)
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
--- a/pkg/policy/mutate/validate.go
+++ b/pkg/policy/mutate/validate.go
@ -37,7 +37,7 @@ func NewMutateFactory(m kyvernov1.Mutation, client dclient.Interface, mock bool,
 }

 // Validate validates the 'mutate' rule
-func (m *Mutate) Validate(ctx context.Context) (warnings []string, path string, err error) {
+func (m *Mutate) Validate(ctx context.Context, _ []string) (warnings []string, path string, err error) {
 	if m.hasForEach() {
 		if m.hasPatchStrategicMerge() || m.hasPatchesJSON6902() {
 			return nil, "foreach", fmt.Errorf("only one of `foreach`, `patchStrategicMerge`, or `patchesJson6902` is allowed")
--- a/pkg/policy/validate/validate.go
+++ b/pkg/policy/validate/validate.go
@ -46,7 +46,7 @@ func NewMockValidateFactory(rule *kyvernov1.Rule) *Validate {
 }

 // Validate validates the 'validate' rule
-func (v *Validate) Validate(ctx context.Context) (warnings []string, path string, err error) {
+func (v *Validate) Validate(ctx context.Context, _ []string) (warnings []string, path string, err error) {
 	if err := v.validateElements(); err != nil {
 		return nil, "", err
 	}
--- a/pkg/policy/validate/validate_test.go
+++ b/pkg/policy/validate/validate_test.go
@ -18,7 +18,7 @@ func Test_Validate_OverlayPattern_Empty(t *testing.T) {
 	assert.NilError(t, err)

 	checker := NewMockValidateFactory(&rule)
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
@ -32,7 +32,7 @@ func Test_Validate_OverlayPattern_Nil_PatternAnypattern(t *testing.T) {
 	err := json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
@ -70,7 +70,7 @@ func Test_Validate_OverlayPattern_Exist_PatternAnypattern(t *testing.T) {
 	err := json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
@ -108,7 +108,7 @@ func Test_Validate_OverlayPattern_Valid(t *testing.T) {
 	err := json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.NilError(t, err)
 	}
 }
@ -141,7 +141,7 @@ func Test_Validate_ExistingAnchor_AnchorOnMap(t *testing.T) {
 	err := json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
@ -171,7 +171,7 @@ func Test_Validate_ExistingAnchor_AnchorOnString(t *testing.T) {
 	err := json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
@ -204,7 +204,7 @@ func Test_Validate_ExistingAnchor_Valid(t *testing.T) {
 	err = json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 	rawValidation = []byte(`
@ -229,7 +229,7 @@ func Test_Validate_ExistingAnchor_Valid(t *testing.T) {
 	err = json.Unmarshal(rawValidation, &validation)
 	assert.NilError(t, err)
 	checker = NewMockValidateFactory(&kyverno.Rule{Validation: validation})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}

@ -270,7 +270,7 @@ func Test_Validate_Validate_ValidAnchor(t *testing.T) {
 	assert.NilError(t, err)

 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validate})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.NilError(t, err)
 	}

@ -292,7 +292,7 @@ func Test_Validate_Validate_ValidAnchor(t *testing.T) {
 	assert.NilError(t, err)

 	checker = NewMockValidateFactory(&kyverno.Rule{Validation: validate})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.NilError(t, err)
 	}
 }
@ -319,7 +319,7 @@ func Test_Validate_Validate_Mismatched(t *testing.T) {
 	err := json.Unmarshal(rawValidate, &validate)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validate})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}
 }
@ -349,7 +349,7 @@ func Test_Validate_Validate_Unsupported(t *testing.T) {
 	err = json.Unmarshal(rawValidate, &validate)
 	assert.NilError(t, err)
 	checker := NewMockValidateFactory(&kyverno.Rule{Validation: validate})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}

@ -375,7 +375,7 @@ func Test_Validate_Validate_Unsupported(t *testing.T) {
 	assert.NilError(t, err)

 	checker = NewMockValidateFactory(&kyverno.Rule{Validation: validate})
-	if _, _, err := checker.Validate(context.TODO()); err != nil {
+	if _, _, err := checker.Validate(context.TODO(), nil); err != nil {
 		assert.Assert(t, err != nil)
 	}

--- a/pkg/validation/policy/actions.go
+++ b/pkg/validation/policy/actions.go
@ -8,6 +8,7 @@ import (
 	kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1"
 	authChecker "github.com/kyverno/kyverno/pkg/auth/checker"
 	"github.com/kyverno/kyverno/pkg/clients/dclient"
+	"github.com/kyverno/kyverno/pkg/config"
 	"github.com/kyverno/kyverno/pkg/logging"
 	"github.com/kyverno/kyverno/pkg/policy/generate"
 	"github.com/kyverno/kyverno/pkg/policy/mutate"
@ -18,7 +19,7 @@ import (

 // Validation provides methods to validate a rule
 type Validation interface {
-	Validate(ctx context.Context) (warnings []string, path string, err error)
+	Validate(ctx context.Context, verbs []string) (warnings []string, path string, err error)
 }

 // validateAction performs validation on the rule actions
@ -35,7 +36,7 @@ func validateActions(idx int, rule *kyvernov1.Rule, client dclient.Interface, mo
 	// Mutate
 	if rule.HasMutate() {
 		checker = mutate.NewMutateFactory(rule.Mutation, client, mock, backgroundSA)
-		if w, path, err := checker.Validate(context.TODO()); err != nil {
+		if w, path, err := checker.Validate(context.TODO(), nil); err != nil {
 			return nil, fmt.Errorf("path: spec.rules[%d].mutate.%s.: %v", idx, path, err)
 		} else if w != nil {
 			warnings = append(warnings, w...)
@ -45,7 +46,7 @@ func validateActions(idx int, rule *kyvernov1.Rule, client dclient.Interface, mo
 	// Validate
 	if rule.HasValidate() {
 		checker = validate.NewValidateFactory(rule, client, mock, reportsSA)
-		if w, path, err := checker.Validate(context.TODO()); err != nil {
+		if w, path, err := checker.Validate(context.TODO(), nil); err != nil {
 			return nil, fmt.Errorf("path: spec.rules[%d].validate.%s.: %v", idx, path, err)
 		} else if w != nil {
 			warnings = append(warnings, w...)
@ -70,14 +71,23 @@ func validateActions(idx int, rule *kyvernov1.Rule, client dclient.Interface, mo
 		// this need to modified to use different implementation for online and offline mode
 		if mock {
 			checker = generate.NewFakeGenerate(rule.Generation)
-			if w, path, err := checker.Validate(context.TODO()); err != nil {
+			if w, path, err := checker.Validate(context.TODO(), nil); err != nil {
 				return nil, fmt.Errorf("path: spec.rules[%d].generate.%s.: %v", idx, path, err)
 			} else if warnings != nil {
 				warnings = append(warnings, w...)
 			}
 		} else {
+			if rule.Generation.Synchronize {
+				admissionSA := fmt.Sprintf("system:serviceaccount:%s:%s", config.KyvernoNamespace(), config.KyvernoServiceAccountName())
+				checker = generate.NewGenerateFactory(client, rule.Generation, admissionSA, logging.GlobalLogger())
+				if w, path, err := checker.Validate(context.TODO(), []string{"list", "get"}); err != nil {
+					return nil, fmt.Errorf("path: spec.rules[%d].generate.%s.: %v", idx, path, err)
+				} else if warnings != nil {
+					warnings = append(warnings, w...)
+				}
+			}
 			checker = generate.NewGenerateFactory(client, rule.Generation, backgroundSA, logging.GlobalLogger())
-			if w, path, err := checker.Validate(context.TODO()); err != nil {
+			if w, path, err := checker.Validate(context.TODO(), nil); err != nil {
 				return nil, fmt.Errorf("path: spec.rules[%d].generate.%s.: %v", idx, path, err)
 			} else if warnings != nil {
 				warnings = append(warnings, w...)
--- a/test/conformance/chainsaw/events/clusterpolicy/generate-events-upon-fail-generation/chainsaw-test.yaml
+++ b/test/conformance/chainsaw/events/clusterpolicy/generate-events-upon-fail-generation/chainsaw-test.yaml
@ -9,6 +9,8 @@ spec:
    try:
    - apply:
        file: crd.yaml
+    - apply:
+        file: permissions.yaml
    - assert:
        file: crd-assert.yaml
  - name: step-02
--- a/test/conformance/chainsaw/events/clusterpolicy/generate-events-upon-fail-generation/permissions.yaml
+++ b/test/conformance/chainsaw/events/clusterpolicy/generate-events-upon-fail-generation/permissions.yaml
@ -0,0 +1,19 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: kyverno:rbac
+  labels:
+    rbac.kyverno.io/aggregate-to-background-controller: "true"
+    rbac.kyverno.io/aggregate-to-admission-controller: "true"
+rules:
+- apiGroups:
+  - iam.aws.crossplane.io
+  resources:
+  - roles
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - delete
--- a/test/conformance/chainsaw/generate/clusterpolicy/cornercases/cpol-data-trigger-not-present/permissions.yaml
+++ b/test/conformance/chainsaw/generate/clusterpolicy/cornercases/cpol-data-trigger-not-present/permissions.yaml
@ -4,6 +4,7 @@ metadata:
  name: kyverno:rbac
  labels:
    rbac.kyverno.io/aggregate-to-background-controller: "true"
+    rbac.kyverno.io/aggregate-to-admission-controller: "true"
 rules:
 - apiGroups:
  - rbac.authorization.k8s.io
@ -17,3 +18,10 @@ rules:
  - create
  - update
  - delete
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - rolebindings
+  verbs:
+  - get
+  - list
--- a/test/conformance/chainsaw/generate/clusterpolicy/cornercases/data-role-and-rolebinding/permissions.yaml
+++ b/test/conformance/chainsaw/generate/clusterpolicy/cornercases/data-role-and-rolebinding/permissions.yaml
@ -4,6 +4,7 @@ metadata:
  name: kyverno:rbac
  labels:
    rbac.kyverno.io/aggregate-to-background-controller: "true"
+    rbac.kyverno.io/aggregate-to-admission-controller: "true"
 rules:
 - apiGroups:
  - rbac.authorization.k8s.io
--- a/test/conformance/chainsaw/generate/validation/clusterpolicy/cloneList/permissions.yaml
+++ b/test/conformance/chainsaw/generate/validation/clusterpolicy/cloneList/permissions.yaml
@ -17,4 +17,25 @@ rules:
  - watch
  - create
  - update
+  - delete
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  name: kyverno:rbac
+  labels:
+    rbac.kyverno.io/aggregate-to-background-controller: "true"
+    rbac.kyverno.io/aggregate-to-admission-controller: "true"
+rules:
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - clusterroles
+  - clusterrolebindings
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
  - delete
--- a/test/conformance/chainsaw/generate/validation/clusterpolicy/target-namespace-scope/chainsaw-step-01-apply-1-1.yaml
+++ b/test/conformance/chainsaw/generate/validation/clusterpolicy/target-namespace-scope/chainsaw-step-01-apply-1-1.yaml
@ -5,19 +5,32 @@ metadata:
    app.kubernetes.io/component: background-controller
    app.kubernetes.io/instance: kyverno
    app.kubernetes.io/part-of: kyverno
+    rbac.kyverno.io/aggregate-to-admission-controller: "true"
  name: kyverno:background-controller:manage-ns-crossplane-role
 rules:
 - apiGroups:
  - ""
-  - iam.aws.crossplane.io
  resources:
  - namespaces
-  - roles
  verbs:
  - create
  - update
  - delete
  - get
+- apiGroups:
+  - iam.aws.crossplane.io
+  resources:
+  - roles
+  verbs:
+  - list
+  - get
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - roles
+  verbs:
+  - list
+  - get
 - apiGroups:
  - kyverno.io
  resources:
@ -26,4 +39,4 @@ rules:
  - create
  - update
  - delete
-  - get
+  - get
--- a/test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/chainsaw-test.yaml
+++ b/test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/chainsaw-test.yaml
@ -7,6 +7,8 @@ spec:
  steps:
  - name: step-01
    try:
+    - apply:
+        file: permissions.yaml
    - apply:
        file: chainsaw-step-01-apply-1-1.yaml
  - name: step-02
--- a/test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/permissions.yaml
+++ b/test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/permissions.yaml
@ -0,0 +1,25 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole
+metadata:
+  labels:
+    rbac.kyverno.io/aggregate-to-admission-controller: "true"
+  name: kyverno:background-controller:manage-ns-crossplane-role
+rules:
+- apiGroups:
+  - ""
+  - iam.aws.crossplane.io
+  resources:
+  - configmaps
+  - roles
+  verbs:
+  - create
+  - update
+  - delete
+  - get
+- apiGroups:
+  - rbac.authorization.k8s.io
+  resources:
+  - roles
+  verbs:
+  - list
+  - get