[TEP-0146] Parameters in Script

[aaron-prindle]

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 * TEP-0099: Parameters in Script

[jerop]

/kind tep

[vdemeester]

/assign

[JeromeJu]

/assign @Yongxuanzhang @JeromeJu

[tekton-robot]

@JeromeJu: GitHub didn't allow me to assign the following users: Yongxuanzhang.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @Yongxuanzhang @JeromeJu

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.

[Yongxuanzhang]

/assign

[QuanZhang-William]

Hi @aaron-prindle, thanks for the PR! We have a TEP template to leverage in case you don't know 😁

[aaron-prindle]

Hi @aaron-prindle, thanks for the PR! We have a TEP template to leverage in case you don't know 😁

@Yongxuanzhang - thank you for this! Initially I was going off the format of the TEP this TEP is building off of [TEP-0099]: Parameters in Scripts which had a different format. I've updated the TEP here to be in line w/ this template now, thanks!

[Yongxuanzhang]

Thanks! I think the problem statement is clear.

[Yongxuanzhang]

Do we consider to implement this as a new feature gated by feature flag? Cluster operators can choose enable this feature to disallow insecure tasks. They can choose to continue use param injection

[aaron-prindle]

This is a great point, I will update the doc to explain how the ability to toggle off the current default behaviour via a feature-gate could be helpful here and allow a goal/requirement here of making Tekton safe by default be more feasible for users. Will update this thread when I have completed adding this

[afrittoli]

I'm not sure we could disallow this by default in v1 - it would be a backward incompatible change.
But we could provide a flag to let operator disallow params in scripts if they want to.

[aaron-prindle]

Updated the doc to explain that feature-gates will be added:

• upon initial implementation so that cluster operators can OPT-IN to disabling direct param injection for script
• [optional/TBD] post-implementation so users can opt-in to default behavior (if we deem this necessary)

[aaron-prindle]

+1, agree the default should be backwards compatible and allow the current behavior. As you mention it makes sense to allow the cluster operator to disable the default behavior via a feature flag though. I’ve updated the doc in a few places (including the highlighted section) to explain how feature-gates could be used to support a potential option for restricting the default v1 (but NOT by default). Users choosing this option would also give us some insights/feedback related to the experience for if/when we remove the default behavior in 1.X/v2 (or after the ~9 mo deprecation policy depending on what we decide)

[JeromeJu]

Thanks for the problem statement @aaron-prindle . The examples and the diff between before and after and resolved LGTM 🙏 Just some minor comments.

[JeromeJu]

Thanks @aaron-prindle for putting putting up the discussion. I noticed there are already discussions at #1077 (comment) on using feature gate to turn off the default behaviour. But I am afraid the existing feature has now been regarded as stable, and we might have to comply with the API compatibility policy for its deprecation cycle to keep both options available to users.

[JeromeJu]

Also noticed in

• "During the inital creation of this feature, the user experience will not change as the original UX/functionality will be preserved."
• "At a later time or in an updated apiVersion (v2*),"

These all SGTM. Wondering if we would like to call out explicitly that we'd wait till the end of the 9-month deprecation period?

[vdemeester]

Wondering if we would like to call out explicitly that we'd wait till the end of the 9-month deprecation period?
@JeromeJu where would this one apply ?

[aaron-prindle]

I've updated the text here to explicitly reference the 9 mo deprecation policy for any v1.X changes

[aaron-prindle]

As noted here and in some other comments, the doc was initially unclear around the backwards compatibility guarantees - it has now been updated. The proposed solution will be backwards compatible for the 9 mo deprectation window and will have an opt-in feature flag for users to modify this if they desire (assuming folks here agree to that during the implementation).

[afrittoli]

It would be nice to have such tooling indeed, maybe something that can detect issues and optionally attempt to fix them.

[aaron-prindle]

+1, as part of the implementation we would add some automation that did either the following:

• Either as a warning in controller logs OR when the insecure defaults are eventually removed (via opt-in feature flag or after decided upon depreciation timing in v1.X, v2, etc.) - tekton will suggests running a new to-be-implemented CLI command such as tkn fix that could update input YAML files, converting them to using the new secure “.env” syntax for script and relaying any issues/manual changes necessary as needed
• Controller implementation that is able to convert all/subset of cases the automatically “under the hood”. Ideally this would work for all cases but perhaps for some 80% (eg: bash only, etc.) cases it works and for others it returns a validation error

[afrittoli]

This looks good, maybe we could have an example with non-bash (e.g. python) too?
I just want to make sure that whatever solution we design, works across the different kinds of scripts we support.

[aaron-prindle]

Ah good point, just added a security vulnerability example for Python here as well detailing how to do a similar exploit as the bash example with the current implementation. Additionally now the preferred solution (Explicity Project Parametersas Environment Variables inSteps (Language-Agnostic)) shows an example of how non-bash implementations would work.

[afrittoli]

+1 on ease of use and possibly I would add verbosity.

The current proposed approach with environment variables works fine but is very verbose. To get some input from a pipeline into a script there are many levels of indirection: pipeline.param -> task.param -> step.env.variable -> env variable in script, which is both frustrating to write and maintain as well as using a lot of space in etcd.

[aaron-prindle]

Yes I agree that currently the solution here is verbose in that there are multiple levels of indirection as you have outlined. The current proposal has been designed based on the consensus and positive feedback received from the previous proposal (now closed TEP-099). Additionally the currently suggested solution (*.env) similar to those employed in tasks that successfully addressed this issue. I added an additional bullet under Requirements titled Minimize verbosity as much as possible which mentions the issue you raised above.

Below in your comments you have mentioned an alternative implementation options could be that we set the env vars in the pod directly which would seemingly be less verbose. Once this is merged I will compare different implementation paths (including setting the env vars in pods directly) and we can weigh the tradeoffs there. I have currently not expressly added that implementation detail in the doc (plan to post merge as "proposed")

[afrittoli]

An alternative to mutating the step specification could be to inject the env in the Pod.
We could do that by default for all params by default.

There are some gotchas:

• the translation from param name to env variable name risks being brittle
• what happens in case of conflicts with user-defined environment variables or how do we prevent such conflicts
• how do we handle array params? We added support for indexing into array params but how do we render the list into an environment variable that is usable by different kinds of script environments? Shall we use a JSON blob? That's not too bash-friendly but very python-friendly
• how do we handle object params? Do we add one variable for each field? Or a single variable with a JSON blob?

[vdemeester]

The last 2 questions of @afrittoli are also valid independently of "mutating the step spec" or not, right ?

[aaron-prindle]

+1 to the gotchas outlined, I will address each one inline:

• the translation from param name to env variable name risks being brittle
  • This is a good point, to clarify could you elaborate on the specific brittleness you mention? My interpretation would be that because param names are somewhat unconstrained multiple param names might map to the same env var name in translation if not careful. I have not added this into the document but to reduce brittleness in the param-name- > env-var name translation we could add a param name hash suffix (similar to k8s pods) to avoid collisions eg: TEKTON_PARAM_FOO_X5GHA. This would make directly knowing the name for the param a bit more difficult for users but Tekton is dynamically generating these anyway so it should be ok if it is somewhat opaque.
• what happens in case of conflicts with user-defined environment variables or how do we prevent such conflicts
  • Good point, this case important to call out explicitly. I’ve added this into the document now under Design Details in a bulleted section titled - “Considered Edge Cases and Gotchas”. The gist is that in case of an overlap with user-defined environment variables, Tekton will throw a validation error stating that the param should be renamed. NOTE: collision should be very unlikely given the TEKTON_PARAM prefix and we can also optionally add a sha suffix to the env var (based on the param name) to further unique-ify the env var name to reduce collision.
• how do we handle array params? We added support for indexing into array params but how do we render the list into an environment variable that is usable by different kinds of script environments? Shall we use a JSON blob? That's not too bash-friendly but very python-friendly
  • Currently the plan would be that the values in the env var map 1:1 with how they are currently used as that UX is working for users atm. In the case of an array param, it would be the same values based on the syntax, for an array param + index (params.arr[0]) we would get the string value and for the entire array we would provide the list as a comma separated array (params.arr -> first,second,third…) The TEP suggestion is built around the script field which only supports string type variables which currently has comma separated serialization for the array and actually has a validation error against using an object directly (vs an object field). I have added some examples under the “Suggested Solution”.
• how do we handle object params? Do we add one variable for each field? Or a single variable with a JSON blob?
  • There would be a variable for each field. I have added some examples under the “Suggested Solution”. Currently in my experimentation around this it seems that you get a validation error if you try to reference a param object in a script directly (vs a param object field which works) - as such there doesn't seem to be a precedent for supporting the obj param directly as they can't be used IIUC in script currently

[afrittoli]

Files would hold JSON blobs similar to what we do for results I guess?
Producing JSON with bash is not too bad, parsing it is a bit harder, but maybe we could provide some helper functions in a bash resource file mounted in a local folder.

[aaron-prindle]

+1 to providing some helper scripts to be used for arrays (and possibly objects) as needed. I’ve added a note under the alternative there related to adding UX helper functions

[afrittoli]

How would this work for array/object params?

[aaron-prindle]

I have not investigated as deeply for the .path alternative as I have for the .env option but I believe this implementation would require helper functions as you suggest above to get this correct. Serializing arrays/objects into JSON syntax and then creating helper functions to parse the necessary piece of information instead when using [*] or [0] syntax (or the obj array directly) would be required. I did not update the document here but if folks prefer this alternative or want more detail here now I can add more information. In the case that would be too cumbersome, we could try to map files 1:1 to used values and have a file for each field/array-index but this should only be used as a final option as it doesn't scale well.

[afrittoli]

If we did fail validation, we would then basically not support them at all, which would be a backwards incompatible change. We could do that in v2, for v1 we might do that as an opt-in feature or a warning system - even though validation in itself does not have any facility for deprecations or warnings that I'm aware of, so it's unclear how the warning could be delivered to the user

[aaron-prindle]

+1, currently the idea for rolling this out would be to keep backwards compatibility with an opt-in feature-gate for cluster operators to remove this in v1 (if we agree this makes sense). For the warnings, it wasn’t clear to me the de-facto way to do this in Tekton. As stated, we could output some “WARN: Tekton is deprecating <...>”, see docs link here for more information on deprecation timelines how to migrate <doc-link AND/OR link to TBD bespoke tkn command for migration>” on creation of a Task but not sure how visible this might be to most end users.

[afrittoli]

Thanks for this proposal.
I left a couple of comments.

NIT: I have the impression that there was a proposal with the .env syntax initially documented, and then removed in favour of a list of alternatives, as some part of the document seem to assume a specific design and others don't.

In any case, I'm happy to have this merged as a proposal, but I'd like to see more coverage of array/object params as well as non-bash environments (e.g. python).

[aaron-prindle]

Thank you for your feedback @afrittoli & @vdemeester! I’ve updated the document addressing all of the feedback posted (see individual comments). Wanted to address the points in you final comment below:

• NIT: I have the impression that there was a proposal with the .env syntax initially documented, and then removed in favour of a list of alternatives, as some part of the document seem to assume a specific design and others don't.

  • Apologies for the confusion in the document, I have updated the document which was unclear in some places. Originally some options in the doc were very similar so I have deduped things now and no longer reference any “implicit” option as it was confusing and is not really an option due to backwards compat guarantees. The proposed solution here is broadly to add additional param suffixes (*.env, *.path, etc.) that modify how a value is interpolated (+ potentially other configuration) with the primary one (and first to be implemented) being *.env. Then we would gauge interest from reviewers on if we should add other options (*.path) or possibly what the preferred option would be if folks don’t agree on *.env as the highest priority.

• In any case, I'm happy to have this merged as a proposal, but I'd like to see more coverage of array/object params as well as non-bash environments (e.g. python).

  • Great, I’ve updated the TEP to explain how the array and object support would work and added examples across the preferred solution for array/object params. Currently the plan would be that the values in the env var map 1:1 with how they are currently used as that UX is working for users atm. In the case of an array param, it would be the same values based on the syntax, for an array param + index (params.arr[0]) we would get the string value and for the entire array we would provide the list as a comma separated array (params.arr -> first,second,third…) The TEP suggestion is built around the script field which only supports string type variables which currently has comma separated serialization for the array and actually has a validation error against using an object directly (vs an object field). I have added some examples under the “Suggested Solution”.
  • Additionally I’ve added some an example related to non-bash environments (eg: python), happy to elaborate in the document or add more examples - currently not sure the best way to enhance the doc related to this.

[aaron-prindle]

Thanks for the reviews here! @afrittoli - does this look good now to be merged as “proposed”? If not, what are the open questions to resolve to get this to implementable? I've added additional information and examples around array params, obj params, and non-bash environments. Happy to answer any additional q's or iterate on the doc as needed. Thanks!

[vdemeester]

What's the suggested solution here ? Usage of $(params.url.env) or having ${PARAMS_URL} working by default ? I am not sure I understand the before/after/resolved.

• Are we mutating the Task definition (to add the environment variables), or are we just setting them at Pod creation time ?
• What happens if params.url and params.URL exists ? (I think we error out on those already, but I am not 100% sure 😅 ).

[aaron-prindle]

Hi @vdemeester, responding to your questions below:

What's the suggested solution here ? Usage of $(params.url.env) or having ${PARAMS_URL} working by default ?

• Usage of $(params.url.env) is the recommended solution here,

Are we mutating the Task definition (to add the environment variables), or are we just setting them at Pod creation time ?

• This is somewhat of an open question that I am hoping to explore/discuss more in the "implementable" phase (happy to discuss it more now well, I'm not quite sure the cutoffs for the phase distinctions). The initial proposal was to set them in the Task definition (to add the environment variables) but
  the suggestion to set them @ Pod creation time above ([TEP-0146] Parameters in Script #1077 (comment)) is an alternative with pros which I plan on investigating after this is merged as proposed. To me this is an implementation detail IIUC, happy to continue discussing it here.

What happens if params.url and params.URL exists ? (I think we error out on those already, but I am not 100% sure 😅 ).

• This was mentioned in this comment as well - [TEP-0146] Parameters in Script #1077 (comment) (the translation from param name to env variable name risks being brittle). In testing this, tektoncd/pipeline allows this currently - you can successfully create a Pipeline with params.url and params.URL and the validation hook allows this. The allowable chars for env vars are: [A-Za-z0-9_]+. The options here would be:
  • add a sha suffix to the env var PARAM_*_<SHA_SUFFIX> based on the param name to guarantee uniqueness for param -> env var conversion.
  • use param variable names with the correct case, eg: PARAM_url and PARAM_URL

[vdemeester]

Sounds good, thanks @aaron-prindle
/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, JeromeJu, vdemeester

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

The pull request process is described here

Needs approval from an approver in each of these files:

• teps/OWNERS [afrittoli,vdemeester]

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

[aaron-prindle]

Thanks @vdemeester! Lmk if there is anything I need to do on my end to get this merged as proposed, currently seeing:
tide Pending — Not mergeable. Needs lgtm label.

[Yongxuanzhang]

We need someone who has the permission to lgtm

[Yongxuanzhang]

This tep is very similar like https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable ? Is it mentioned in the proposal as a reference?

[aaron-prindle]

This tep is very similar like https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable ? Is it mentioned in the proposal as a reference?

@Yongxuanzhang - just added this as a reference, thanks!

[dibyom]

/lgtm