Skip to content

ProxyCommand incorrectly requires an absolute path #1185

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

Closed
jstarks opened this issue Jun 13, 2018 · 4 comments
Closed

ProxyCommand incorrectly requires an absolute path #1185

jstarks opened this issue Jun 13, 2018 · 4 comments
Assignees
Labels
Issue-Enhancement Feature request

Comments

@jstarks
Copy link

jstarks commented Jun 13, 2018

ProxyCommand requires an executable with an absolute path or a path relative to the ssh.exe installation directory. This is because it uses posix_spawn instead of posix_spawnp (although the latter is not currently implemented).

This seems wrong. OpenSSH on POSIX passes the ProxyCommand to the shell, and the shell looks up the executable in the path.

This breaks the inbox Windows hvc tool for connecting to Hyper-V VMs.

Expected:

> ssh -o "ProxyCommand=cmd.exe /c echo Invalid proxy 1>&2" abc
Invalid proxy
ssh_exchange_identification: Connection closed by remote host

Observed (Linux, relative path):

$ ssh -o "ProxyCommand=sh -c 'echo Invalid proxy 1>&2'" abc
Invalid proxy
ssh_exchange_identification: Connection closed by remote host

Observed (Windows, absolute path):

> ssh -o "ProxyCommand=c:\windows\system32\cmd.exe /c echo Invalid proxy 1>&2" abc
Invalid proxy
ssh_exchange_identification: Connection closed by remote host

Observed (Windows, relative path):

> ssh -o "ProxyCommand=cmd.exe /c echo Invalid proxy 1>&2" abc
CreateProcessW failed error:2
posix_spawn: No such file or directory
@NoMoreFood
Copy link

From what I can tell, the upstream code doesn't actually use posix_spawnp(), it uses execv() after a fork(). Regardless, it looks like Win32 fork prepends the OpenSSH directory to the executable string in spawn_child_internal() if the string is not an absolute path which subsequently causes CreateProcess() to not go through the normal PATH environment variable search routine. I imagine this might have been to ensure that an unqualified executable in sshd_config could be launched without worrying about what the working directory might have been sshd was launched. Maybe a better thing to do would be to set the working directory to the OpenSSH at launch via SetCurrentDirectory() and let the CreateProcess() executable resolution code do it's thing or maybe prepend the OpenSSH directory into the beginning of the path which should have the same effect? Thoughts?

@jstarks
Copy link
Author

jstarks commented Jul 19, 2018

Right, upstream uses execv, but it launches the process through the shell (arg0 = $SHELL, arg1 = -c, arg2 = the proxy command) , which will handle looking up the binary in the path internally, among other things.

Perhaps the right fix is to do the same and run the command line through %COMSPEC% /c.

@manojampalam
Copy link
Contributor

Behavior of current posix_spawn is to support launching ssh from sftp and scp.
I like the idea of supporting a posix_spawnp variant that would respect PATH instead. Will have it supported in next drop.

@manojampalam manojampalam added this to the vNext milestone Jul 20, 2018
@bingbing8 bingbing8 modified the milestones: 7.7.2.0p1-Beta, vNext Jul 26, 2018
bingbing8 added a commit to bingbing8/openssh-portable that referenced this issue Sep 25, 2018
@bingbing8 bingbing8 self-assigned this Sep 26, 2018
bingbing8 added a commit to PowerShell/openssh-portable that referenced this issue Sep 29, 2018
 Added support of posix_spawnp.
1. fix of issue PowerShell/Win32-OpenSSH#1185
2. add End2End tests
@bingbing8
Copy link
Contributor

posix_spawnp is implemented to respect PATH. Relative path should work for ProxyCommand now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Enhancement Feature request
Projects
None yet
Development

No branches or pull requests

4 participants