From 5e634fd24012db2c482daed465943081c0c408c2 Mon Sep 17 00:00:00 2001 From: Mason Malone <651224+MasonM@users.noreply.github.com> Date: Wed, 1 Jan 2025 19:25:50 -0800 Subject: [PATCH] feat: add example ValidatingAdmissionPolicy to block param interpolation. Fixes #5114 Signed-off-by: Mason Malone <651224+MasonM@users.noreply.github.com> --- .devcontainer/pre-build.sh | 4 +- .../argo-dangerous-interpolation-vap.yaml | 41 +++++++++++ examples/validation_test.go | 2 +- test/e2e/argo_server_test.go | 72 +++++++++++++++++++ ...o-dangerous-interpolation-vap-binding.yaml | 12 ++++ .../argo-dangerous-interpolation-vap.yaml | 1 + 6 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 examples/argo-dangerous-interpolation-vap.yaml create mode 100644 test/e2e/functional/argo-dangerous-interpolation-vap-binding.yaml create mode 120000 test/e2e/functional/argo-dangerous-interpolation-vap.yaml diff --git a/.devcontainer/pre-build.sh b/.devcontainer/pre-build.sh index 16727647fa36..252dfa0fe909 100755 --- a/.devcontainer/pre-build.sh +++ b/.devcontainer/pre-build.sh @@ -3,11 +3,11 @@ set -eux # install kubernetes wget -q -O - https://raw.githubusercontent.com/k3d-io/k3d/main/install.sh | bash -k3d cluster get k3s-default || k3d cluster create --image rancher/k3s:v1.27.3-k3s1 --wait +k3d cluster get k3s-default || k3d cluster create --image rancher/k3s:v1.29.10-k3s1 --wait k3d kubeconfig merge --kubeconfig-merge-default # install kubectl -curl -LO https://dl.k8s.io/release/v1.27.3/bin/linux/$(go env GOARCH)/kubectl +curl -LO https://dl.k8s.io/release/v1.29.10/bin/linux/$(go env GOARCH)/kubectl chmod +x ./kubectl sudo mv ./kubectl /usr/local/bin/kubectl kubectl cluster-info diff --git a/examples/argo-dangerous-interpolation-vap.yaml b/examples/argo-dangerous-interpolation-vap.yaml new file mode 100644 index 000000000000..8d36eb904cbe --- /dev/null +++ b/examples/argo-dangerous-interpolation-vap.yaml @@ -0,0 +1,41 @@ +# This admission policy (https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/) +# can be used to block workflows that use string interpolation inside the fields "command", +# "args", or "source". +# +# Interpolating untrusted user input into this fields can lead to command +# injection vulnerabilities (https://owasp.org/www-community/attacks/Command_Injection). +# +# This policy will only work when using the full CRDs (https://argo-workflows.readthedocs.io/en/latest/installation/#full-crds). +# You must create a ValidatingAdmissionPolicyBinding to use it. +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicy +metadata: + name: "argo-dangerous-interpolation-vap" +spec: + failurePolicy: Fail + matchConstraints: + resourceRules: + - apiGroups: ["argoproj.io"] + apiVersions: ["v1alpha1"] + operations: ["CREATE", "UPDATE"] + resources: ["workflows"] + validations: + - expression: > + !object.spec.templates.map( + t, has(t.container) + ? ( + (has(t.container.command) ? t.container.command : []) + + + (has(t.container.args) ? t.container.args : []) + ) + : ( + has(t.script) + ? ( + (has(t.script.command) ? t.script.command : []) + + + (has(t.script.source) ? [t.script.source] : []) + ) + : [] + ) + ).exists(values, values.exists(value, value.matches('\\{\\{[^\\}]*\\}\\}'))) + message: "Dangerous interpolation detected" \ No newline at end of file diff --git a/examples/validation_test.go b/examples/validation_test.go index 14f28e7ebb82..8fb209e51826 100644 --- a/examples/validation_test.go +++ b/examples/validation_test.go @@ -7,7 +7,7 @@ import ( ) func TestValidateExamples(t *testing.T) { - failures, err := ValidateArgoYamlRecursively(".", []string{"testvolume.yaml", "simple-parameters-configmap.yaml", "memoize-simple.yaml"}) + failures, err := ValidateArgoYamlRecursively(".", []string{"testvolume.yaml", "simple-parameters-configmap.yaml", "memoize-simple.yaml", "argo-dangerous-interpolation-vap.yaml"}) if err != nil { t.Errorf("There was an error: %s", err) } diff --git a/test/e2e/argo_server_test.go b/test/e2e/argo_server_test.go index d49c3d5f3713..c6ef8af763d5 100644 --- a/test/e2e/argo_server_test.go +++ b/test/e2e/argo_server_test.go @@ -18,6 +18,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + admissionv1 "k8s.io/api/admissionregistration/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -668,6 +669,77 @@ func (s *ArgoServerSuite) TestPermission() { }) } +func (s *ArgoServerSuite) TestWorkflowRejectedByValidatingAdmissionPolicy() { + ctx := context.Background() + + s.Run("Create ValidatingAdmissionPolicy", func() { + obj, err := fixtures.LoadObject("@functional/argo-dangerous-interpolation-vap.yaml") + s.Require().NoError(err) + _, err = s.KubeClient.AdmissionregistrationV1().ValidatingAdmissionPolicies().Create(ctx, obj.(*admissionv1.ValidatingAdmissionPolicy), metav1.CreateOptions{}) + s.Require().NoError(err) + }) + s.T().Cleanup(func() { + _ = s.KubeClient.AdmissionregistrationV1().ValidatingAdmissionPolicies().Delete(ctx, "argo-dangerous-interpolation-vap", metav1.DeleteOptions{}) + }) + + s.Run("Create ValidatingAdmissionPolicyBinding", func() { + obj, err := fixtures.LoadObject("@functional/argo-dangerous-interpolation-vap-binding.yaml") + s.Require().NoError(err) + _, err = s.KubeClient.AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Create(ctx, obj.(*admissionv1.ValidatingAdmissionPolicyBinding), metav1.CreateOptions{}) + s.Require().NoError(err) + }) + s.T().Cleanup(func() { + _ = s.KubeClient.AdmissionregistrationV1().ValidatingAdmissionPolicyBindings().Delete(ctx, "argo-dangerous-interpolation-vap-binding", metav1.DeleteOptions{}) + }) + + // Sleep to wait for VAP to be created + time.Sleep(2 * time.Second) + + workflow := `{ + "workflow": { + "metadata": { + "name": "test", + "labels": { "workflows.argoproj.io/test": "dangerous-interpolation" } + }, + "spec": { + "arguments": { + "parameters": [{ "name": "message", "value": "foo" }] + }, + "entrypoint": "run-workflow", + "templates": [{ + "name": "run-workflow", + %s + }] + } + } + }` + s.Run("Rejects workflow with interpolation in script template", func() { + s.e().POST("/api/v1/workflows/argo"). + WithText(fmt.Sprintf(workflow, `"script": { + "image": "argoproj/argosay:v2", + "command": ["sh", "-c"], + "source": "echo message: {{workflow.parameters.message}}" + }`)). + Expect(). + Status(400). + Body(). + Contains("denied request: Dangerous interpolation detected") + }) + + s.Run("Rejects workflow with interpolation in container template", func() { + s.e().POST("/api/v1/workflows/argo"). + WithText(fmt.Sprintf(workflow, `"container": { + "image": "argoproj/argosay:v2", + "command": ["sh", "-c"], + "args": ["echo message: {{workflow.parameters.message}}"] + }`)). + Expect(). + Status(400). + Body(). + Contains("denied request: Dangerous interpolation detected") + }) +} + func (s *ArgoServerSuite) TestLintWorkflow() { s.e().POST("/api/v1/workflows/argo/lint"). WithBytes([]byte((`{ diff --git a/test/e2e/functional/argo-dangerous-interpolation-vap-binding.yaml b/test/e2e/functional/argo-dangerous-interpolation-vap-binding.yaml new file mode 100644 index 000000000000..2feb00f4a58b --- /dev/null +++ b/test/e2e/functional/argo-dangerous-interpolation-vap-binding.yaml @@ -0,0 +1,12 @@ +apiVersion: admissionregistration.k8s.io/v1 +kind: ValidatingAdmissionPolicyBinding +metadata: + name: "argo-dangerous-interpolation-vap-binding" + namespace: argo +spec: + policyName: "argo-dangerous-interpolation-vap" + validationActions: [Deny] + matchResources: + objectSelector: + matchLabels: + workflows.argoproj.io/test: dangerous-interpolation \ No newline at end of file diff --git a/test/e2e/functional/argo-dangerous-interpolation-vap.yaml b/test/e2e/functional/argo-dangerous-interpolation-vap.yaml new file mode 120000 index 000000000000..464b32e1b743 --- /dev/null +++ b/test/e2e/functional/argo-dangerous-interpolation-vap.yaml @@ -0,0 +1 @@ +../../../examples/argo-dangerous-interpolation-vap.yaml \ No newline at end of file