-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to disable param interpolation in command, args, and source #5114
Comments
From #12534 (comment), I suggested turning this on by default in a breaking change and then eventually removing the option to template these entirely. |
…ion. Fixes argoproj#5114 Signed-off-by: Mason Malone <[email protected]>
…ion. Fixes argoproj#5114 Signed-off-by: Mason Malone <[email protected]>
I entered #14045 to add an example validating admission policy that can block interpolation in those fields. I think ValidatingAdmissionPolicy is going to be the future for things like this, because they're very flexible and don't require changes to Argo. They do, however, require using the full CRDs, which are currently broken, so I entered I entered #14044 to fix them. |
This was split off from argoproj#5114 to make it easier to review. This makes a few minor improvements to the build to support the full CRD fixes: 1. Update devcontainer to install k3s 1.29.10, since that's what we use in CI: https://github.com/argoproj/argo-workflows/blob/ef41f83f801a6ff48c87f882a6b75d0e37529134/.github/workflows/ci-build.yaml#L263 1.27 is EOL and doesn't support things like validation rules in CRDs. 2. Update `make install` to use [server-side apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/), which is meant to replace client-side apply and isn't affected by size limitations for the CRDs. Since `kubectl apply --server-side` isn't compatible with `kubectl apply --prune`, I had to switch to [apply sets](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#alternative-kubectl-apply-f-directory-prune), which is intended to replace allow list pruning, and seems to work just as wlel. 3. Minor refactoring of the manifests under `manifests/` to use [Kustomize Components](https://kubectl.docs.kubernetes.io/guides/config_management/components/) so that we can share code with the the manifests under `test/e2e/manifests` without duplication. See argoproj#14001 for more details on this approach. Signed-off-by: Mason Malone <[email protected]>
Summary
(Follow-up to #5061 after thinking about it some more.)
I think param interpolation in
command
,args
, andsource
is an anti-pattern. In 99.9% of use cases, inputs are input, they're not code. Allowing interpolation in these fields creates a major code injection attack surface. Developers will copy/paste hello-world examples and then build on them, forgetting to add safeguards against malicious param values.When Argo was a young project, I think this feature made a lot of sense. It's super easy to write up a workflow and use input param interpolation to change code behavior. But as Argo gains adoption, I think it will become a major problem. SQL injections became ubiquitous because adoption of the language grew faster than the safeguards. The param interpolation feature introduces a similar attack surface to the Kubernetes ecosystem.
Of course, removing such a widely-used feature would be very disruptive. So I recommend adding a workflow controller option to disable param interpolation in the
command
,args
, andsource
fields, which are the fields most often interpreted directly as code.If the dev team doesn't love this idea, here are a few alternatives:
command
,args
, andsource
param interpolation examples w/ the equivalent env vars implementationUse Cases
Large organizations could enable this feature to enforce best practices (e.g. using env vars for params instead of interpolation) across a large group of developers and workflows which would be difficult to manually review/govern.
Message from the maintainers:
Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.
The text was updated successfully, but these errors were encountered: