[PATCH] Ensure that SSH runs in non-interactive mode

[PATCH] Ensure that SSH runs in non-interactive mode

[Fredrik Tolf]

OpenSSH has the nice feature that it sets the IP TOS value of its
connection depending on usage. When used in interactive mode, it
is set to Minimize-Delay, and other wise to Maximize-Throughput. Its
usage by Git is best served by Maximize-Throughput, for obvious
reasons.

However, it seems to use a DWIM heuristic for detecting interactive
mode. The current implementation enters interactive mode if either
a PTY is allocated or X11 forwarding is enabled, and even though Git
SSH:ing does not allocate a PTY, X11 forwarding is often turned on
by default. By removing the DISPLAY env variable before forking, SSH
can thus be forced into non-interactive mode, without any obvious
ill effects.

Signed-off-by: Fredrik Tolf <fredrik@dolda2000.com>
---
 connect.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/connect.c b/connect.c
index 574f42f..54888d3 100644
--- a/connect.c
+++ b/connect.c
@@ -607,6 +607,13 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 			*arg++ = port;
 		}
 		*arg++ = host;
+		/* Remove the X11 DISPLAY from the environment, to
+		 * make SSH run non-interactively */
+		const char *env[] = {
+			"DISPLAY",
+			NULL
+		};
+		conn->env = env;
 	}
 	else {
 		/* remove these from the environment */
-- 
1.5.6.2

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Mike Hommey]

On Sat, Jul 19, 2008 at 07:06:55PM +0200, Fredrik Tolf wrote:
> OpenSSH has the nice feature that it sets the IP TOS value of its
> connection depending on usage. When used in interactive mode, it
> is set to Minimize-Delay, and other wise to Maximize-Throughput. Its
> usage by Git is best served by Maximize-Throughput, for obvious
> reasons.
> 
> However, it seems to use a DWIM heuristic for detecting interactive
> mode. The current implementation enters interactive mode if either
> a PTY is allocated or X11 forwarding is enabled, and even though Git
> SSH:ing does not allocate a PTY, X11 forwarding is often turned on
> by default. By removing the DISPLAY env variable before forking, SSH
> can thus be forced into non-interactive mode, without any obvious
> ill effects.

Wouldn't adding the -x option be better ? Also adding -T could be a good
idea.

Mike

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 312 bytes --]

On Sat, 2008-07-19 at 19:06 +0200, Fredrik Tolf wrote:
>  By removing the DISPLAY env variable before forking, SSH
> can thus be forced into non-interactive mode, without any obvious
> ill effects.

This will keep ssh-askpass from using any X-based password input
program.

-- 
keith.packard@intel.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Fredrik Tolf]

On Sat, 2008-07-19 at 10:57 -0700, Keith Packard wrote:
> On Sat, 2008-07-19 at 19:06 +0200, Fredrik Tolf wrote:
> >  By removing the DISPLAY env variable before forking, SSH
> > can thus be forced into non-interactive mode, without any obvious
> > ill effects.
> 
> This will keep ssh-askpass from using any X-based password input
> program.

Ah, right. Would it be OK to add the `-x' flag to ssh instead? I imagine
that that might make git less portable to SSH implementations other than
OpenSSH, but I don't know if that is considered a problem.

Fredrik Tolf

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Johannes Schindelin]

Hi,

On Sat, 19 Jul 2008, Fredrik Tolf wrote:

> On Sat, 2008-07-19 at 10:57 -0700, Keith Packard wrote:
> > On Sat, 2008-07-19 at 19:06 +0200, Fredrik Tolf wrote:
> > >  By removing the DISPLAY env variable before forking, SSH
> > > can thus be forced into non-interactive mode, without any obvious
> > > ill effects.
> > 
> > This will keep ssh-askpass from using any X-based password input
> > program.
> 
> Ah, right. Would it be OK to add the `-x' flag to ssh instead?

I think this would be the correct way, together with "-T".

> I imagine that that might make git less portable to SSH implementations 
> other than OpenSSH, but I don't know if that is considered a problem.

Well, this was to be expected, after what I wrote in response to 3. in
http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=2598

Reality always catches up with you, and here again we see that plink and 
other siblings of OpenSSH should be best handled with scripts, preferably 
ones that strip out options they do not recognize.

IOW something like

-- snip --
#!/bin/bash

plinkopt=
while test $# != 0
do
	case "$1" in
	-p)
		plinkopt="$plinkopt -P $2"
		shift
	;;
	-*)
		# unrecognized; strip out
	;;
	*)
		break
	;;
	esac
	shift
done

exec plink $plinkopt "$@"
-- snap --

Ciao,
Dscho

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Fredrik Tolf]

On Sun, 2008-07-20 at 12:27 +0200, Johannes Schindelin wrote:
> Well, this was to be expected, after what I wrote in response to 3. in
> http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=2598
> 
> Reality always catches up with you, and here again we see that plink and 
> other siblings of OpenSSH should be best handled with scripts, preferably 
> ones that strip out options they do not recognize.

Otherwise, an alternative may be to always install a script, say
`git-ssh', that would invoke the real SSH in a manner specific for the
platform. The exact script installed could even be parametrized by the
Makefile. For systems using OpenSSH, it would probably just consist of
`ssh -xT "$@"'.

What do you think?

Fredrik Tolf

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Ah, right. Would it be OK to add the `-x' flag to ssh instead?
>
> I think this would be the correct way, together with "-T".
>
>> I imagine that that might make git less portable to SSH implementations 
>> other than OpenSSH, but I don't know if that is considered a problem.
>
> Well, this was to be expected, after what I wrote in response to 3. in
> http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=2598
>
> Reality always catches up with you, and here again we see that plink and 
> other siblings of OpenSSH should be best handled with scripts, preferably 
> ones that strip out options they do not recognize.
>
> IOW something like
>
> -- snip --
> #!/bin/bash
>
> plinkopt=
> while test $# != 0
> do
> 	case "$1" in
> 	-p)
> 		plinkopt="$plinkopt -P $2"
> 		shift
> 	;;
> 	-*)
> 		# unrecognized; strip out
> 	;;
> 	*)
> 		break
> 	;;
> 	esac
> 	shift
> done
>
> exec plink $plinkopt "$@"
> -- snap --

I think that is a very sensible approach, but just like we have a few
"built-in" function-header regexps with customization possibilities for
the user, we might want to:

 * Have that "-x", "-T" in the command line we generate for OpenSSH;

 * Allow users to specify OpenSSH substitute via a configuration and/or
   environment variable, and have them use your script; and

 * Have a built-in logic for selected and common "OpenSSH substitute",
   e.g. plink.

There is no reason to make users suffer an extra redirection for common
enough alternatives.

Here is to get it started...

 connect.c |   30 +++++++++++++++++++++++++++---
 1 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 574f42f..c72dd9e 100644
--- a/connect.c
+++ b/connect.c
@@ -599,12 +599,36 @@ struct child_process *git_connect(int fd[2], const char *url_orig,
 	conn->argv = arg = xcalloc(6, sizeof(*arg));
 	if (protocol == PROTO_SSH) {
 		const char *ssh = getenv("GIT_SSH");
+		const char *ssh_basename;
 		if (!ssh) ssh = "ssh";
 
+		ssh_basename = strrchr(ssh, '/');
+		ssh_basename = ssh_basename ? (ssh_basename + 1) : ssh;
+
 		*arg++ = ssh;
-		if (port) {
-			*arg++ = "-p";
-			*arg++ = port;
+		/*
+		 * Make sure to enlarge conn->argv if you add more
+		 * paremeters here.
+		 *
+		 * We know how to invoke a few ssh implementations
+		 * ourselves.
+		 */
+		if (!strcmp(ssh_basename, "plink")) {
+			if (port) {
+				*arg++ = "-P";
+				*arg++ = port;
+			}
+		} else {
+			/*
+			 * This is for stock OpenSSH, but you can have
+			 * your custom wrapper script to parse this
+			 * and invoke other ssh implementations after
+			 * rearranging parameters as well.
+			 */
+			if (port) {
+				*arg++ = "-p";
+				*arg++ = port;
+			}
 		}
 		*arg++ = host;
 	}

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Johannes Schindelin]

Hi,

On Sun, 20 Jul 2008, Fredrik Tolf wrote:

> On Sun, 2008-07-20 at 12:27 +0200, Johannes Schindelin wrote:
>
> > Well, this was to be expected, after what I wrote in response to 3. in 
> > http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=2598
> > 
> > Reality always catches up with you, and here again we see that plink 
> > and other siblings of OpenSSH should be best handled with scripts, 
> > preferably ones that strip out options they do not recognize.
> 
> Otherwise, an alternative may be to always install a script, say 
> `git-ssh', that would invoke the real SSH in a manner specific for the 
> platform. The exact script installed could even be parametrized by the 
> Makefile. For systems using OpenSSH, it would probably just consist of 
> `ssh -xT "$@"'.
> 
> What do you think?

Umm, why?  I fully expect OpenSSH to be the most common ssh helper.  I 
fail to see why we should optimize for something else.

The GIT_SSH solution works.  Why not just leave things like they are?

Ciao,
Dscho

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Fredrik Tolf]

On Sun, 2008-07-20 at 20:33 +0200, Johannes Schindelin wrote:
> > Otherwise, an alternative may be to always install a script, say 
> > `git-ssh', that would invoke the real SSH in a manner specific for the 
> > platform. The exact script installed could even be parametrized by the 
> > Makefile. For systems using OpenSSH, it would probably just consist of 
> > `ssh -xT "$@"'.
> > 
> > What do you think?
> 
> Umm, why?  I fully expect OpenSSH to be the most common ssh helper.  I 
> fail to see why we should optimize for something else.

I guess I just thought the guys trying to port Git to Windows might find
it helpful as well. I don't really care myself.

> The GIT_SSH solution works.  Why not just leave things like they are?

I don't think it is a good idea to leave it like it is, because I think
it is unreasonable to run Git traffic with Minimize-Delay TOS by
default. If for no other reason, changing the TOS would at least make
many networks happier.

Fredrik Tolf

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Johannes Schindelin]

Hi,

On Sun, 20 Jul 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Ah, right. Would it be OK to add the `-x' flag to ssh instead?
> >
> > I think this would be the correct way, together with "-T".
> >
> >> I imagine that that might make git less portable to SSH implementations 
> >> other than OpenSSH, but I don't know if that is considered a problem.
> >
> > Well, this was to be expected, after what I wrote in response to 3. in
> > http://thread.gmane.org/gmane.comp.version-control.git/76650/focus=2598
> >
> > Reality always catches up with you, and here again we see that plink and 
> > other siblings of OpenSSH should be best handled with scripts, preferably 
> > ones that strip out options they do not recognize.
> >
> > IOW something like
> >
> > -- snip --
> > #!/bin/bash
> >
> > plinkopt=
> > while test $# != 0
> > do
> > 	case "$1" in
> > 	-p)
> > 		plinkopt="$plinkopt -P $2"
> > 		shift
> > 	;;
> > 	-*)
> > 		# unrecognized; strip out
> > 	;;
> > 	*)
> > 		break
> > 	;;
> > 	esac
> > 	shift
> > done
> >
> > exec plink $plinkopt "$@"
> > -- snap --
> 
> I think that is a very sensible approach, but just like we have a few
> "built-in" function-header regexps with customization possibilities for
> the user, we might want to:
> 
>  * Have that "-x", "-T" in the command line we generate for OpenSSH;
> 
>  * Allow users to specify OpenSSH substitute via a configuration and/or
>    environment variable, and have them use your script; and
> 
>  * Have a built-in logic for selected and common "OpenSSH substitute",
>    e.g. plink.
> 
> There is no reason to make users suffer an extra redirection for common
> enough alternatives.
> 
> Here is to get it started...

How about this instead?

-- snipsnap --
diff --git a/connect.c b/connect.c
index 574f42f..7e7f4d3 100644
--- a/connect.c
+++ b/connect.c
@@ -603,7 +603,8 @@ struct child_process *git_connect(int fd[2], const char *url
 
 		*arg++ = ssh;
 		if (port) {
-			*arg++ = "-p";
+			const char *opt = getenv("GIT_SSH_PORT_OPTION");
+			*arg++ = opt ? opt : "-p";
 			*arg++ = port;
 		}
 		*arg++ = host;

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> How about this instead?
>
> -- snipsnap --
> diff --git a/connect.c b/connect.c
> index 574f42f..7e7f4d3 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -603,7 +603,8 @@ struct child_process *git_connect(int fd[2], const char *url
>  
>  		*arg++ = ssh;
>  		if (port) {
> -			*arg++ = "-p";
> +			const char *opt = getenv("GIT_SSH_PORT_OPTION");
> +			*arg++ = opt ? opt : "-p";
>  			*arg++ = port;
>  		}
>  		*arg++ = host;

If you only care only about the ones we currently want to support, I do
not htink it makes any difference either way, but if we are shooting for
having a minimum-but-reasonable framework to make it easy to support other
ones that we haven't seen, it feels very much like an inadequate hack to
waste an envirnoment variable for such a narrow special case.  With this,
what you really mean is "Plink uses -P instead of -p", right?

I do not know if "plink" is used widely enough to be special cased, but if
so, I think we would better have an explicit support for it.  Will we add
GIT_SSH_FORBID_X11_FORWARDING_OPTION environment variable and friends,
too?

The extra environment would not help dealing with an implementation that
wants --port=90222 (i.e. not as two separate arguments but a single one),
for example.  You would need the extra wrapper support for that kind of
thing anyway.  That extra environment _solution_ will need to make an
assuption that any reasonable implementation would have an option string
to specify port which may not be "-p" and that is to be followed by a
separate argument that is a decimal port number, which probably is
reasonable for this particular "port" thing, but as a general design
principle I do not think it is a good direction to go.

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Johannes Schindelin]

Hi,

On Sun, 20 Jul 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > How about this instead?
> >
> > -- snipsnap --
> > diff --git a/connect.c b/connect.c
> > index 574f42f..7e7f4d3 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -603,7 +603,8 @@ struct child_process *git_connect(int fd[2], const char *url
> >  
> >  		*arg++ = ssh;
> >  		if (port) {
> > -			*arg++ = "-p";
> > +			const char *opt = getenv("GIT_SSH_PORT_OPTION");
> > +			*arg++ = opt ? opt : "-p";
> >  			*arg++ = port;
> >  		}
> >  		*arg++ = host;
> 
> If you only care only about the ones we currently want to support, I do
> not htink it makes any difference either way, but if we are shooting for
> having a minimum-but-reasonable framework to make it easy to support other
> ones that we haven't seen, it feels very much like an inadequate hack to
> waste an envirnoment variable for such a narrow special case.  With this,
> what you really mean is "Plink uses -P instead of -p", right?

Yeah.  My first attempt was to allow "GIT_SSH='plink.exe -P %p %h'" to 
work, and for that matter, "git config --global transport.ssh 'plink.exe 
-P %p %h'", but I decided that it would be easier to do the patch I 
posted.

Anyway, I think that this issue wasted enough of my time, as I will never 
use plink anyway.  As long as the patch does not have an adverse effect on 
my use case, which happens to be the default case, I will just not bother 
anymore, even if I think that GIT_SSH=wrapper would be better than special 
case rarely exercized ssh programs in the source code.

Ciao,
Dscho

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Jeff King]

On Sun, Jul 20, 2008 at 11:23:13AM -0700, Junio C Hamano wrote:

> I think that is a very sensible approach, but just like we have a few
> "built-in" function-header regexps with customization possibilities for
> the user, we might want to:
> 
>  * Have that "-x", "-T" in the command line we generate for OpenSSH;

I am slightly negative on this, because we are setting OpenSSH
preferences behind the user's back that they would not normally expect
git to be tampering with.

I think the expectation for this is that it impacts only the ssh session
used by git.  But because OpenSSH supports the concept of "master" and
"slave" sessions (i.e., it can multiplex many sessions over a single ssh
session, avoiding authentication and thus reducing latency until the
start of the session), what you do in one session can impact other
sessions. In particular, if the 'master' does not have x11 forwarding
(because it happens to be started by git), then slave connections do not
get it. So a user with X11Forwarding and ControlMaster set in his config
would usually have everything work, but bad timing with the
git-initiated session as the master would unexpectedly break his
X11Forwarding for other sessions.

I don't know how commonly the ControlMaster option for openssh is used.
I also don't know if this should simply be considered a bug in openssh,
since it silently ignores the request for X forwarding.  Personally, I
will not be affected because I don't do X forwarding by default, anyway.
But I thought I would raise the point.

-Peff

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Steffen Prohaska]

On Jul 20, 2008, at 9:51 PM, Junio C Hamano wrote:

> I do not know if "plink" is used widely enough to be special cased,  
> but if
> so, I think we would better have an explicit support for it.

Our installer on Windows explicitly supports plink as an alternative to
OpenSSH.  Putty has a GUI for managing your ssh keys (Pageant).  You  
need
to type your password only once to unlock a key and make it available to
all connections that you start afterwards.

I think it should be special cased.  I use plink myself.

	Steffen

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Mike Hommey]

On Sun, Jul 20, 2008 at 08:14:22PM -0400, Jeff King wrote:
> On Sun, Jul 20, 2008 at 11:23:13AM -0700, Junio C Hamano wrote:
> 
> > I think that is a very sensible approach, but just like we have a few
> > "built-in" function-header regexps with customization possibilities for
> > the user, we might want to:
> > 
> >  * Have that "-x", "-T" in the command line we generate for OpenSSH;
> 
> I am slightly negative on this, because we are setting OpenSSH
> preferences behind the user's back that they would not normally expect
> git to be tampering with.
> 
> I think the expectation for this is that it impacts only the ssh session
> used by git.  But because OpenSSH supports the concept of "master" and
> "slave" sessions (i.e., it can multiplex many sessions over a single ssh
> session, avoiding authentication and thus reducing latency until the
> start of the session), what you do in one session can impact other
> sessions. In particular, if the 'master' does not have x11 forwarding
> (because it happens to be started by git), then slave connections do not
> get it. So a user with X11Forwarding and ControlMaster set in his config
> would usually have everything work, but bad timing with the
> git-initiated session as the master would unexpectedly break his
> X11Forwarding for other sessions.
> 
> I don't know how commonly the ControlMaster option for openssh is used.
> I also don't know if this should simply be considered a bug in openssh,
> since it silently ignores the request for X forwarding.  Personally, I
> will not be affected because I don't do X forwarding by default, anyway.
> But I thought I would raise the point.

I'm not sure the ControlMaster option is still followed when using -T. 
Also, IIRC, ControlMaster doesn't exit until slave connections are
done, so git ssh sessions granted the master control would stall until
then if they happen to have slaves launched. i.e. It can *already* have
bad side effects.

Adding '-S none' would ensure ControlMaster would not take effect; on
the other hand, it would not allow git's ssh connection to be a slave
either. '-o ControlMaster no' could be a solution.

All these need to be tested, obviously.

Mike

Re: [PATCH] Ensure that SSH runs in non-interactive mode

[Jeff King]

On Mon, Jul 21, 2008 at 08:53:48AM +0200, Mike Hommey wrote:

> I'm not sure the ControlMaster option is still followed when using -T. 

It is still followed.

> Also, IIRC, ControlMaster doesn't exit until slave connections are
> done, so git ssh sessions granted the master control would stall until
> then if they happen to have slaves launched. i.e. It can *already* have
> bad side effects.

Yes, that is a problem (and IMHO a weakness in the implementation, but
obviously not git's problem at all).

> Adding '-S none' would ensure ControlMaster would not take effect; on

I think that is definitely a mistake; git is one of the main reasons I
use ControlMaster in the first place.

> the other hand, it would not allow git's ssh connection to be a slave
> either. '-o ControlMaster no' could be a solution.

That is actually quite sensible, and would make this a non-issue, as
far as I can see.

> All these need to be tested, obviously.

I tested, and doing "ssh -Tx -o 'ControlMaster no'" does the right thing
(reuse existing session if possible, create a new one with -Tx
otherwise, and never create a control socket for slaves).

-Peff