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.

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

OCPBUGS-4811: New-App Using Git via SSH #1269

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

otaviof
Copy link
Member

@otaviof otaviof commented Oct 19, 2022

When a user tries to create a new-app with a SSH based repository, oc fails on not being able to git ls-remote --heads due authentication error, i.e.:

$ oc new-app --v=8 --source-secret=github-repo-key --name=test-app 'git@github.com:otaviof/nodejs-ex.git'
I1019 09:41:10.701737   24709 loader.go:374] Config loaded from file:  /home/otaviof/.kube/config
I1019 09:41:10.702768   24709 sourcelookup.go:316] Checking if git@github.com:otaviof/nodejs-ex.git requires authentication
I1019 09:41:10.702789   24709 repository.go:450] Executing git ls-remote --heads git@github.com:otaviof/nodejs-ex.git
I1019 09:41:10.702811   24709 repository.go:459] Environment:
I1019 09:41:10.702818   24709 repository.go:461] - HOME=/tmp/githome2802467764
I1019 09:41:10.702825   24709 repository.go:461] - GIT_SSH=/dev/null
I1019 09:41:10.702832   24709 repository.go:461] - GIT_CONFIG_NOSYSTEM=true
I1019 09:41:10.702837   24709 repository.go:461] - GIT_ASKPASS=true
I1019 09:41:10.704143   24709 repository.go:541] Error executing command: exit status 128
warning: Cannot check if git requires authentication.
[...]
See 'oc new-app -h' for examples.
exit status 1

This pull request removes the hardcoded GIT_SSH=/dev/null allowing Git to use the local SSH-Agent:

$ oc new-app --v=8 --source-secret=github-repo-key --name=test-app 'git@github.com:otaviof/nodejs-ex.git'
I1019 09:35:20.560638   23876 loader.go:374] Config loaded from file:  /home/otaviof/.kube/config
I1019 09:35:20.561705   23876 sourcelookup.go:315] Checking if git@github.com:otaviof/nodejs-ex.git requires authentication
I1019 09:35:20.561722   23876 repository.go:450] Executing git ls-remote --heads git@github.com:otaviof/nodejs-ex.git
I1019 09:35:20.561742   23876 repository.go:459] Environment:
I1019 09:35:20.561747   23876 repository.go:461] - HOME=/tmp/githome2424980829
I1019 09:35:20.561750   23876 repository.go:461] - GIT_CONFIG_NOSYSTEM=true
I1019 09:35:20.561753   23876 repository.go:461] - GIT_ASKPASS=true
I1019 09:35:21.892018   23876 sourcelookup.go:94] git@github.com:otaviof/nodejs-ex.git is a valid remote git repository
I1019 09:35:21.892044   23876 newapp.go:318] treating git@github.com:otaviof/nodejs-ex.git as possible source repo
[...]
--> Success
    Build scheduled, use 'oc logs -f buildconfig/test-app' to track its progress.
    Application is not exposed. You can expose services to the outside world by executing one or more of the commands below:
     'oc expose service/test-app'
    Run 'oc status' to view your app.

Removing `GIT_SSH=/dev/null` to allow using SSH based authentication.
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2022

@otaviof: Bugzilla bug 2078694 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • nec
  • qe_staff
  • redhat

In response to this:

Bug 2078694: New-App Using Git via SSH

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.

@otaviof
Copy link
Member Author

otaviof commented Oct 19, 2022

/assign @coreydaley

@otaviof
Copy link
Member Author

otaviof commented Oct 19, 2022

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2022

@otaviof: Bugzilla bug 2078694 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • nec
  • qe_staff
  • redhat

In response to this:

/bugzilla refresh

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.

@otaviof
Copy link
Member Author

otaviof commented Oct 19, 2022

/test e2e-aws-serial
/test e2e-aws-builds

@coreydaley
Copy link
Member

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 19, 2022

@coreydaley: Bugzilla bug 2078694 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • nec
  • qe_staff
  • redhat

In response to this:

/bugzilla refresh

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.

@coreydaley
Copy link
Member

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2022
@otaviof
Copy link
Member Author

otaviof commented Oct 19, 2022

/test e2e-aws-builds
/test e2e-aws-serial

@coreydaley
Copy link
Member

/retest

@otaviof
Copy link
Member Author

otaviof commented Oct 20, 2022

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2022

@otaviof: Bugzilla bug 2078694 is in a bug group that is not in the allowed groups for this repo.
Allowed groups for this repo are:

  • nec
  • qe_staff
  • redhat

In response to this:

/bugzilla refresh

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.

@otaviof
Copy link
Member Author

otaviof commented Oct 20, 2022

/assign @coreydaley @gabemontero

Gabe, would you please review this PR? Thanks in advance.

Comment on lines -278 to -282
// 1) The HOME directory is set to a temporary dir to avoid loading any settings in .gitconfig
// 2) The GIT_SSH variable is set to /dev/null so the regular SSH keys are not used
// (changing the HOME directory is not enough).
// 3) GIT_CONFIG_NOSYSTEM prevents git from loading system-wide config
// 4) GIT_ASKPASS to prevent git from prompting for a user/password
Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, the changes in this block are due to gofmt.

@coreydaley
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: coreydaley, otaviof

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

@otaviof
Copy link
Member Author

otaviof commented Oct 20, 2022

/retest

@gabemontero
Copy link
Contributor

/assign @coreydaley @gabemontero

Gabe, would you please review this PR? Thanks in advance.

hey @otaviof - first, I must admit I'm worried that either bad memory or not being what I would consider an SME in how SSH clients work might render my review of little value.

I did look back at git history and saw I touched this file as part of https://bugzilla.redhat.com/show_bug.cgi?id=2052578

maybe that change and calling DetectAuth uncovered whatever symptom you are now trying to address? Is that what you are seeing and why you asked for my input?

In any event, I suppose that as long as the ssh scenarios noted in the bz there still works, I think this is OK. For reference, from that bz's description:

3. Using `ssh-auth` instead of `basic-auth` as authentication method works:
   3.1 ssh-keygen -f git-cred -N ''
   3.2 oc create secret generic git-cred --type=kubernetes.io/ssh-auth --from-file=ssh-privatekey=${PWD}/git-cred.pub
   3.3 oc secrets link builder git-cred
   3.4 from GitHub repository > Settings > Deploy Keys > Add Deploy Key (paste the content of the file git-cred.pub)
   3.5 oc new-app git:<username>/<repo_name>.git --source-secret git-cred

That looks like the same usage you are trying now though in the PR description here, right? If that BZ fix broke that SSH scenario though, I don't remember seeing that during testing, nor the user reporting that basic auth was now OK, but SSH was broke.

Doing git-blame on the file, that /dev/null was added 6 years ago via ace8cab from @csrwng ... maybe he remembers why he added that env var then?

Otherwise, I suppose by removing GIT_SSH, per the doc for GIT_SSH at https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables your intent was to fall back to ~/.ssh/config, which in an new-app scenario, I think makes sense and is OK, as long as the ssh key is in the secret so that the build has the same key new-app has on your local machine.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 20, 2022

@otaviof: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit a4cfdc7 into openshift:master Oct 20, 2022
@otaviof
Copy link
Member Author

otaviof commented Oct 21, 2022

/assign @coreydaley @gabemontero
Gabe, would you please review this PR? Thanks in advance.

hey @otaviof - first, I must admit I'm worried that either bad memory or not being what I would consider an SME in how SSH clients work might render my review of little value.

I did look back at git history and saw I touched this file as part of https://bugzilla.redhat.com/show_bug.cgi?id=2052578

maybe that change and calling DetectAuth uncovered whatever symptom you are now trying to address? Is that what you are seeing and why you asked for my input?

Yes, that's correct, Gabe. The bug this pull-request is addressing (BZ-2078694), the issue happends in that method.

In any event, I suppose that as long as the ssh scenarios noted in the bz there still works, I think this is OK. For reference, from that bz's description:

3. Using `ssh-auth` instead of `basic-auth` as authentication method works:
   3.1 ssh-keygen -f git-cred -N ''
   3.2 oc create secret generic git-cred --type=kubernetes.io/ssh-auth --from-file=ssh-privatekey=${PWD}/git-cred.pub
   3.3 oc secrets link builder git-cred
   3.4 from GitHub repository > Settings > Deploy Keys > Add Deploy Key (paste the content of the file git-cred.pub)
   3.5 oc new-app git:<username>/<repo_name>.git --source-secret git-cred

That looks like the same usage you are trying now though in the PR description here, right? If that BZ fix broke that SSH scenario though, I don't remember seeing that during testing, nor the user reporting that basic auth was now OK, but SSH was broke.

Doing git-blame on the file, that /dev/null was added 6 years ago via ace8cab from @csrwng ... maybe he remembers why he added that env var then?

Otherwise, I suppose by removing GIT_SSH, per the doc for GIT_SSH at https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables your intent was to fall back to ~/.ssh/config, which in an new-app scenario, I think makes sense and is OK, as long as the ssh key is in the secret so that the build has the same key new-app has on your local machine.

The issue happens during the oc execution, it tries to run git ls-remote --heads and it fails when the GIT_SSH variable points to the /dev/null. A simple way of reproducing this behavior is the following:

$ ssh-add -l
4096 SHA256:qrzpseAqTLDuNDQ9x6MDei2wGQqz4J1LBkdHVip9EGI /home/otaviof/.ssh/id_rsa (RSA)

$ git ls-remote --heads git@github.com:otaviof/nodejs-ex.git
77059f8edae03f2184b61cd321e21e540a50dbf2        refs/heads/main

$ GIT_SSH=/dev/null git ls-remote --heads git@github.com:otaviof/nodejs-ex.git
fatal: cannot exec '/dev/null': Permission denied
fatal: cannot exec '/dev/null': Permission denied
fatal: unable to fork

Thanks for sharing the official Git documentation about GIT_SSH, let me quote it here:

GIT_SSH, if specified, is a program that is invoked instead of ssh when Git tries to connect to an SSH host

So, indeed, I think the best way is to remove GIT_SSH from oc, thus we don't interfere in the user's SSH settings.

Thanks for the comments and references!

@coreydaley
Copy link
Member

/retitle OCPBUGS-4811: New-App Using Git via SSH

@openshift-ci openshift-ci bot changed the title Bug 2078694: New-App Using Git via SSH OCPBUGS-4811: New-App Using Git via SSH Dec 13, 2022
@openshift-ci-robot
Copy link

@otaviof: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-4811 has been moved to the MODIFIED state.

In response to this:

When a user tries to create a new-app with a SSH based repository, oc fails on not being able to git ls-remote --heads due authentication error, i.e.:

$ oc new-app --v=8 --source-secret=github-repo-key --name=test-app 'git@github.com:otaviof/nodejs-ex.git'
I1019 09:41:10.701737   24709 loader.go:374] Config loaded from file:  /home/otaviof/.kube/config
I1019 09:41:10.702768   24709 sourcelookup.go:316] Checking if git@github.com:otaviof/nodejs-ex.git requires authentication
I1019 09:41:10.702789   24709 repository.go:450] Executing git ls-remote --heads git@github.com:otaviof/nodejs-ex.git
I1019 09:41:10.702811   24709 repository.go:459] Environment:
I1019 09:41:10.702818   24709 repository.go:461] - HOME=/tmp/githome2802467764
I1019 09:41:10.702825   24709 repository.go:461] - GIT_SSH=/dev/null
I1019 09:41:10.702832   24709 repository.go:461] - GIT_CONFIG_NOSYSTEM=true
I1019 09:41:10.702837   24709 repository.go:461] - GIT_ASKPASS=true
I1019 09:41:10.704143   24709 repository.go:541] Error executing command: exit status 128
warning: Cannot check if git requires authentication.
[...]
See 'oc new-app -h' for examples.
exit status 1

This pull request removes the hardcoded GIT_SSH=/dev/null allowing Git to use the local SSH-Agent:

$ oc new-app --v=8 --source-secret=github-repo-key --name=test-app 'git@github.com:otaviof/nodejs-ex.git'
I1019 09:35:20.560638   23876 loader.go:374] Config loaded from file:  /home/otaviof/.kube/config
I1019 09:35:20.561705   23876 sourcelookup.go:315] Checking if git@github.com:otaviof/nodejs-ex.git requires authentication
I1019 09:35:20.561722   23876 repository.go:450] Executing git ls-remote --heads git@github.com:otaviof/nodejs-ex.git
I1019 09:35:20.561742   23876 repository.go:459] Environment:
I1019 09:35:20.561747   23876 repository.go:461] - HOME=/tmp/githome2424980829
I1019 09:35:20.561750   23876 repository.go:461] - GIT_CONFIG_NOSYSTEM=true
I1019 09:35:20.561753   23876 repository.go:461] - GIT_ASKPASS=true
I1019 09:35:21.892018   23876 sourcelookup.go:94] git@github.com:otaviof/nodejs-ex.git is a valid remote git repository
I1019 09:35:21.892044   23876 newapp.go:318] treating git@github.com:otaviof/nodejs-ex.git as possible source repo
[...]
--> Success
   Build scheduled, use 'oc logs -f buildconfig/test-app' to track its progress.
   Application is not exposed. You can expose services to the outside world by executing one or more of the commands below:
    'oc expose service/test-app'
   Run 'oc status' to view your app.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants