Make StepActions secure by default ?

[chitrangpatel]

Today, we allow param substitution directly in the scripts. For inlined-steps, we cannot change that behaviour because of v1 API compatibility reasons.

However, StepActions are an alpha feature. We can try to make them secure by default before they reach beta.

The proposal here to NOT allow param substitutions in scripts in StepActions. Users would need to add the params as env variables and use the env vars in scripts. In turn, we would not need to enable sharing of step results in scripts as well and take the sketchy approach of modifying the user's script.

This reduces the chances of shell attacks.

Alternatively:

The reason for not doing this would be consistency with inlined-steps. We could do this by default in tekton/v2 (?)
Here we can call this out and provide best practices via documentation.

WDYT ??

cc @wlynch @vdemeester @afrittoli

[vdemeester]

I "think" I would vote for consistency (and documentation).

[chitrangpatel]

I "think" I would vote for consistency (and documentation).

Good point. I added this point for context in the original message.

[dibyom]

/cc @aaron-prindle

[wlynch]

My concern is largely in line with the motivation of https://github.com/tektoncd/community/blob/main/teps/0146-parameters-in-script.md

I understand we don't want to remove the existing behavior because v1 compatibility, but StepActions give us an opportunity to move users in the right direction and make this change in a non-breaking way. I don't see v1 leaving any time soon - waiting on a v2 doesn't seem pragmatic if we know this is problematic behavior.

[chitrangpatel]

@wlynch do you think that based on https://github.com/tektoncd/community/blob/main/teps/0146-parameters-in-script.md, we should take the same approach for both, inlined-steps and step-actions?

There, instead of not allowing params in scripts, I think it is suggesting that users use a suffix .env like: $(params.foo.env). The controller would then replace it with an env variable ${PARAMS_FOO} for the user.

cc @aaron-prindle

[vdemeester]

@chitrangpatel that doesn't necessary work, ${PARAMS_FOO} works with bash (and most sh derivative), but a script can be anything —python, ruby, goscript, …) — and for those ${PARAMS_FOO} is not going to work.

[chitrangpatel]

@chitrangpatel that doesn't necessary work, ${PARAMS_FOO} works with bash (and most sh derivative), but a script can be anything —python, ruby, goscript, …) — and for those ${PARAMS_FOO} is not going to work.

Agreed. I think the TEP goes into more details about how it can enable this for any script (not just bash). I'm definitely not capturing all the different cases here.

My main point here was that we should try and take the same secure approach for inlined-steps and step-actions, whatever that is. The most secure way is not allowing direct param substitution in scripts but we can't do that for inlined-steps unfortunately. It is possible that they might need different approaches but we should try to look for an approach that works well for both.

[vdemeester]

Another point/question: StepAction is shipped and usuable in 0.54 am I right ? With the parameter substitutions, etc.. as it works on Task today — so we would break early adopters if we were to change that by default. I would rather treat this "security threat" the same for everything, aka Task, StepAction, … We should making sure we update the TEP to also take into account StepAction, and treat all at the same time (consistency again).

[chitrangpatel]

Another point/question: StepAction is shipped and usuable in 0.54 am I right ? With the parameter substitutions, etc.. as it works on Task today — so we would break early adopters if we were to change that by default. I would rather treat this "security threat" the same for everything, aka Task, StepAction, … We should making sure we update the TEP to also take into account StepAction, and treat all at the same time (consistency again).

Agreed! A consistent approach in that TEP would be preferred. WDYT @wlynch ?

[wlynch]

@wlynch do you think that based on https://github.com/tektoncd/community/blob/main/teps/0146-parameters-in-script.md, we should take the same approach for both, inlined-steps and step-actions?

Should? Yes - if we were to do this over again today I think this is what I'd advocate for.
That said, I'm skeptical that we can do this for existing inlined steps given that this has been the existing behavior for years - this would be a significant breaking change. I don't think we can make a breaking change here even for security reasons without significant disruption to users.

The less invasive approach here would be to leave the existing behavior as-is, and add a check for catlin to discourage use of it (similar to the scorecards dangerous workflow check).

This probably needs to be broken down into 2 questions:

1. What do we want the behavior to be long term, ignoring current usage/compatibility?
2. What can we do today given the existing v1 behavior?

For 1 it sounds like we want to disallow the behavior for script blocks everywhere.
For 2 my preference would be to keep inline behavior for compatibility as is w/ catlin check, don't allow this for step actions since we know that's where we want to move towards, and look to remove the behavior in inline steps if/when there's a v2.

[chitrangpatel]

Discussed in the API WG meeting.

In v1:

1. No param substitutions are allowed in step actions. The usage of params in StepActions will lead to a validation error.
2. For inlined-steps, we cannot do anything since it would be a major breaking change. Instead, we will add lint check in catlin to warn/throw errors if params are directly used in scripts. The hope is that the linter will ensure consistency between inlined-steps and step-actions.

In v2:

1. Make things consistent between inlined steps and step actions and secure (i.e. no param substitutions allowed in scripts.)

cc @wlynch @vdemeester @pritidesai

I will close this now. Feel free to reopen to add any concerns with this approach.