From 01cc42e78af19db4ea1ed936381a3ee686bd627c Mon Sep 17 00:00:00 2001 From: shuting <shuting@nirmata.com> Date: Wed, 4 Sep 2024 19:26:24 +0800 Subject: [PATCH] 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> --- pkg/policy/generate/validate.go | 22 +++++++++------- pkg/policy/generate/validate_test.go | 4 +-- pkg/policy/mutate/validate.go | 2 +- pkg/policy/validate/validate.go | 2 +- pkg/policy/validate/validate_test.go | 26 +++++++++---------- pkg/validation/policy/actions.go | 20 ++++++++++---- .../chainsaw-test.yaml | 2 ++ .../permissions.yaml | 19 ++++++++++++++ .../permissions.yaml | 8 ++++++ .../permissions.yaml | 1 + .../clusterpolicy/cloneList/permissions.yaml | 21 +++++++++++++++ .../chainsaw-step-01-apply-1-1.yaml | 19 +++++++++++--- .../target-namespace-scope/chainsaw-test.yaml | 2 ++ .../target-namespace-scope/permissions.yaml | 25 ++++++++++++++++++ 14 files changed, 138 insertions(+), 35 deletions(-) create mode 100644 test/conformance/chainsaw/events/clusterpolicy/generate-events-upon-fail-generation/permissions.yaml create mode 100644 test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/permissions.yaml diff --git a/pkg/policy/generate/validate.go b/pkg/policy/generate/validate.go index e95149bc37..b476e66dde 100644 --- 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) diff --git a/pkg/policy/generate/validate_test.go b/pkg/policy/generate/validate_test.go index e5f6c5a345..7213360d9c 100644 --- 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) } } diff --git a/pkg/policy/mutate/validate.go b/pkg/policy/mutate/validate.go index 12b2299868..fedc0dd187 100644 --- 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") diff --git a/pkg/policy/validate/validate.go b/pkg/policy/validate/validate.go index 277e1e48f9..8781c97c9a 100644 --- 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 } diff --git a/pkg/policy/validate/validate_test.go b/pkg/policy/validate/validate_test.go index 329263ba6e..53fabc3158 100644 --- 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) } diff --git a/pkg/validation/policy/actions.go b/pkg/validation/policy/actions.go index 6dba3fb157..a4db7905db 100644 --- 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...) diff --git 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 index fce5084c92..9cd3fe65c8 100755 --- 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 diff --git 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 new file mode 100644 index 0000000000..f0f0c6e9ab --- /dev/null +++ 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 \ No newline at end of file diff --git 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 index 36bb686b73..2518b4d0e5 100644 --- 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 diff --git 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 index 1db18bfeaa..9441af52f6 100644 --- 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 diff --git a/test/conformance/chainsaw/generate/validation/clusterpolicy/cloneList/permissions.yaml b/test/conformance/chainsaw/generate/validation/clusterpolicy/cloneList/permissions.yaml index a3a5fc60c4..5e8d7db4e0 100644 --- 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 \ No newline at end of file diff --git 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 index da013ecf07..859416a4ba 100755 --- 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 \ No newline at end of file diff --git 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 index 75b9ca0345..a90972d137 100755 --- 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 diff --git a/test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/permissions.yaml b/test/conformance/chainsaw/generate/validation/policy/target-namespace-scope/permissions.yaml new file mode 100644 index 0000000000..d4a08cd5fa --- /dev/null +++ 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 \ No newline at end of file