feat: add example ValidatingAdmissionPolicy to block param interpolation. Fixes #5114 #14045
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The full CRDs at `manifests/base/crds/full` are only intended for usage in editors (argoproj#11266 (comment)) and are unusable otherwise. Trying to install them will cause spurious validation errors with nearly every workflow. Having the full CRDs would enable many useful features, such as `kubectl explain` output (argoproj#8190), [validating admission policies](https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/) (argoproj#13503), and integration with tools like cdk8s (argoproj#8532). As explained at argoproj#13754 (comment), there's two issues that prevent us from using the full CRDs everywhere: 1. [Kubernetes size limits](kubernetes/kubernetes#82292) when using client-side apply. This is not a problem when using . 2. Wrong validation information due to kubebuilder limitations. For the first issue, I verified that this is no longer an issue when using [server-side apply](https://kubernetes.io/docs/reference/using-api/server-side-apply/), so I updated the `make install` target to use it. Since `kubectl apply --server-side` isn't compatible with `kubectl apply --prune`, I had to switch to use [apply sets](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/declarative-config/#alternative-kubectl-apply-f-directory-prune), which is intended to replace `--prune`, and seems to work well. For the second issue, I went through and added workarounds to `hack/manifests/crds.go` for all the kubebuilder issues I could find. Since the E2E test suite now uses the full CRDs, it should tell us if there's anything I missed. I didn't update the release manifests to use the full CRDs, since that's a big change and we probably should wait awhile to make sure there aren't any unexpected issues. Users can easily opt into the full CRDs if they want. Signed-off-by: Mason Malone <651224+MasonM@users.noreply.github.com>
MasonM
added
type/security
…ion. Fixes argoproj#5114 Signed-off-by: Mason Malone <651224+MasonM@users.noreply.github.com>
This was referenced Jan 2, 2025
|
/retest |
This was referenced Jan 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5114
Motivation
This PR builds off #14044
As @crenshaw-dev explained in #5114, allowing string interpolation inside the
command,args, andsourcefields 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 a
ValidatingAdmissionPolicyto https://github.com/argoproj/argo-workflows/blob/5e634fd24012db2c482daed465943081c0c408c2/examples/argo-dangerous-interpolation-vap.yaml that rejects workflows that interpolate parameters in thecommand,args, and/orsourcefields. I also added a test attest/e2e/argo_server_test.gothat verifies it. Initially, I includedValidatingAdmissionPolicyin 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.TODO: I need to add documentation for this and update the examples that are currently using dangerous interpolation
Verification
E2E tests