Store SSH private key on the file system from SSH plugin #352

vinokurig · 2019-07-16T13:21:14Z

What does this PR do?

Store SSH private key file on the file system when key is generated from the SSH Plugin

What issues does this PR fix or reference?

fixes eclipse-che/che#13866

Release Notes

Docs PR

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

sunix · 2019-07-16T15:22:51Z

@vinokurig Could you describe the end user flow that would use that ?
How can I test it ? how can I set ssh keys ?

l0rd

I understand that a user will generate/create a key pair and update the ssh config in theia container. This is an operation that the user will do only once.

Then, when he delete this workspace and creates a new one, the SSH key pair will be mounted as a Kubernetes secret. But what about the SSH config? Will it be re-generated and mounted as well?

plugins/ssh-plugin/src/ssh-plugin-backend.ts

vparfonov · 2019-07-17T08:12:36Z

I understand that a user will generate/create a key pair and update the ssh config in theia container. This is an operation that the user will do only once.

Then, when he delete this workspace and creates a new one, the SSH key pair will be mounted as a Kubernetes secret. But what about the SSH config? Will it be re-generated and mounted as well?

yes, it's done here https://github.com/eclipse/che/pull/13809/files#diff-5e2165ab176b2a1fb9829aa579fa546fR139

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig · 2019-07-17T09:35:45Z

@sunix

@vinokurig Could you describe the end user flow that would use that ?
How can I test it ? how can I set ssh keys ?

When user executes command: SSH: generate key pair... the plugin generates an SSH key pair and stores it in the che database. The improvement generates a file with a private ssh key and a config file that refers the private key to host. It allows to execute git operation via SSH.
To test it you need to generate a new SSH key, store the public key in the git provider (github.com), then you can clone a repository by ssh url and push to remote.

sunix · 2019-07-17T15:30:10Z

ok so the ssh key generation work is done on wsmaster ? and we have a dedicated service on wsmaster API. I guess this is comming from che6 but it sounds weird in a Che7 architecture.
is it just for generating keys ? if yes I am not sure it has to stay on wsmaster ... it could just be a microservice that runs on a container or invoking the right command locally... just thinking out loud ...

vinokurig · 2019-07-18T06:59:44Z

@sunix

ok so the ssh key generation work is done on wsmaster ? and we have a dedicated service on wsmaster API. I guess this is comming from che6 but it sounds weird in a Che7 architecture.
is it just for generating keys ? if yes I am not sure it has to stay on wsmaster ... it could just be a microservice that runs on a container or invoking the right command locally... just thinking out loud ...

Agree, but that's a separate issue.

sunix · 2019-07-18T07:34:10Z

Agree, but that's a separate issue.

Is it an existing GH issue ? could you create one if not ?
Again thinking out loud, but it could just be invoking the right command in the local theia container

ssh-keygen -t rsa -b 4096 -C "your_email@example.com"

and we won't have to deal with the legacy problems (like having to deal with the host provider stuff)

vinokurig · 2019-07-18T07:46:39Z

Is it an existing GH issue ? could you create one if not ?

eclipse-che/che#13522

vinokurig · 2019-07-18T12:25:45Z

@l0rd Reworked the plugin according to eclipse-che/che#13494 (comment), could you please review

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

l0rd

LGTM but we need a review by someone from IDE2 team as well. @evidolob can you help with it?

plugins/ssh-plugin/src/ssh-plugin-backend.ts

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig · 2019-07-19T12:09:53Z

@benoitf I've fixed all your comments could you please review

benoitf

I found small glitches

benoitf · 2019-07-19T12:13:29Z

plugins/ssh-plugin/src/ssh-plugin-backend.ts

+};
+
+const updateConfig = async function (hostName: string): Promise<void> {
+    const sshDir = resolve(os.homedir(), '.ssh');


when I was referencing a function for ssh folder, it was for this part
resolve(os.homedir(), '.ssh')

benoitf · 2019-07-19T12:15:02Z

plugins/ssh-plugin/src/ssh-plugin-backend.ts

-    if (sshServiceValue) {
-        return Promise.resolve(sshServiceValue.label);
+const getKeyFilePath = function (name: string): string {
+    return resolve(os.homedir(), '.ssh', name.replace('.', '_'));


probably function here should be for resolve(os.homedir(), '.ssh')

also function() can be replaced by getKeyFilePath = (name:string): string => {

probably function here should be for resolve(os.homedir(), '.ssh')

We need to resolve key file name as well

benoitf · 2019-07-19T12:15:35Z

plugins/ssh-plugin/src/ssh-plugin-backend.ts

-            description: 'SSH keys injected into all Workspace Containers'
-        }
-    ];
+const getHostName = async function (): Promise<string> {


(): Promise<string> => { ?

benoitf · 2019-07-19T12:16:04Z

plugins/ssh-plugin/src/ssh-plugin-backend.ts

+    const keyConfig = `\nHost ${hostName.startsWith('default-') ? '*' : hostName}\nIdentityFile ${getKeyFilePath(hostName)}\n`;
+    const configContentBuffer = await readFile(configFile);
+    if (configContentBuffer.indexOf(keyConfig) >= 0) {
+        const newConfigContent = configContentBuffer.toString().replace(keyConfig, '');


replace function is only replacing the first occurence, it's ok ?

Yes, it is not expected that the config file has 2 identical key configs

benoitf · 2019-07-19T12:17:44Z

plugins/ssh-plugin/src/ssh-plugin-backend.ts

-        .then(key => {
-            theia.workspace.openTextDocument({ content: key.publicKey });
+        .get('vcs', keyName)
+        .then(async key => {


why do we use a promise there with then and not with await ?

const key = await sshkeyManager.get(sshServiceName, keyName) const document = await theia.workspace.openTextDocument({ content: key.publicKey }); theia.window.showTextDocument(document);

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

Store SSH private key on the file system

7b58cb1

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig requested review from benoitf, evidolob and vparfonov July 16, 2019 13:21

vinokurig requested review from olexii4, sunix and vitaliy-guliy as code owners July 16, 2019 13:21

l0rd reviewed Jul 16, 2019

View reviewed changes

plugins/ssh-plugin/src/ssh-plugin-backend.ts Outdated Show resolved Hide resolved

plugins/ssh-plugin/src/ssh-plugin-backend.ts Outdated Show resolved Hide resolved

evidolob added this to the 7.0.0 milestone Jul 17, 2019

fixup! Store SSH private key on the file system

3ce1956

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig requested a review from rhopp as a code owner July 17, 2019 09:20

fixup! Store SSH private key on the file system

0b95223

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig force-pushed the improveSshPlugin branch from a273fd7 to 0b95223 Compare July 18, 2019 12:28

l0rd approved these changes Jul 18, 2019

View reviewed changes

benoitf suggested changes Jul 18, 2019

View reviewed changes

fixup! Store SSH private key on the file system

5a9da8d

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

benoitf approved these changes Jul 19, 2019

View reviewed changes

fixup! Store SSH private key on the file system

0fa1401

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig merged commit 9d210ad into master Jul 19, 2019

vinokurig deleted the improveSshPlugin branch July 19, 2019 14:07

vinokurig mentioned this pull request Jul 19, 2019

Store SSH private key on the file system from SSH plugin #360

Merged

vinokurig mentioned this pull request Jul 22, 2019

SSH plugin doesn't show if the generation has been successful or not eclipse-che/che#13857

Closed

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store SSH private key on the file system from SSH plugin #352

Store SSH private key on the file system from SSH plugin #352

vinokurig commented Jul 16, 2019

sunix commented Jul 16, 2019

l0rd left a comment •

edited

Loading

vparfonov commented Jul 17, 2019

vinokurig commented Jul 17, 2019

sunix commented Jul 17, 2019 •

edited

Loading

vinokurig commented Jul 18, 2019

sunix commented Jul 18, 2019 •

edited

Loading

vinokurig commented Jul 18, 2019

vinokurig commented Jul 18, 2019

l0rd left a comment

vinokurig commented Jul 19, 2019

benoitf left a comment

benoitf Jul 19, 2019

benoitf Jul 19, 2019

vinokurig Jul 19, 2019

benoitf Jul 19, 2019

benoitf Jul 19, 2019

vinokurig Jul 19, 2019

benoitf Jul 19, 2019

Store SSH private key on the file system from SSH plugin #352

Store SSH private key on the file system from SSH plugin #352

Conversation

vinokurig commented Jul 16, 2019

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

sunix commented Jul 16, 2019

l0rd left a comment • edited Loading

Choose a reason for hiding this comment

vparfonov commented Jul 17, 2019

vinokurig commented Jul 17, 2019

sunix commented Jul 17, 2019 • edited Loading

vinokurig commented Jul 18, 2019

sunix commented Jul 18, 2019 • edited Loading

vinokurig commented Jul 18, 2019

vinokurig commented Jul 18, 2019

l0rd left a comment

Choose a reason for hiding this comment

vinokurig commented Jul 19, 2019

benoitf left a comment

Choose a reason for hiding this comment

benoitf Jul 19, 2019

Choose a reason for hiding this comment

benoitf Jul 19, 2019

Choose a reason for hiding this comment

vinokurig Jul 19, 2019

Choose a reason for hiding this comment

benoitf Jul 19, 2019

Choose a reason for hiding this comment

benoitf Jul 19, 2019

Choose a reason for hiding this comment

vinokurig Jul 19, 2019

Choose a reason for hiding this comment

benoitf Jul 19, 2019

Choose a reason for hiding this comment

l0rd left a comment •

edited

Loading

sunix commented Jul 17, 2019 •

edited

Loading

sunix commented Jul 18, 2019 •

edited

Loading