Re: [PATCH v3 10/10] ssh: introduce a 'simple' ssh variant

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 3, 2017, 21:42 UTC
Message-ID: <20171003214206.GY19555@aiede.mtv.corp.google.com>
In-Reply-To: <20171003201507.3589-11-bmwill@google.com>

Hi,

Brandon Williams wrote:

Show 11 quoted lines

> When using the 'ssh' transport, the '-o' option is used to specify an
> environment variable which should be set on the remote end.  This allows
> git to send additional information when contacting the server,
> requesting the use of a different protocol version via the
> 'GIT_PROTOCOL' environment variable like so: "-o SendEnv=GIT_PROTOCOL"
>
> Unfortunately not all ssh variants support the sending of environment
> variables to the remote end.  To account for this, only use the '-o'
> option for ssh variants which are OpenSSH compliant.  This is done by
> checking that the basename of the ssh command is 'ssh' or the ssh
> variant is overridden to be 'ssh' (via the ssh.variant config).

This also affects -p (port), right?

What happens if I specify a ssh://host:port/path URL and the SSH implementation is of 'simple' type?

> Previously if an ssh command's basename wasn't 'plink' or

Git's commit messages use the present tense to describe the current state of the code, so this is "Currently". :)

Show 12 quoted lines

> 'tortoiseplink' git assumed that the command was an OpenSSH variant.
> Since user configured ssh commands may not be OpenSSH compliant, tighten
> this constraint and assume a variant of 'simple' if the basename of the
> command doesn't match the variants known to git.  The new ssh variant
> 'simple' will only have the host and command to execute ([username@]host
> command) passed as parameters to the ssh command.
>
> Update the Documentation to better reflect the command-line options sent
> to ssh commands based on their variant.
>
> Reported-by: Jeffrey Yasskin <jyasskin@google.com>
> Signed-off-by: Brandon Williams <bmwill@google.com>

Thanks for working on this.

For background, the GIT_SSH implementation that motivated this is https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215, which does not support -p or -4/-6, either.

Show 7 quoted lines

> ---
>  Documentation/config.txt |  27 ++++++++++--
>  Documentation/git.txt    |   9 ++--
>  connect.c                | 107 ++++++++++++++++++++++++++---------------------
>  t/t5601-clone.sh         |   9 ++--
>  t/t5700-protocol-v1.sh   |   2 +
>  5 files changed, 95 insertions(+), 59 deletions(-)

[...]

> --- a/connect.c
> +++ b/connect.c
> @@ -776,37 +776,44 @@ static const char *get_ssh_command(void)

[...]

> +static enum ssh_variant determine_ssh_variant(const char *ssh_command,
> +					      int is_cmdline)

[...]

Show 5 quoted lines

> -	if (!strcasecmp(variant, "plink") ||
> -	    !strcasecmp(variant, "plink.exe"))
> -		*port_option = 'P';
> +	if (!strcasecmp(variant, "ssh"))
> +		ssh_variant = VARIANT_SSH;

Could this handle ssh.exe, too?

[...]

> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh

Can this get tests for the new defaulting behavior?  E.g.

 - default is "simple"
 - how "simple" treats an ssh://host:port/path URL
 - how "simple" treats ipv4/ipv6 switching
 - ssh defaults to "ssh"
 - if GIT_SSH=ssh, can set ssh.variant to "simple" to force the "simple"
   mode

One other worry: this (intentionally) changes the behavior of a previously-working GIT_SSH=ssh-wrapper that wants to support OpenSSH-style options but does not declare ssh.variant=ssh. When discovering this change, what should the author of such an ssh-wrapper do?

They could instruct their users to set ssh.variant or GIT_SSH_VARIANT to "ssh", but then they are at the mercy of future additional options supported by OpenSSH we may want to start using in the future (e.g., maybe we will start passing "--" to separate options from the hostname). So this is not a futureproof option for them.

They could take the new default behavior or instruct their users to set ssh.variant or GIT_SSH_VARIANT to "simple", but then they lose support for handling alternate ports, ipv4/ipv6, and specifying -o SendEnv to propagate GIT_PROTOCOL or other envvars. They can handle GIT_PROTOCOL propagation manually, but losing port support seems like a heavy cost.

They could send a patch to define yet another variant that is forward-compatible, for example using an interface similar to what git-credential(1) uses. Then they can set GIT_SSH to their OpenSSH-style helper and GIT_FANCY_NEW_SSH to their more modern helper, so that old Git versions could use GIT_SSH and new Git versions could use GIT_FANCY_NEW_SSH. This might be their best option. It feels odd to say that their only good way forward is to send a patch, but on the other hand someone with such an itch is likely to be in the best position to define an appropriate interface.

They could send a documentation patch to make more promises about the commandline used in OpenSSH mode: e.g. setting a rule in advance about which options can take an argument so that they can properly parse an OpenSSH command line in a future-compatible way.

Or they could send a patch to allow passing the port in "simple" mode, for example using an environment variable.

Am I missing another option?  What advice do we give to this person?

Thanks, Jonathan