From 14ab6b72a29c7a4191b9eeaa949d449762e20251 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Wed, 13 Sep 2023 16:04:39 +0200 Subject: [PATCH] fix: Kyverno variable substitution might not work correctly if the top level variable key contains dots (#8377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Kyverno variable substitution might not work correctly if the top level variable key contains dots Signed-off-by: Charles-Edouard Brétéché * fix Signed-off-by: Charles-Edouard Brétéché --------- Signed-off-by: Charles-Edouard Brétéché --- .../processor/policy_processor.go | 70 ++++++++++++++++++- .../deny-modify-platform-label.yaml | 30 ++++++++ .../kyverno-test.yaml | 25 +++++++ .../resource.yaml | 35 ++++++++++ .../variables.yaml | 11 +++ .../deny-modify-platform-label.yaml | 27 +++++++ .../kyverno-test.yaml | 13 ++++ .../resource.yaml | 11 +++ .../variables.yaml | 6 ++ 9 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 test/cli/test/deny-modify-platform-label-2/deny-modify-platform-label.yaml create mode 100644 test/cli/test/deny-modify-platform-label-2/kyverno-test.yaml create mode 100644 test/cli/test/deny-modify-platform-label-2/resource.yaml create mode 100644 test/cli/test/deny-modify-platform-label-2/variables.yaml create mode 100644 test/cli/test/deny-modify-platform-label-3/deny-modify-platform-label.yaml create mode 100644 test/cli/test/deny-modify-platform-label-3/kyverno-test.yaml create mode 100644 test/cli/test/deny-modify-platform-label-3/resource.yaml create mode 100644 test/cli/test/deny-modify-platform-label-3/variables.yaml diff --git a/cmd/cli/kubectl-kyverno/processor/policy_processor.go b/cmd/cli/kubectl-kyverno/processor/policy_processor.go index d19d1c4f40..eb8d36d5a6 100644 --- a/cmd/cli/kubectl-kyverno/processor/policy_processor.go +++ b/cmd/cli/kubectl-kyverno/processor/policy_processor.go @@ -204,6 +204,7 @@ func (p *PolicyProcessor) makePolicyContext( } resourceValues = vals } + // TODO: this is kind of buggy, we should read that from the json context switch resourceValues["request.operation"] { case "DELETE": operation = kyvernov1.Delete @@ -219,9 +220,13 @@ func (p *PolicyProcessor) makePolicyContext( ) if err != nil { log.Log.Error(err, "failed to create policy context") + return nil, fmt.Errorf("failed to create policy context (%w)", err) } if operation == kyvernov1.Update { policyContext = policyContext.WithOldResource(resource) + if err := policyContext.JSONContext().AddOldResource(resource.Object); err != nil { + return nil, fmt.Errorf("failed to update old resource in json context (%w)", err) + } } policyContext = policyContext. WithPolicy(policy). @@ -230,7 +235,70 @@ func (p *PolicyProcessor) makePolicyContext( for key, value := range resourceValues { err = policyContext.JSONContext().AddVariable(key, value) if err != nil { - log.Log.Error(err, "failed to add variable to context") + log.Log.Error(err, "failed to add variable to context", "key", key, "value", value) + return nil, fmt.Errorf("failed to add variable to context %s (%w)", key, err) + } + } + // we need to get the resources back from the context to account for injected variables + switch operation { + case kyvernov1.Create: + ret, err := policyContext.JSONContext().Query("request.object") + if err != nil { + return nil, err + } + if ret == nil { + policyContext = policyContext.WithNewResource(unstructured.Unstructured{}) + } else { + object, ok := ret.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("the object retrieved from the json context is not valid") + } + policyContext = policyContext.WithNewResource(unstructured.Unstructured{Object: object}) + } + case kyvernov1.Update: + { + ret, err := policyContext.JSONContext().Query("request.object") + if err != nil { + return nil, err + } + if ret == nil { + policyContext = policyContext.WithNewResource(unstructured.Unstructured{}) + } else { + object, ok := ret.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("the object retrieved from the json context is not valid") + } + policyContext = policyContext.WithNewResource(unstructured.Unstructured{Object: object}) + } + } + { + ret, err := policyContext.JSONContext().Query("request.oldObject") + if err != nil { + return nil, err + } + if ret == nil { + policyContext = policyContext.WithOldResource(unstructured.Unstructured{}) + } else { + object, ok := ret.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("the object retrieved from the json context is not valid") + } + policyContext = policyContext.WithOldResource(unstructured.Unstructured{Object: object}) + } + } + case kyvernov1.Delete: + ret, err := policyContext.JSONContext().Query("request.oldObject") + if err != nil { + return nil, err + } + if ret == nil { + policyContext = policyContext.WithOldResource(unstructured.Unstructured{}) + } else { + object, ok := ret.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("the object retrieved from the json context is not valid") + } + policyContext = policyContext.WithOldResource(unstructured.Unstructured{Object: object}) } } return policyContext, nil diff --git a/test/cli/test/deny-modify-platform-label-2/deny-modify-platform-label.yaml b/test/cli/test/deny-modify-platform-label-2/deny-modify-platform-label.yaml new file mode 100644 index 0000000000..4b5877ef3b --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-2/deny-modify-platform-label.yaml @@ -0,0 +1,30 @@ +apiVersion: kyverno.io/v1 +kind: ClusterPolicy +metadata: + name: deny-modify-platform-label + annotations: + policies.kyverno.io/title: Deny Modification of platform owned roles + policies.kyverno.io/category: Hardening + policies.kyverno.io/severity: medium + policies.kyverno.io/subject: Role + policies.kyverno.io/description: >- + Restrict modification of platform owned roles to admins only +spec: + validationFailureAction: audit + background: false + rules: + - name: deny-modify-platform-role + match: + any: + - resources: + kinds: + - Role + preconditions: + any: + - key: '{{ request.object.metadata.annotations."hpedevops.net/platform" || "" }}' + operator: Equals + value: 'true' + validate: + message: >- + Roles owned by platform team (ones with label hpedevops.net/platform=true) should not be modified by non-admin users. + deny: {} \ No newline at end of file diff --git a/test/cli/test/deny-modify-platform-label-2/kyverno-test.yaml b/test/cli/test/deny-modify-platform-label-2/kyverno-test.yaml new file mode 100644 index 0000000000..abb7a17412 --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-2/kyverno-test.yaml @@ -0,0 +1,25 @@ +name: deny-modify-platform-label-2 +policies: +- deny-modify-platform-label.yaml +resources: +- resource.yaml +variables: variables.yaml +results: +- kind: Role + policy: deny-modify-platform-label + resources: + - my-role-with-platform + result: fail + rule: deny-modify-platform-role +- kind: Role + policy: deny-modify-platform-label + resources: + - my-role-without-platform + result: skip + rule: deny-modify-platform-role +- kind: Role + policy: deny-modify-platform-label + resources: + - my-role-with-platform-false + result: skip + rule: deny-modify-platform-role diff --git a/test/cli/test/deny-modify-platform-label-2/resource.yaml b/test/cli/test/deny-modify-platform-label-2/resource.yaml new file mode 100644 index 0000000000..23fbba3a29 --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-2/resource.yaml @@ -0,0 +1,35 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: my-role-with-platform +rules: + - apiGroups: + - "" + resources: + - services + verbs: + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: my-role-without-platform +rules: + - apiGroups: + - "" + resources: + - services + verbs: + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: my-role-with-platform-false +rules: + - apiGroups: + - "" + resources: + - services + verbs: + - watch diff --git a/test/cli/test/deny-modify-platform-label-2/variables.yaml b/test/cli/test/deny-modify-platform-label-2/variables.yaml new file mode 100644 index 0000000000..b60d25e6f5 --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-2/variables.yaml @@ -0,0 +1,11 @@ +policies: + - name: deny-modify-platform-label + resources: + - name: my-role-with-platform + values: + request.object.metadata.annotations."hpedevops.net/platform": 'true' + - name: deny-modify-platform-label + resources: + - name: my-role-with-platform-false + values: + request.object.metadata.annotations."hpedevops.net/platform": 'false' diff --git a/test/cli/test/deny-modify-platform-label-3/deny-modify-platform-label.yaml b/test/cli/test/deny-modify-platform-label-3/deny-modify-platform-label.yaml new file mode 100644 index 0000000000..dec3e05ac6 --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-3/deny-modify-platform-label.yaml @@ -0,0 +1,27 @@ +apiVersion: kyverno.io/v1 +kind: ClusterPolicy +metadata: + name: deny-modify-platform-label + annotations: + policies.kyverno.io/title: Deny Modification of platform owned roles + policies.kyverno.io/category: Hardening + policies.kyverno.io/severity: medium + policies.kyverno.io/subject: Role + policies.kyverno.io/description: >- + Restrict modification of platform owned roles to admins only +spec: + validationFailureAction: audit + background: false + rules: + - name: deny-modify-platform-role + match: + any: + - resources: + kinds: + - Role + annotations: + hpedevops.net/platform: 'true' + validate: + message: >- + Roles owned by platform team (ones with label hpedevops.net/platform=true) should not be modified by non-admin users. + deny: {} \ No newline at end of file diff --git a/test/cli/test/deny-modify-platform-label-3/kyverno-test.yaml b/test/cli/test/deny-modify-platform-label-3/kyverno-test.yaml new file mode 100644 index 0000000000..338735116e --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-3/kyverno-test.yaml @@ -0,0 +1,13 @@ +name: deny-modify-platform-label-2 +policies: +- deny-modify-platform-label.yaml +resources: +- resource.yaml +variables: variables.yaml +results: +- kind: Role + policy: deny-modify-platform-label + resources: + - my-role-with-platform + result: fail + rule: deny-modify-platform-role diff --git a/test/cli/test/deny-modify-platform-label-3/resource.yaml b/test/cli/test/deny-modify-platform-label-3/resource.yaml new file mode 100644 index 0000000000..32610f7be3 --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-3/resource.yaml @@ -0,0 +1,11 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: my-role-with-platform +rules: + - apiGroups: + - "" + resources: + - services + verbs: + - watch diff --git a/test/cli/test/deny-modify-platform-label-3/variables.yaml b/test/cli/test/deny-modify-platform-label-3/variables.yaml new file mode 100644 index 0000000000..13ac84cb8c --- /dev/null +++ b/test/cli/test/deny-modify-platform-label-3/variables.yaml @@ -0,0 +1,6 @@ +policies: + - name: deny-modify-platform-label + resources: + - name: my-role-with-platform + values: + request.object.metadata.annotations."hpedevops.net/platform": 'true'