Remove creds-init initContainer #2671
Conversation
tekton-robot
added
do-not-merge/hold
tekton-robot
added
the
size/XL
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
I am +:100: for this change :heart_eyes_cat: |
|
I wonder if we can use a gitolite deployment spinned just before running the tests with dynamically generated ssh keys for testing, I mean instead of having to deal with private external github repositories? |
I've managed to spin up an example which doesn't need the private external repo - see |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
ghost
mentioned this pull request
In order to test Pipelines credentials in environments without root users (i.e. in openshift clusters, or other more locked-down envs) we need a container image that has a non-root user along with a tool to exercise the credentials. This commit adds an alpine-git-nonroot image to our plumbing repo for use in testing credentials in non-root environments. It uses alpine/git as its base and then adds a user called nonroot with UID 1000 to it. This image's first intended use is as a Step in an example TaskRun that will test creds-init credentials with non-root securityContext. That PR is ongoing here: tektoncd/pipeline#2671
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/hold cancel OK, I think this PR is ready for review. It may be worth taking a look at the I've also added some short docs to try and help users out navigating the quagmire. It's definitely not easy to wrap one's head around unfortunately. |
tekton-robot
removed
the
do-not-merge/hold
|
The following is the coverage report on the affected files.
|
@bobcatfish You might be looking at an older report? The latest numbers look positive to me:
|
Whoops, sorry about that @sbwsg , you're totally right! |
tekton-robot
added
do-not-merge/release-note-label-needed
💯 I love it. /lgtm |
Creds-init is an initContainer that writes credential files (like .ssh) to /tekton/home. One side-effect of relying on an initContainer to write these files is that they receive the UID / Group ID of the initContainer's process. This side-effect breaks any Step in the Task that relies on these credentials but runs with a different UID. An example of where this can happen is on OpenShift, where the UID of the user is randomized for each container in order to limit the fallout of malicious process breaking out. This commit removes the creds-init initContainer from all TaskRun Pods. I haven't removed the creds-init binary from our build process in this changeset. Doing so generates a lot of extra line noise which distracts from the core modification being presented here. This commit introduces an example YAML that successfully exercises creds-init with vanilla git commands both when the disable-home-env-overwrite flag is "false" (the current default) and when it's "true". In addition the example demonstrates working with creds-init credentials when a non-root securityContext is set on a Step.
|
I just rebase it against master and fix the conflict on the |
|
The following is the coverage report on the affected files.
|
| @@ -375,6 +379,28 @@ Credential annotation keys must begin with `tekton.dev/docker-` or | |||
| `tekton.dev/git-`, and the value describes the URL of the host with which to use | |||
| the credential. | |||
|
|
|||
| ## Using credentials as non-root user | |||
| annotations: | ||
| tekton.dev/git-0: localhost | ||
| data: | ||
| # This key was generated for this test and isn't used for anything else. |
| - name: messages | ||
| mountPath: /messages | ||
| script: | | ||
| #!/usr/bin/env ash |
There was a problem hiding this comment.
oh, never mind, I didn't know about ash
| // resolves to a single place with all the stored credentials. | ||
| builders := []credentials.Builder{dockercreds.NewBuilder(), gitcreds.NewBuilder()} | ||
| for _, c := range builders { | ||
| if err := c.Write("/tekton/creds"); err != nil { |
There was a problem hiding this comment.
NIT: I wonder if "/tekton/creds" should be set as a const instead of being embedded here.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: afrittoli 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/test pull-tekton-pipeline-integration-tests |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
ghost
mentioned this pull request
This reverts commit bbb767c. In #2671 I removed the creds-init initContainer from Task Pods so that credentials could be used by containers running with non-root users. The intention was for this change to be free of any side-effects to end-users. Unfortunately a [backwards incompatible issue](#2951) has cropped up with this change: When a user does not specify the `known_hosts` field in a creds-init Secret, the credential code will perform an `ssh-keyscan` of the remote server to get its public key. The problem is that previously we could guarantee `ssh-keyscan` was available since the code ran in our own creds-init container with our own docker image. Since we've now moved that code into Steps' entrypoint the Steps container is required to provide `ssh-keyscan`. This is a change in container contract and therefore backwards-incompatible. In this PR I've reverted the creds-init change for the 0.14 branch rather than attempt to fix the `ssh-keyscan` issue and possibly introduce more problems. Before 0.15 I'd like to get a better backwards-compatible fix organized. So I plan to leave the creds-init change in place in the `master` branch for the time being.
ghost
deleted the
Changes
Fixes #2524 and puts us in a better position to flip the
disable-home-env-overwritefeature flag with less fallout for users.Fixes #1271 when the feature-flag is flipped. Also we have documentation on how to use SSH creds without the feature flag flip in auth.md now too.
TLDR; The behaviour that used to be implemented by creds-init is now performed by
the entrypointer.
Creds-init is an initContainer that writes credential files (like .ssh) to
/tekton/home. One side-effect of relying on an initContainer to write these
files is that they receive the UID / Group ID of the initContainer's process.
This side-effect breaks any Step in the Task that relies on these credentials
but runs with a different UID. An example of where this can happen is on
OpenShift, where the UID of the user is randomized for each container in
order to limit the fallout of malicious process breaking out.
This commit removes the creds-init initContainer from all TaskRun Pods. I
haven't removed the creds-init binary from our build process in this
changeset. Doing so generates a lot of extra line noise which distracts
from the core modification being presented here.
Further fallout from this change:
in the
creds-initinitContainer, at/tekcon/creds-secrets/.... These volume mountsare now given to every Step container so that they can be copied out by the entrypointer.
/tekton/creds-secrets/...to/tekton/homeor
/tekton/creds, depending on whether thedisable-home-env-overwritefeature flagwas set. Now those credentials are always copied into
/tekton/creds.$(credentials.path)variable is now always/tekton/credsregardless of whetherthe
disable-home-env-overwritefeature flag is set or not.disable-home-env-overwritefeature flag is set, credentials will be placed atthe actual home directory of the current user in the Step container, instead of being placed
in
/tekton/home. If the container or Step explicitly sets a$HOMEthen credentials willend up in there.
/tekton/credsvolume. This allows/tekton/credsto be world-writable and empty for every Step to copy in to. The directory needs to be world-writable so it's accessible even if the user's UID wouldn't normally have permissions to create a directory under/tekton. This is also why there are so many lines changed in the variouspkg/podtests.I've put a hold on this while I add better testing and examples that exercise the cases that this PR deals with.
Need to figure out a safe way to introduce a credential that can be used for testing. Possibly aread-onlyGithub Deploy Secret for a private test Github repo in the tektoncd org?Edit: It took me way too long but I finally figured out how to spin up a git/ssh sidecar that works as an authenticated remote repo for testing creds-init.
securityContexts per Step.gitcommands, not git-init or Git PipelineResource.securityContexts.I think there also needs to be a bit of discussion about some of the more questionable design decisions. Maybe next API WG.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes
/kind bug
/hold