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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

Brandon Williams wrote:

> 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". :)

> '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@xxxxxxxxxx>
> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>

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.

> ---
>  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)
[...]
> -	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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux