Merge pull request #4 from sbwsg/params-script

Updated TEP-0099 with feedback and new alts
tektoncd · Jan 7, 2022 · 006bd26 · 006bd26
2 parents 7d22bd9 + 7e7d815
commit 006bd26
Showing 1 changed file with 115 additions and 22 deletions.
diff --git a/teps/0099-parameters-in-script.md b/teps/0099-parameters-in-script.md
@@ -58,8 +58,8 @@ in two ways:
 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*.
+- By default in v1, prevent implicit interpolatin of param values into executable
+  contexts unless we can validate that doing so is safe.
 
 ## Motivation
 
@@ -200,9 +200,11 @@ the unterminated quote string, instead of echoing the quote string.
 
 4. **`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.
-
+   Disallow interpolating `Param` values directly into executable
+   contexts. If the user wants to execute the contents of a param as a
+   script then they should be able to, but it shouldn't be possible
+   without "opting in" to that behaviour by calling `eval` or writing
+   the value of a param to a script and running it.
 
 ### Use Case
 
@@ -227,8 +229,8 @@ 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
+1. **[Secure by default in v1][secure-by-default]**: Out of the box
+   *Tekton Pipelines* v1 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
@@ -237,18 +239,13 @@ while still using it reliably and safely.
    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`
+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.
+1. **Provide an escape hatch in v1**. If a cluster operator or admin 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
 
@@ -293,8 +290,8 @@ 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.<name>)`.
+in the same way that `Results` are supplied. The file path for the param
+would be made available via a variable (possibly reusing `$(params.<name>)`.
 
 
 ```yaml
@@ -310,7 +307,16 @@ file path, we would hide it behind a variable (possibly reusing `$(params.<name>
   - name: foo
     image: myimage
     script: |
-      echo $(params.bar)
+      # note: echo becomes cat because we now print
+      # the contents of a file
+      cat $(params.bar)
+  
+  # After - Resolved
+  steps:
+  - name: foo
+    image: myimage
+    script: |
+      cat /tekton/params/bar
 ```
 
 This approach is also under discussion in the context of changing the way `Result`
@@ -352,8 +358,13 @@ This was the proposal in [TEP-0017][tep-0017].
 
 ##### Discussion
 
-This approach mutates the specification, but it is something we're already doing
-with implicit `Parameters` per [TEP-0023][tep-0023].
+This approach mutates the specification (at the user's request), but it
+is something we're already doing with implicit `Parameters` per
+[TEP-0023][tep-0023].
+
+Note: while this would be safer than Tekton Pipeline's existing
+behaviour our own catalog guidance claims that "no amount of escaping
+will be air-tight".
 
 ### 4. Reject `Parameters` in `Script` and provide instructions to fix
 
@@ -413,7 +424,9 @@ the implicit projection optional, where it only applies to a subset of
   - name: foo
     image: myimage
     script: |
-      echo $(params.bar.env)
+      # note here that params.bar.env must be double-wrapped
+      # once for tekton in $() and again for shell ${}
+      echo ${$(params.bar.env)}
       
   # After - Resolved
   steps:
@@ -432,6 +445,86 @@ 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 .
 
+### 6. Optionally Project `Parameters` as Environment Variables in `Steps` (Language-Agnostic)
+
+Building on Alternative 6, Tekton's `params.foo.env` variable could
+expand into `PARAM_FOO`, without the shell-specific braces `${}`. The
+user would then be required to look up the env var using their script
+language's tools (e.g. `os.getenv` in python, `process.env.X` in node,
+`${}` in shell).
+
+```yaml
+  # Before
+  steps:
+  - name: foo
+    image: myimage
+    script: |
+      echo $(params.bar)
+  
+  # After
+  steps:
+  - name: foo
+    image: myimage
+    script: |
+      # note here that params.bar.env must be double-wrapped
+      # once for tekton in $() and again for shell ${}
+      echo ${$(params.bar.env)}
+      
+  # After - Resolved
+  steps:
+  - name: foo
+    image: myimage
+    env:
+    - name: PARAM_BAR
+      value: $(params.bar)
+    script: |
+      echo ${PARAM_BAR}
+```
+
+##### Discussion
+
+This alternative has similar properties to 5 but removes the constraint
+of being shell-only. Unfortunately it also loses the backwards-compatibility.
+
+### 6. Combine some of the above alternatives
+
+Building on alternatives 1, 2 and 5 Tekton Pipelines could offer an
+explicit option to the user whether to project as env or file.
+
+```yaml
+  # Before
+  steps:
+  - name: foo
+    image: myimage
+    script: |
+      echo $(params.bar)
+  
+  # After
+  steps:
+  - name: foo
+    image: myimage
+    script: |
+      echo ${$(params.bar.env)}
+      cat $(params.quux.path)
+      
+  # After - Resolved
+  steps:
+  - name: foo
+    image: myimage
+    env:
+    - name: PARAM_BAR
+      value: $(params.bar)
+    script: |
+      echo ${PARAM_BAR}
+      cat /tekton/params/quux
+```
+
+##### Discussion
+
+Similar to the other options this mutates the spec, though does so at the user's
+discretion. This alternative could be an extension of 5 and support for
+the filesystem (`$(params.bar.path)`) could be added later if needed.
+
 ## Open Questions
 
 1. Is it sufficient to address `Parameters` in `Script` only, or do we need to