-
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
docs: add example ValidatingAdmissionPolicy to block param interpolation #14045
base: main
Are you sure you want to change the base?
Conversation
…ion. Fixes argoproj#5114 Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
This rewrites `hack/test-examples.sh` in Go to make those tests easier to debug and facilitate testing more complex scenarios, like the VAP added in argoproj#14045 The new tests do strict validation, which caught a bug in `examples/webhdfs-input-output-artifacts.yaml`: ``` $ kubectl apply -f examples/webhdfs-input-output-artifacts.yaml Error from server (BadRequest): error when creating "examples/webhdfs-input-output-artifacts.yaml": Workflow in version "v1alpha1" cannot be handled as a Workflow: strict decoding error: unknown field "spec.templates[0].outputs.artifacts[0].overwrite" ``` $ make test-examples <SNIP> examples_test.go:28: Error parsing ../../examples/webhdfs-input-output-artifacts.yaml: strict decoding error: unknown field "spec.templates[0].outputs.artifacts[0].overwrite" === FAIL: ExamplesSuite/TestExampleWorkflows FAIL github.com/argoproj/argo-workflows/v3/test/e2e 1.580s FAIL make: *** [Makefile:609: test-examples] Error 1 ``` Signed-off-by: Mason Malone <[email protected]>
Signed-off-by: Mason Malone <[email protected]>
/retest |
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 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. |
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
, andsource
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 #14044.
This PR adds an example
ValidatingAdmissionPolicy
that rejects workflows that interpolate parameters in thecommand
,args
, and/orsource
fields. Initially, I included it in the release manifests undermanifests/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 #14094 to refactor how we handle testing examples.
Verification
Ran examples locally: