Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 9, 2017, 07:46 UTC
Message-ID: <alpine.DEB.2.20.1701090825510.3469@virtualbox>
In-Reply-To: <xmqq4m1911mf.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Sun, 8 Jan 2017, Junio C Hamano wrote:

Show 27 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I suspect that this will break when GIT_SSH_COMMAND, which is meant
> > to be processed by the shell, hence the user can write anything,
> > begins with a one-shot environment variable assignment, e.g.
> >
> > 	[core] sshcommand = VAR1=VAL1 VAR2=VAL2 //path/to/tortoiseplink
> >
> > One possible unintended side effect of this patch is when VAL1 ends
> > with /plink (or /tortoiseplink) and the command is not either of
> > these, in which case the "tortoiseplink" and "putty" variables will
> > tweak the command line for an SSH implementation that does not want
> > such a tweak to be made.  There may be other unintended fallouts.
> 
> Thinking about this further, as the sshcommand (or GIT_SSH_COMMAND)
> interface takes any shell-interpretable commands, the first "token"
> you see is not limited to a one-shot environment assignment.  It
> could even be "cmd1 && cmd2 && ..." or even:
> 
> 	if test -f ${TPLINK:=//path/to/tortoiseplink}
> 	then
> 		exec "$TPLINK" "$@"
> 	elif test -f ${PLINK:=//path/to/plink}
> 		exec "$PLINK" "$@"
> 	else
> 		echo >&2 no usable ssh on this host
> 	fi

Sure, it could be all of that. It could even blast through the environment limits in some environments on some platforms, but not on others. It could automatically transfer bitcoins whenever a connection to github.com is attempted. It could start the camera and build a time-lapse of "every time I pushed or fetched a repository", as an art project. It could shut down the computer.

It could do all of that.

In practice, however, what users realistically do is to use GIT_SSH_COMMAND whenever they need to override the ssh command with options.

This is the common case, and that is what we must make less cumbersome to use.

If you feel strongly about your contrived examples possibly being affected by this patch, we could easily make this conditional on

1) no '&&' or '||' being found on the command-line, and
2) argv[0] not containing an '='

Another approach would be to verify that argv[i] starts with '-' for non-zero i.

But do we really need to do that?

Let's have a very brief look at the amount of work required to come up with a false positive (I am not so much concerned about any power user writing elaborate shell expressions that may call plink.exe: those power users will simply have to continue to use their own -P => -p and -batch hacks):

The logic kicks in if the split command-line's first component has a basename `plink` or `tortoiseplink` (possibly with `.exe` suffix). The only way this logic can kick in by mistake is if the first component is *not* the command to call *and* if the first component *still* has a basename of `plink` or `tortoiseplink`.

That means that the user has to specify something like

	HAHAHA_IT_IS_NOT=/plink.exe ssh

as GIT_SSH_COMMAND.

Now, let's take a *very* brief look at the question how likely it is to have a basename of `plink` or `tortoiseplink`. Remember, there are two ways that the basename can be a specific term: either the original string is already equal to that term, or it ends in a slash followed by the term. That is, either the first component refers to plink.exe/tortoiseplink.exe, i.e. it would be a *true* positive. Or the first component would end in "/plink" or "/tortoiseplink" (possibly with the `.exe` suffix) *and not be a command*. But how likely is it to specify a non-command (such as a per-process variable assignment) that specifically mentions plink or tortoiseplink?

Is it even realistic to expect users to specify values for GIT_SSH_COMMAND that contain "plink" as a substring when they do *not* want to call plink at all?

After thinking a bit longer than I had originally planned about it, my answer is: no. No, I do not think it is realistic. I fully expect *no* user to have a GIT_SSH_COMMAND containing even a substring "plink" *anywhere*, unless they do, in fact, want to call plink or tortoiseplink.

My main aim with Git for Windows is to improve the user experience, and the patch in question does do that. Of course, I also try to avoid breaking existing setups while improving the user experience, and I believe that it is safe to assume that the patch in question in all likelihood does not break any existing setup.

Ciao, Dscho