argoproj
docs: add example ValidatingAdmissionPolicy to block param interpolation
Partial fix for #5114. After this is merged, I'll update Workflow Variables and Security to add warnings about this. Also, I'll fix the examples currently using interpolation in a shell script to stop doing so, since we shouldn't encourage this practice.
Motivation
As @crenshaw-dev explained in #5114, allowing string interpolation inside the command, args, and source fields can be dangerous in the presence of user input, since it can lead to command injection vulnerabilities. These vulnerabilities are ubiquitous with GitHub Actions, and barely a month goes by before another high-profile open-source project gets compromised (example).
For security-conscious organization, providing options to prevent these kind of vulnerabilities is important and would go a long way to distinguishing Argo Workflows from the competition in the CI space.
Modifications
@crenshaw-dev suggested adding a controller option to disable interpolation. That works and would be fairly easy to implement, but the problem is flexibility. It's likely some organizations would want to narrowly target the option, or provide an allowlist that bypasses validation, which is difficult to do with custom logic.
Validating admission policies are a new feature in Kubernetes v1.30 that allows highly flexible, in-process validation logic using CEL. The downside is the CRD needs to specify all fields that are being validated, which necessitates the changes in https://github.com/argoproj/argo-workflows/pull/14044.
This PR adds an example ValidatingAdmissionPolicy that rejects workflows that interpolate parameters in the command, args, and/or source fields. Initially, I included it in the release manifests under manifests/quick-start/, but then I realized we still support Kubernetes v1.29, which requires enabling a feature gate to use validating admission policies. Once we drop support for v1.29, then I think we should include this in the release manifests.
Initially, I added some automated tests for this, but it was getting a bit messy, so I entered https://github.com/argoproj/argo-workflows/pull/14094 to refactor how we handle testing examples.
Verification
Ran examples locally:
$ kubectl apply --server-side -f examples/validating-admission-policies/argo-dangerous-interpolation-vap.yaml
validatingadmissionpolicy.admissionregistration.k8s.io/argo-dangerous-interpolation-vap serverside-applied
$ kubectl apply --server-side -f examples/validating-admission-policies/argo-dangerous-interpolation-vap
-binding.yaml
validatingadmissionpolicybinding.admissionregistration.k8s.io/argo-dangerous-interpolation-vap-binding serverside-applied
$ kubectl create -f examples/validating-admission-policies/rejected-workflow.yaml
The workflows "rejected-workflow-fvj4h" is invalid: : ValidatingAdmissionPolicy 'argo-dangerous-interpolation-vap' with binding 'argo-dangerous-interpolation-vap-binding' denied request: Dangerous interpolation detected
Personally, I'd like to see a proper documentation page on this. Stumbling across the examples probably isn't the best UX.
@tico24 You're right. I mentioned in the PR description, but I think the ideal would be to include the ValidatingAdmissionPolicy in the release manifests so people can quickly enable this by just applying a ValidatingAdmissionPolicyBinding. The blocker there is k8s 1.29 support, but it sounds like we can drop that now (or very soon): https://github.com/argoproj/argo-workflows/pull/14142#issuecomment-2629570868
So I'll work on a PR to drop support for 1.29, then come back to this. But let me know if you disagree.
is dis ready? i wonder if u have found out a way to block wf yaml definitions over 200kb size with the validatingpolicy syntax as many of those can make etcd trip
@MasonM - would you prefer to do the rework to make this default, or shall I merge it as is?
