Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove ssh-keyscan from git credential initialization #2953

Merged
merged 1 commit into from Jul 30, 2020
Merged

Remove ssh-keyscan from git credential initialization #2953

merged 1 commit into from Jul 30, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jul 16, 2020 •
edited by ghost

Changes

Fixes #2951

Before this change an SSH Git Secret without a known_hosts would cause Tekton to perform an ssh-keyscan of the remote host to get its public key. This would happen in the entrypoint of the Step which meant that every Step container would have to have a copy of ssh-keyscan.

After this change the reponsibility for fetching missing host keys now falls on git-init, meaning that the Git PipelineResource and git-clone catalog task will fetch the missing keys at runtime. This is preferable because we can guarantee that the git-init image has ssh in it. We no longer rely on ssh-keyscan at all and instead use ssh's StrictHostKeyChecking=accept-new option.

Need tests/examples to:

  • Confirm that omitting known_hosts does not result in ssh-keyscan erroring out a TaskRun.
  • Confirm that omitting known_hosts does result in git-init running with StrictHostKeyChecking=accept-new
  • Confirm that including known_hosts doesnt result in StrictHostKeyChecking=accept-new.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

Release Notes

Tekton's creds-init initContainer has been removed and will no longer show up in TaskRun Pods. Credentials from annotated Secrets will continue to be mounted and placed in Step containers' home directory.

Further, previously a missing known_hosts entry from a Git SSH Secret would result in ssh-keyscan being executed to fetch a server's public key. That behaviour has been replaced with passing "-o StrictHostKeyChecking=accept-new" to ssh when using a Git PipelineResource or the git-clone Catalog Task. The ultimate result should be the same - the public key of the server is accepted without question - but the location where this work is performed has changed.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 16, 2020
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 16, 2020
@ghost
Copy link
Author

ghost commented Jul 16, 2020

/kind bug

/hold

needs tests

@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/bug Categorizes issue or PR as related to a bug. labels Jul 16, 2020
@tekton-robot
Copy link
Collaborator

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
pkg/credentials/gitcreds/ssh.go 73.0% 89.8% 16.9

@tekton-robot
Copy link
Collaborator

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
pkg/credentials/gitcreds/ssh.go 73.0% 89.8% 16.9

@ghost ghost added this to the Pipelines v0.15 milestone Jul 17, 2020
@tekton-robot
Copy link
Collaborator

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
pkg/credentials/gitcreds/ssh.go 73.0% 89.8% 16.9

@ghost
Copy link
Author

ghost commented Jul 20, 2020

/hold cancel

In order to test these changes I've updated an existing example and added a new one. It's actually kinda hard to write unit tests for our credential code that calls out to git. As an example, our git.Fetch func is relatively long and calls out to the filesystem in lots of different ways that would be quite brittle to mock I think. This is probably worth some deeper investigation so I've created #2974 to get some work done on that.

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2020
@ghost
Copy link
Author

ghost commented Jul 20, 2020

/test pull-tekton-pipeline-integration-tests

1 similar comment
@ghost
Copy link
Author

ghost commented Jul 20, 2020

/test pull-tekton-pipeline-integration-tests

vdemeester
vdemeester reviewed Jul 21, 2020
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. Should we have a "config" to disallow the "accept-new" (if the operator of tekton-pipelines wants to force users to provide ssh known_hosts) ?

@ghost
Copy link
Author

ghost commented Jul 21, 2020

Seems good to me. Should we have a "config" to disallow the "accept-new" (if the operator of tekton-pipelines wants to force users to provide ssh known_hosts) ?

Good thinking - I've created #2981 to capture the feature request.

@dlorenc
Copy link
Contributor

dlorenc commented Jul 21, 2020

Should we include something about the change in behavior of ssh-keyscan in the release notes?

@ghost
Copy link
Author

ghost commented Jul 21, 2020

Updated the release note to call it out.

vdemeester
vdemeester reviewed Jul 24, 2020
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/cc @dlorenc @bobcatfish @afrittoli

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2020
@ghost
Copy link
Author

ghost commented Jul 28, 2020

ping; would be great to get this in as part of the 0.15 milestone. if that's not feasible I'll rollback the creds-init change and try again in 0.16.

Remove ssh-keyscan from git credential initialization
c20c21b 
Before this change an SSH Git Secret without a known_hosts would cause Tekton
to perform an ssh-keyscan of the remote host to get its public key. This would
happen in the entrypoint of the Step which meant that every Step container would
have to have a copy of ssh-keyscan.

After this change the reponsibility for fetching missing host keys
now falls on git-init, meaning that the Git PipelineResource and git-clone
catalog task will fetch the missing keys at runtime. This is preferable because
we can guarantee that the git-init image has ssh-keyscan in it.
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020
@tekton-robot
Copy link
Collaborator

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
pkg/credentials/gitcreds/ssh.go 73.7% 88.5% 14.8

@vdemeester
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020
@dlorenc
Copy link
Contributor

dlorenc commented Jul 30, 2020

/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dlorenc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2020
@tekton-robot tekton-robot merged commit f112277 into tektoncd:master Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afrittoli afrittoli Awaiting requested review from afrittoli

@imjasonh imjasonh Awaiting requested review from imjasonh

@bobcatfish bobcatfish Awaiting requested review from bobcatfish

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees

@vdemeester vdemeester

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
Pipelines v0.15
Development

Successfully merging this pull request may close these issues.

"ssh-keyscan": executable file not found in $PATH after upgrading from v0.13.2 to v0.14.1
3 participants
@tekton-robot @dlorenc @vdemeester