Lockdown /tekton/step folders to their own steps. #4352

wlynch · 2021-11-03T18:59:17Z

Changes

This change symlinks /tekton/step folders to a step's corresponding /tekton/run folder.
This is an incremental change to lock down the /tekton folder to prevent tampering
of exitCode files from other steps.
Note: this does not completely protect against a step from tampering from its own
output - more work will needed in a future PR to fully lock this down, but this
is a step in the right direction (and a complete fix will likely require a more
involved design).

Moves exitCode output to /tekton/run/<step #>/status
Symlinks /tekton/steps/<step #> and /tekton/steps/<step name> to
/tekton/run/<step #>/status.
Creates new tekton-init entrypoint subcommand to initialize the
Tekton step directory.
Removes -step_metadata_dir_link flag from the main entrypoint binary
(this behavior is now handled by the initcontainer).

Co-authored-by: Lee Bernick leebernick@google.com

/kind cleanup

Things to bikeshed:

Thoughts on /tekton/run/0/status as the run directory name for user facing data?

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

- /tekton/steps is now read-only.
- Directories in /tekton/steps are now symlinks. The content for the resolved paths remains the same.

tekton-robot · 2021-11-03T19:02:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	71.4%
cmd/entrypoint/subcommands/subcommands.go	75.0%	63.2%	-11.8
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.5%	89.0%	0.5

tekton-robot · 2021-11-03T19:03:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	71.4%
cmd/entrypoint/subcommands/subcommands.go	75.0%	63.2%	-11.8
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.5%	89.0%	0.5

tekton-robot · 2021-11-03T19:33:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	71.4%
cmd/entrypoint/subcommands/subcommands.go	75.0%	63.2%	-11.8
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.5%	89.0%	0.5

tekton-robot · 2021-11-03T19:39:49Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	71.4%
cmd/entrypoint/subcommands/subcommands.go	75.0%	63.2%	-11.8
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.5%	89.0%	0.5

wlynch · 2021-11-04T15:28:56Z

/test pull-tekton-pipeline-alpha-integration-tests

docs/developers/README.md

bobcatfish

Some initial drive by feedback from me, mostly I think I'm missing some details around why we need to maintain the 2 folders.

Also looks like a lot of coverage went down, maybe some missing unit test cases?

bobcatfish · 2021-11-04T21:27:00Z

docs/developers/README.md

+### `/tekton/steps`
+
+`/tekton/steps` contains Step information that is intended to be used by user
+steps (and is therefore covered by the Tekton API compatibility policy).


I'm wondering:

intended to be used by user steps in what way?

this is currently not included in our compat policy, maybe we need to update it - our compatibility policy currently mentions "the structure of the directories created in executing containers by Tekton" (https://github.com/tektoncd/pipeline/blob/main/api_compatibility_policy.md#what-is-the-api) and links to https://github.com/tektoncd/pipeline/blob/main/docs/tasks.md#reserved-directories which indicates everything other than /workspace and /tekton/results is considered an implementation detail

i think im generally not understanding why we want both steps and run to exist - i think i understand that "steps" is intended to be used by the contents of the steps themselves (the user specified steps) but i dont understand why - and also if run is readonly anyway (except for the current step) im not sure why we need the 2 dirs

We have conflicting documentation -

pipeline/docs/developers/README.md

Lines 195 to 200 in 5dcd434

The following directories are covered by the

[Tekton API Compatibility policy](../api_compatibility_policy.md), and can be

relied on for stability:

- `/tekton/results`

- `/tekton/steps`

says it is in scope.

If it's not in scope, I'm more than happy to remove the /steps directory altogether. :D The symlink shenanigans are purely to preserve the /tekton/steps file structure for backwards compatibility.

/cc @pritidesai who wrote the original documentation in 92746a2

ah yikes! some scope creep but if we could fix this so that we are only attempting to list the folders in the compat policy in one place that would be ideal 🙏 (pretty good chance adding 2 sources was my doing...)

Looking at TEP-0040 and the origin of the change, it's not clear to me if that folder needs to be part of the API spec or not (https://github.com/tektoncd/community/blob/main/teps/0040-ignore-step-errors.md#proposal), since we provide $(steps.<step-name>.exitCode.path) that does give us some freedom to potentially change the location of the directory. The TEP even says:

If you would like to use the tekton internal path, you can access the exit code by reading the file (it is not recommended though)

i.e. the path is referred to as "internal" several times in the TEP

so basically I have mixed feelings - if @pritidesai and/or others feel strongly the path should remain part of our API, no objections from me - but if possible I'd rather consider that an implementation detail and reply on variable replacement insteaed

thanks @wlynch and @bobcatfish for bringing my attention to this!

TEP-0040 proposed using the path variable $(steps.<step-name>.exitCode.path) and avoid using the tekton internal path.

At the same time, we had a vision of having the debug feature utilizing the /steps directory structure (not yet implemented) and the symlinks will make it easier to invoke the debug scripts.

tektoncd/community#463 (comment)

Just based on TEP-0040, I would avoid specifying it as covered by the Tekton API compatibility policy.

SGTM! I'll make these changes in another PR so it's clear why we're making the change.

Done in #4395. To limit the scope of this PR, I'm keeping the /tekton/steps folder (since I'm not sure what else is depending on it and how intrusive of a change it would be to remove). We can look into removing this in another PR.

Tiny nit to close the loop here: remove the remaining parenthetical in this commit describing /tekton/steps as covered ("and is therefore covered by the Tekton API compatibility policy").

wlynch · 2021-11-09T15:44:48Z

Part of #4227

tekton-robot · 2021-11-12T20:44:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	71.4%
cmd/entrypoint/subcommands/subcommands.go	75.0%	63.2%	-11.8
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.5%	89.0%	0.5

tekton-robot · 2021-11-23T13:15:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [vdemeester]

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

Previously the developer docs listed that /tekton/steps was in scope for the API compatibility policy, where-as the [user docs](docs/tasks.md#reserved-directories) did not say it was in scope. After discussion in [PR tektoncd#4352](tektoncd#4352 (comment)), we have come to agreement that this is **not** in scope. While this change in itself is not a breaking change, this opens the door for breaking changes (such as tektoncd#4352). Our stance is that users should use the [`$(steps.step-<step-name>.exitCode.path)` syntax](docs/tasks.md#accessing-steps-exitcode-in-subsequent-steps) to avoid breaking changes.

Previously the developer docs listed that /tekton/steps was in scope for the API compatibility policy, where-as the [user docs](https://github.com/tektoncd/pipeline/blob/aceb5880f009884be14f9a7409f428cc84157a75/docs/tasks.md#reserved-directories) did not say it was in scope. After discussion in [PR tektoncd#4352](tektoncd#4352 (comment)), we have come to agreement that this is **not** in scope. While this change in itself is not a breaking change, this opens the door for breaking changes (such as tektoncd#4352). Our stance is that users should use the [`$(steps.step-<step-name>.exitCode.path)` syntax](https://github.com/tektoncd/pipeline/blob/aceb5880f009884be14f9a7409f428cc84157a75/docs/tasks.md#accessing-steps-exitcode-in-subsequent-steps) to avoid breaking changes.

Previously the developer docs listed that /tekton/steps was in scope for the API compatibility policy, where-as the [user docs](https://github.com/tektoncd/pipeline/blob/aceb5880f009884be14f9a7409f428cc84157a75/docs/tasks.md#reserved-directories) did not say it was in scope. After discussion in [PR #4352](#4352 (comment)), we have come to agreement that this is **not** in scope. While this change in itself is not a breaking change, this opens the door for breaking changes (such as #4352). Our stance is that users should use the [`$(steps.step-<step-name>.exitCode.path)` syntax](https://github.com/tektoncd/pipeline/blob/aceb5880f009884be14f9a7409f428cc84157a75/docs/tasks.md#accessing-steps-exitcode-in-subsequent-steps) to avoid breaking changes.

afrittoli

Thank you for this! It's great to see security oriented patches to the core bits of Tekton!
This will need a rebase (because of the a document change 😅 ) - so perhaps you could add one more unit tests to step_init to cover the new functionality added there to create the root folder.

Otherwise this looks good to me

afrittoli · 2021-12-02T07:39:13Z

cmd/entrypoint/main.go

@@ -45,8 +45,7 @@ var (
 	breakpointOnFailure = flag.Bool("breakpoint_on_failure", false, "If specified, expect steps to not skip on failure")
 	onError             = flag.String("on_error", "", "Set to \"continue\" to ignore an error and continue when a container terminates with a non-zero exit code."+
 		" Set to \"stopAndFail\" to declare a failure with a step error and stop executing the rest of the steps.")
-	stepMetadataDir     = flag.String("step_metadata_dir", "", "If specified, create directory to store the step metadata e.g. /tekton/steps/<step-name>/")
-	stepMetadataDirLink = flag.String("step_metadata_dir_link", "", "creates a symbolic link to the specified step_metadata_dir e.g. /tekton/steps/<step-index>/")
+	stepMetadataDir = flag.String("step_metadata_dir", "", "If specified, create directory to store the step metadata e.g. /tekton/steps/<step-name>/")


NIT: it looks like the "=" lost its alignment?

🤷 gofmt is doing this, so I'm not going to fight it. My guess is the string concat above is resetting the alignment, and stepMetadataDirLink just happened to be the right length to align this block with the flags above.

afrittoli · 2021-12-02T07:45:22Z

cmd/entrypoint/post_writer.go

+
+	// Create directory if it doesn't already exist
+	if err := os.MkdirAll(filepath.Dir(file), os.ModePerm); err != nil {
+		log.Fatalf("Error creating parent directory of %q: %v", file, err)
+	}
+


Could you add tests for this new behaviour?
I tried removing this and unit tests in cmd/entrypoint/post_writer_test.go still work fine.

examples/v1beta1/taskruns/readonly-internal-dir.yaml

tekton-robot · 2021-12-07T20:13:03Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/step_init.go	Do not exist	70.0%
cmd/entrypoint/subcommands/subcommands.go	75.0%	73.7%	-1.3

tekton-robot · 2021-12-07T20:20:24Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	70.0%
cmd/entrypoint/subcommands/subcommands.go	75.0%	73.7%	-1.3
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.8%	89.3%	0.5

tekton-robot · 2021-12-07T22:11:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	70.0%
cmd/entrypoint/subcommands/subcommands.go	75.0%	73.7%	-1.3
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.8%	89.3%	0.5

docs/developers/README.md

afrittoli · 2021-12-14T17:47:38Z

/test check-pr-has-kind-label

ghost

Some small comments but otherwise I think this looks ready to merge.

cmd/entrypoint/subcommands/subcommands_test.go

ghost · 2021-12-16T15:45:33Z

docs/developers/README.md

+### `/tekton/steps`
+
+`/tekton/steps` contains Step information that is intended to be used by user
+steps (and is therefore covered by the Tekton API compatibility policy).


Tiny nit to close the loop here: remove the remaining parenthetical in this commit describing /tekton/steps as covered ("and is therefore covered by the Tekton API compatibility policy").

docs/developers/README.md

This change symlinks /tekton/step folders to a step's corresponding /tekton/run folder. This is an incremental change to lock down the /tekton folder to prevent tampering of exitCode files from other steps. Note: this does not completely protect against a step from tampering from its own output - more work will needed in a future PR to fully lock this down, but this is a step in the right direction (and a complete fix will likely require a more involved design). While /tekton/steps is now considered an implementation detail and could potentially be removed, the folder is preserved for now to limit the scope of this PR. - Moves `exitCode` output to `/tekton/run/<step #>/status` - Symlinks `/tekton/steps/<step #>` and `/tekton/steps/<step name>` to `/tekton/run/<step #>/status`. - Creates new `tekton-init` entrypoint subcommand to initialize the Tekton step directory. - Removes `-step_metadata_dir_link` flag from the main entrypoint binary (this behavior is now handled by the initcontainer). Co-authored-by: Lee Bernick <leebernick@google.com>

tekton-robot · 2021-12-16T16:12:52Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/post_writer.go	77.8%	70.0%	-7.8
cmd/entrypoint/subcommands/step_init.go	Do not exist	70.0%
cmd/entrypoint/subcommands/subcommands.go	75.0%	73.7%	-1.3
pkg/entrypoint/entrypointer.go	71.0%	70.6%	-0.4
pkg/pod/pod.go	88.8%	89.3%	0.5

ghost · 2021-12-16T17:03:42Z

/lgtm

wlynch · 2021-12-16T17:05:32Z

/remove-kind cleanup

wlynch · 2021-12-16T17:05:37Z

/kind feature

dlorenc · 2021-12-23T14:49:55Z

/kind feature

wlynch · 2022-01-04T15:59:12Z

/test pull-tekton-pipeline-alpha-integration-tests

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Nov 3, 2021

tekton-robot requested review from jerop and pritidesai November 3, 2021 18:59

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 3, 2021

wlynch changed the title ~~Lockdown /tekton/step folders to their own tasks.~~ Lockdown /tekton/step folders to their own steps. Nov 3, 2021

wlynch force-pushed the step-isolation branch from d7821cc to 808897a Compare November 3, 2021 19:00

wlynch force-pushed the step-isolation branch from 808897a to 24a646b Compare November 3, 2021 19:31

wlynch force-pushed the step-isolation branch from 24a646b to cd4de82 Compare November 3, 2021 19:36

ghost reviewed Nov 4, 2021

View reviewed changes

docs/developers/README.md Show resolved Hide resolved

bobcatfish reviewed Nov 4, 2021

View reviewed changes

wlynch force-pushed the step-isolation branch from cd4de82 to 8267dd1 Compare November 12, 2021 20:42

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2021

vdemeester approved these changes Nov 23, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2021

wlynch mentioned this pull request Nov 24, 2021

Fix conflicting docs for /tekton/steps API Compatibility. #4395

Merged

5 tasks

afrittoli reviewed Dec 2, 2021

View reviewed changes

wlynch force-pushed the step-isolation branch from 8267dd1 to 95cd6c4 Compare December 7, 2021 18:16

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2021

wlynch force-pushed the step-isolation branch from 74f0fce to 342dff6 Compare December 7, 2021 20:16

wlynch force-pushed the step-isolation branch from 342dff6 to af7caed Compare December 7, 2021 22:08

ghost mentioned this pull request Dec 13, 2021

/tekton/debug scripts absent after step has failed #4224

Closed

waveywaves mentioned this pull request Dec 14, 2021

Breakpoint exit status code read from /tekton/run/<step-no>/out.breakpointexit and not /tekton/run/<step-no>/status #4422

Closed

waveywaves reviewed Dec 14, 2021

View reviewed changes

docs/developers/README.md Show resolved Hide resolved

ghost reviewed Dec 16, 2021

View reviewed changes

wlynch force-pushed the step-isolation branch from af7caed to e9ea7ec Compare December 16, 2021 16:09

wlynch mentioned this pull request Dec 16, 2021

subcommands_test: Group command tests with t.Run. #4437

Merged

5 tasks

tekton-robot assigned ghost Dec 16, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2021

tekton-robot removed the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 16, 2021

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 16, 2021

dlorenc removed the kind/feature Categorizes issue or PR as related to a new feature. label Dec 23, 2021

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 23, 2021

tekton-robot merged commit e6399ce into tektoncd:main Jan 4, 2022

SaschaSchwarze0 mentioned this pull request Mar 7, 2022

March 7, 2022 Community Meeting shipwright-io/community#65

Closed

afrittoli mentioned this pull request Mar 9, 2022

Can the "place-tools" and "place-scripts" init-containers merge into one init-container? #4519

Open

wlynch mentioned this pull request Mar 10, 2022

Restrict step volumes as read-only #4227

Closed

3 tasks

lbernick mentioned this pull request Jun 17, 2022

Add wlynch to entrypoint OWNERS #4999

Merged

3 tasks

	The following directories are covered by the
	[Tekton API Compatibility policy](../api_compatibility_policy.md), and can be
	relied on for stability:

	- `/tekton/results`
	- `/tekton/steps`

Lockdown /tekton/step folders to their own steps. #4352

Lockdown /tekton/step folders to their own steps. #4352

Conversation

wlynch commented Nov 3, 2021 • edited Loading

Changes

Things to bikeshed:

Submitter Checklist

Release Notes

tekton-robot commented Nov 3, 2021

tekton-robot commented Nov 3, 2021

tekton-robot commented Nov 3, 2021

tekton-robot commented Nov 3, 2021

wlynch commented Nov 4, 2021

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlynch commented Nov 9, 2021

tekton-robot commented Nov 12, 2021

tekton-robot commented Nov 23, 2021

afrittoli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlynch Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Dec 7, 2021

tekton-robot commented Dec 7, 2021

tekton-robot commented Dec 7, 2021

afrittoli commented Dec 14, 2021

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Dec 16, 2021

ghost commented Dec 16, 2021

wlynch commented Dec 16, 2021

wlynch commented Dec 16, 2021

dlorenc commented Dec 23, 2021

wlynch commented Jan 4, 2022

wlynch commented Nov 3, 2021 •

edited

Loading

wlynch Dec 7, 2021 •

edited

Loading