Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Prompt users when cloning Git repositories using an invalid SSH key #995

Merged
merged 16 commits into from
Feb 23, 2021

Conversation

vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Feb 16, 2021

What does this PR do?

Allows the workspace plugin to handle events when cloning Git projects by SSH uri
When the devfile contain GIT SSH project, the workspace plugin

  • performs the secure login to Git repository
  • in case of GitHub repository, proposes the user to upload public key
  • proposes the user to add auto-generated key to the Git repository by hands
  • allows the user to use its custom certificate

Screenshot/screencast of this PR

Screenshot from 2021-02-22 14-19-56

Screenshot from 2021-02-22 14-20-04

What issues does this PR fix or reference?

eclipse-che/che#13494

HappyPath tests related on SSH

How to test this PR?

Devfile to test

---
apiVersion: 1.0.0
metadata:
  generateName: clone-ssh-
projects:
  -
    name: che-theia
    source:
      type: git
      location: "git@github.com:eclipse/che-theia.git"
  -
    name: atlascode
    source:
      type: git
      location: "git@bitbucket.org:atlassianlabs/atlascode.git"
components:
  - type: cheEditor
    alias: che-theia
    reference: https://raw.githubusercontent.com/chepullreq4/pr-check-files/master/che-theia/pr-995/che_theia_meta.yaml
    memoryLimit: 2G

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=stable

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Feb 16, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@che-bot
Copy link
Contributor

che-bot commented Feb 16, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Feb 16, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

plugins/workspace-plugin/src/theia-commands.ts Outdated Show resolved Hide resolved
plugins/workspace-plugin/src/git.ts Outdated Show resolved Hide resolved
vitaliy-guliy and others added 2 commits February 17, 2021 11:15
Co-authored-by: Eric Williams <ericwill@redhat.com>
Co-authored-by: Eric Williams <ericwill@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Feb 17, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@sunix
Copy link
Contributor

sunix commented Feb 17, 2021

Upload Private Key

It is not clear that it is to upload a private key to the che workspace. What about Upload Key Pair ? to be consistant with the other commands.

Upload existing key to Github

What about Add existing key to Github ?

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@vitaliy-guliy
Copy link
Contributor Author

Upload Private Key

It is not clear that it is to upload a private key to the che workspace. What about Upload Key Pair ? to be consistant with the other commands.

Probably yes, but as I understand, the command must upload a private key from your local machine and register it in the system(your workspace) with ssh-add.
I don't remember, have we discussed that or not, but you could want to generate a keypair on your machine and then use it in Che. You will be able to upload your private key to your workpspace and add correspodring public key to your Git account.

Upload existing key to Github

What about Add existing key to Github ?

Renamed, thanks.

@che-bot
Copy link
Contributor

che-bot commented Feb 17, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@sunix
Copy link
Contributor

sunix commented Feb 17, 2021

Probably yes, but as I understand, the command must upload a private key from your local machine and register it in the system(your workspace) with ssh-add.
I don't remember, have we discussed that or not, but you could want to generate a keypair on your machine and then use it in Che. You will be able to upload your private key to your workpspace and add correspodring public key to your Git account.

Usually, you generate a private key and a public key (key pair). upload the public key to github. But they generally come together in the generation phase.

Generating public/private ed25519 key pair.

I would recommend to use the same terminology here https://docs.github.com/en/github/authenticating-to-github/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent.

So what about in all the commands related to view/remove/upload commands, talking about public/private key pair ?

@sunix
Copy link
Contributor

sunix commented Feb 17, 2021

BTW what is the difference between Generate Key pair, Create key pair, and upload private key ?

@vitaliy-guliy
Copy link
Contributor Author

BTW what is the difference between Generate Key pair, Create key pair, and upload private key ?

It seems Create key pair creates key pair from the content, provided by user. You have to enter content of public and private keys. @vinokurig am I right? It's pretty similar to Upload Private Key.., but creates a pair instead of uploading and registering one file.

Generate Key pair creates a new SSH key using ssh-keygen.

Upload Private Key uploads and registers your private key (without creating a key pair).

@vinokurig
Copy link
Contributor

vinokurig commented Feb 18, 2021

It will appear if you click Configure SSH. It did so intentionally to not overload the popup.

I know, I am talking about having an action to add the key to GitHub IN the notification, if the url is GitHub, instead of the list of commands. My point is that we already have a predefined SSH key so why not to use it? BTW when we added this predefined key the idea was to help unexperienced users and propose them to add the ssh key in the notification when they clone projects via SSH.

@vitaliy-guliy
Copy link
Contributor Author

It will appear if you click Configure SSH. It did so intentionally to not overload the popup.

I know, I am talking about having an action to add the key to GitHub IN the notification, if the url is GitHub, instead of the list of commands. My point is that we already have a predefined SSH key so why not to use it? BTW when we added this predefined key the idea was to help unexperienced users and propose them to add the ssh key in the notification when they clone projects via SSH.

I think inexperienced user will retry cloning and then click on Configure SSH as there are no more buttons (except button to close the popup).
He could want to play a bit with SSH before uploading the key to GH, isn't he?

@vitaliy-guliy
Copy link
Contributor Author

@l0rd since you requested the SSH plugin some time ago, it would be nice to see your opinion.
WDYT about having a button to upload an SSH key to the GitHub next to Configure SSH button?

We are discussing here two ways

  • not to overload the notification with dozen of buttons and use menu/command palette to manage SSH keys

configure-ssh

  • do the same as above, but add 'Add Key To GitHub' when cloning from GH

configure-ssh

@vitaliy-guliy
Copy link
Contributor Author

@vitaliy-guliy I expect that you give a try with these different flow:

User starts a workspace with a devfile and a ssh github url
Flow#1: User wants Che to generate a new key pair and upload the public key to github
Flow#2: User wants to add his private key from his machine: upload the file(s). He has already uploaded the public key to github.
Flow#3: User wants to add his private key from his machine: copy paste to an editor. He has already uploaded the public key to github.

For flow#2 and flow#3, these are pretty similar. We have to see if keys with passphrase working in Che or not. If not, create a gh-issue to fix that.
Also flow#2 and flow#3, check that upload/add private key is enough (don't need to upload a public key?)

Uploading of private keyfile does not work as expected. In fact, the file is uploaded, but need to investigate what's going when the plugins tries to register it with ssh-add.
I see that a keypair is created, but seems it has a wrong content.

See the issue eclipse-che/che#19109 where I explained the problem and steps to reproduce.
(need additional time to fix that ^^)

BTW, when I uploaded and registered the file by hand, I was able to clone the repository. So, the flow is working. Just need to fix the bug.

SSH: Create Key... command is also not working. It creates a key from the content, provided by user, but it's not possible to clone the git repository. I think we will fix that withing fixing the bug with uploading of private key.

@l0rd
Copy link
Contributor

l0rd commented Feb 19, 2021

@vitaliy-guliy I like the option with the "Add Key to GitHub" cc @sympatheticmoose

@sunix
Copy link
Contributor

sunix commented Feb 19, 2021

If someone has a git ssh url in the devfile, we expect him to understand these kind of doc: https://docs.github.com/en/github/authenticating-to-github/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent

For those people, giving the list of actions to perform in case of failure is the way to go.

We can't neither be too specific to Github and cover all the flows with bitbucket or any others private or public git repos.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@sunix
Copy link
Contributor

sunix commented Feb 19, 2021

Also, in most of the cases, Github users would get through https, not ssh.
With https urls, they would be already logged in thanks to Github Oauth to perform any push/clone to github repos even the private ones. We just need to use the right token when cloning (the same one we use to upload a ssh key anyway).

Ssh could be widely used in private git repos with onprem github/bitbucket/gitlab/other. So let's not hard coding this.

@che-bot
Copy link
Contributor

che-bot commented Feb 19, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Feb 19, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@vitaliy-guliy
Copy link
Contributor Author

[crw-ci-test]

@vitaliy-guliy
Copy link
Contributor Author

[crw-ci-test --rebuild]

@che-bot
Copy link
Contributor

che-bot commented Feb 22, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@af8b33d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #995   +/-   ##
=========================================
  Coverage          ?   57.93%           
=========================================
  Files             ?       45           
  Lines             ?     2028           
  Branches          ?      333           
=========================================
  Hits              ?     1175           
  Misses            ?      843           
  Partials          ?       10           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af8b33d...b006e8b. Read the comment docs.

@che-bot
Copy link
Contributor

che-bot commented Feb 23, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:995
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@vinokurig
Copy link
Contributor

I confirm that the ssh notifications buttons work as expected.

monaka pushed a commit to PizzaFactory/che-theia that referenced this pull request Mar 6, 2021
* Improve the UX when cloning Git projects with SSH uri

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@l0rd l0rd changed the title Clone Git projects with SSH URI Better support when git cloning using SSH URI Apr 19, 2021
@l0rd l0rd changed the title Better support when git cloning using SSH URI Better support when git cloning using SSH UR Apr 19, 2021
@che-bot
Copy link
Contributor

che-bot commented Apr 19, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia quay.io/crw_pr/che-theia:995
che-theia-endpoint-runtime-binary quay.io/crw_pr/che-theia-endpoint-runtime-binary:995

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@l0rd l0rd changed the title Better support when git cloning using SSH UR Prompt users when cloning Git repositories using an invalid SSH key Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants