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

Support private key forwarding to winscp #7143

Merged
merged 20 commits into from Sep 28, 2022
Merged

Conversation

artu-ole
Copy link
Contributor

@artu-ole artu-ole commented Sep 19, 2022

Fixes #5106
Fixes #3480
Fixes #6126

Naive ssh key transfer via temporary file converting it to ppk format that is the only one supported in cli arguments in winscp. https://winscp.net/eng/docs/public_key#private
There are couple of additional dependancies in the code.
First one, tmp-promise, already used in some other modules, like tabby-electron. Used for the temporary file creation as winscp does not accept private key directly from stdin
Second one, node-forge, merely used to convert the private key into putty format and is something that would not be needed in an ideal world. Unfortunatelly, sshpk putty private keys support(TritonDataCenter/node-sshpk#76) did not come with proper putty files writer. We'd need to either make a pull request to that upstream project or to Eugene's fork of it.

And finally, even though current sftp functionality is sufficient for 95% of work, making a more sophisticated sftp client(#6319) would be the way to go in the future for a one stop shop experience.

P.S. also, ugly timeout is there not to dispose of a temporary file beforehand and because we don't get a child process reference to hook into.

@Eugeny
Copy link
Owner

Eugeny commented Sep 21, 2022

Thanks! I'm on vacation until net week and will check it out then.

@artu-ole
Copy link
Contributor Author

Sure, no rush, In the end this might be more of a proof of concept rather than final implementation due to aforementioned shortcomings.
Getting rid of timeout is fairly simple, should be good enough if exec returned the result of execFile, something along the lines:

await this.platform.exec(path, args)
tmpFile?.cleanup()

Though that would make tabby wait to remove the file until winSCP is closed(or tabby itself is closed as tmp-promise library states that it will clean files on process exit by itself)

There might be a deeper issue lurking. It does not feel entirely safe saving user's private key into a temporary directory rather than some secure application storage. Though I'm not too well versed with electron to suggest a better solution right away.

@Eugeny
Copy link
Owner

Eugeny commented Sep 27, 2022

Made exec async. It's very unfortunate that WinSCP doesn't accept private keys through environment vars which would be both convenient and perfectly safe. A workaround could be to use WinSCP's agent auth by spinning up a temporary SSH agent, but this will conflict with an already running Pageant since there's no way to run multiple agents on Windows.

@Eugeny
Copy link
Owner

Eugeny commented Sep 27, 2022

Tested and it LGTM - let me know if the changes look ok to you and I'll merge ✌️

@artu-ole
Copy link
Contributor Author

Yep, LGTM, works like a charm

@Eugeny Eugeny merged commit 4675b75 into Eugeny:master Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants