From: Brandon Williams <bmwill@google.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Stefan Beller <sbeller@google.com>,
Jonathan Tan <jonathantanmy@google.com>,
Segev Finer <segev208@gmail.com>
Subject: Re: [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple'
Date: Mon, 20 Nov 2017 14:25:21 -0800 [thread overview]
Message-ID: <20171120222521.GD92506@google.com> (raw)
In-Reply-To: <20171120213004.57552ja3nmxy6pmc@aiede.mtv.corp.google.com>
On 11/20, Jonathan Nieder wrote:
> Android's "repo" tool is a tool for managing a large codebase
> consisting of multiple smaller repositories, similar to Git's
> submodule feature. Starting with Git 94b8ae5a (ssh: introduce a
> 'simple' ssh variant, 2017-10-16), users noticed that it stopped
> handling the port in ssh:// URLs.
>
> The cause: when it encounters ssh:// URLs, repo pre-connects to the
> server and sets GIT_SSH to a helper ".repo/repo/git_ssh" that reuses
> that connection. Before 94b8ae5a, the helper was assumed to support
> OpenSSH options for lack of a better guess and got passed a -p option
> to set the port. After that patch, it uses the new default of a
> simple helper that does not accept an option to set the port.
>
> The next release of "repo" will set GIT_SSH_VARIANT to "ssh" to avoid
> that. But users of old versions and of other similar GIT_SSH
> implementations would not get the benefit of that fix.
>
> So update the default to use OpenSSH options again, with a twist. As
> observed in 94b8ae5a, we cannot assume that $GIT_SSH always handles
> OpenSSH options: common helpers such as travis-ci's dpl[*] are
> configured using GIT_SSH and do not accept OpenSSH options. So make
> the default a new variant "auto", with the following behavior:
>
> 1. First, check for a recognized basename, like today.
>
> 2. If the basename is not recognized, check whether $GIT_SSH supports
> OpenSSH options by running
>
> $GIT_SSH -G <options> <host>
>
> This returns status 0 and prints configuration in OpenSSH if it
> recognizes all <options> and returns status 255 if it encounters
> an unrecognized option. A wrapper script like
>
> exec ssh -- "$@"
>
> would fail with
>
> ssh: Could not resolve hostname -g: Name or service not known
>
> , correctly reflecting that it does not support OpenSSH options.
> The command is run with stdin, stdout, and stderr redirected to
> /dev/null so even a command that expects a terminal would exit
> immediately.
>
> 3. Based on the result from step (2), behave like "ssh" (if it
> succeeded) or "simple" (if it failed).
>
> This way, the default ssh variant for unrecognized commands can handle
> both the repo and dpl cases as intended.
>
> This autodetection has been running on Google workstations since
> 2017-10-23 with no reported negative effects.
>
> [*] https://github.com/travis-ci/dpl/blob/6c3fddfda1f2a85944c544446b068bac0a77c049/lib/dpl/provider.rb#L215
>
> Reported-by: William Yan <wyan@google.com>
> Improved-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Added two notes to the commit message:
> - describing the real-world testing this patch has undergone
> - stdin, stdout, and stderr go to /dev/null, preventing a
> hypothetical ssh variant that *ignores* -G from hanging waiting for
> input from the terminal.
>
> This is to address the worries at
> https://public-inbox.org/git/xmqq60b59toe.fsf@gitster.mtv.corp.google.com/
> and https://public-inbox.org/git/CAGZ79kZTjUvcq_hKHCqTDoaBxt2x+9XcqYc6ao1bhcET2SM-PQ@mail.gmail.com/
> about hanging.
>
> No change to the code from last time.
>
Thanks a lot for getting this patch out. It's a much more robust
solution than I had originally and should hopefully avoid any more issues
with detecting different ssh programs. My only concern is there may be
some program out there that uses -G in a way different from OpenSSH.
Though realistically i don't think that is an issue because if that ends
up being a problem we can just have the authors of the offending ssh
client send a patch to us to fix it! :)
> Documentation/config.txt | 24 +++++++++++++++---------
> connect.c | 32 +++++++++++++++++++++++++-------
> t/t5601-clone.sh | 21 +++++++++++++++++++++
> 3 files changed, 61 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0460af37e2..0c371ad786 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2081,16 +2081,22 @@ matched against are those given directly to Git commands. This means any URLs
> visited as a result of a redirection do not participate in matching.
>
> ssh.variant::
> - Depending on the value of the environment variables `GIT_SSH` or
> - `GIT_SSH_COMMAND`, or the config setting `core.sshCommand`, Git
> - auto-detects whether to adjust its command-line parameters for use
> - with ssh (OpenSSH), plink or tortoiseplink, as opposed to the default
> - (simple).
> + By default, Git determines the command line arguments to use
> + based on the basename of the configured SSH command (configured
> + using the environment variable `GIT_SSH` or `GIT_SSH_COMMAND` or
> + the config setting `core.sshCommand`). If the basename is
> + unrecognized, Git will attempt to detect support of OpenSSH
> + options by first invoking the configured SSH command with the
> + `-G` (print configuration) option and will subsequently use
> + OpenSSH options (if that is successful) or no options besides
> + the host and remote command (if it fails).
> +
> -The config variable `ssh.variant` can be set to override this auto-detection;
> -valid values are `ssh`, `simple`, `plink`, `putty` or `tortoiseplink`. Any
> -other value will be treated as normal ssh. This setting can be overridden via
> -the environment variable `GIT_SSH_VARIANT`.
> +The config variable `ssh.variant` can be set to override this detection.
> +Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
> +`tortoiseplink`, `simple` (no options except the host and remote command).
> +The default auto-detection can be explicitly requested using the value
> +`auto`. Any other value is treated as `ssh`. This setting can also be
> +overridden via the environment variable `GIT_SSH_VARIANT`.
> +
> The current command-line parameters used for each variant are as
> follows:
> diff --git a/connect.c b/connect.c
> index d2fbb15cc5..455c54a2ec 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -788,6 +788,7 @@ static const char *get_ssh_command(void)
> }
>
> enum ssh_variant {
> + VARIANT_AUTO,
> VARIANT_SIMPLE,
> VARIANT_SSH,
> VARIANT_PLINK,
> @@ -795,14 +796,16 @@ enum ssh_variant {
> VARIANT_TORTOISEPLINK,
> };
>
> -static int override_ssh_variant(enum ssh_variant *ssh_variant)
> +static void override_ssh_variant(enum ssh_variant *ssh_variant)
> {
> const char *variant = getenv("GIT_SSH_VARIANT");
>
> if (!variant && git_config_get_string_const("ssh.variant", &variant))
> - return 0;
> + return;
>
> - if (!strcmp(variant, "plink"))
> + if (!strcmp(variant, "auto"))
> + *ssh_variant = VARIANT_AUTO;
> + else if (!strcmp(variant, "plink"))
> *ssh_variant = VARIANT_PLINK;
> else if (!strcmp(variant, "putty"))
> *ssh_variant = VARIANT_PUTTY;
> @@ -812,18 +815,18 @@ static int override_ssh_variant(enum ssh_variant *ssh_variant)
> *ssh_variant = VARIANT_SIMPLE;
> else
> *ssh_variant = VARIANT_SSH;
> -
> - return 1;
> }
>
> static enum ssh_variant determine_ssh_variant(const char *ssh_command,
> int is_cmdline)
> {
> - enum ssh_variant ssh_variant = VARIANT_SIMPLE;
> + enum ssh_variant ssh_variant = VARIANT_AUTO;
> const char *variant;
> char *p = NULL;
>
> - if (override_ssh_variant(&ssh_variant))
> + override_ssh_variant(&ssh_variant);
> +
> + if (ssh_variant != VARIANT_AUTO)
> return ssh_variant;
>
> if (!is_cmdline) {
> @@ -982,6 +985,21 @@ static void fill_ssh_args(struct child_process *conn, const char *ssh_host,
> variant = determine_ssh_variant(ssh, 0);
> }
>
> + if (variant == VARIANT_AUTO) {
> + struct child_process detect = CHILD_PROCESS_INIT;
> +
> + detect.use_shell = conn->use_shell;
> + detect.no_stdin = detect.no_stdout = detect.no_stderr = 1;
> +
> + argv_array_push(&detect.args, ssh);
> + argv_array_push(&detect.args, "-G");
> + push_ssh_options(&detect.args, &detect.env_array,
> + VARIANT_SSH, port, flags);
> + argv_array_push(&detect.args, ssh_host);
> +
> + variant = run_command(&detect) ? VARIANT_SIMPLE : VARIANT_SSH;
> + }
> +
> argv_array_push(&conn->args, ssh);
> push_ssh_options(&conn->args, &conn->env_array, variant, port, flags);
> argv_array_push(&conn->args, ssh_host);
> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
> index 9d007c0f8d..209e2d5604 100755
> --- a/t/t5601-clone.sh
> +++ b/t/t5601-clone.sh
> @@ -369,6 +369,12 @@ test_expect_success 'variant can be overriden' '
> expect_ssh myhost src
> '
>
> +test_expect_success 'variant=auto picks based on basename' '
> + copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> + git -c ssh.variant=auto clone -4 "[myhost:123]:src" ssh-auto-clone &&
> + expect_ssh "-4 -P 123" myhost src
> +'
> +
> test_expect_success 'simple is treated as simple' '
> copy_ssh_wrapper_as "$TRASH_DIRECTORY/simple" &&
> git clone -4 "[myhost:123]:src" ssh-bracket-clone-simple &&
> @@ -381,6 +387,21 @@ test_expect_success 'uplink is treated as simple' '
> expect_ssh myhost src
> '
>
> +test_expect_success 'OpenSSH-like uplink is treated as ssh' '
> + write_script "$TRASH_DIRECTORY/uplink" <<-EOF &&
> + if test "\$1" = "-G"
> + then
> + exit 0
> + fi &&
> + exec "\$TRASH_DIRECTORY/ssh$X" "\$@"
> + EOF
> + test_when_finished "rm -f \"\$TRASH_DIRECTORY/uplink\"" &&
> + GIT_SSH="$TRASH_DIRECTORY/uplink" &&
> + test_when_finished "GIT_SSH=\"\$TRASH_DIRECTORY/ssh\$X\"" &&
> + git clone "[myhost:123]:src" ssh-bracket-clone-sshlike-uplink &&
> + expect_ssh "-p 123" myhost src
> +'
> +
> test_expect_success 'plink is treated specially (as putty)' '
> copy_ssh_wrapper_as "$TRASH_DIRECTORY/plink" &&
> git clone "[myhost:123]:src" ssh-bracket-clone-plink-0 &&
> --
> 2.15.0.448.gf294e3d99a
>
--
Brandon Williams
next prev parent reply other threads:[~2017-11-20 22:25 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-20 21:21 [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH Jonathan Nieder
2017-11-20 21:22 ` [PATCH 1/8] ssh test: make copy_ssh_wrapper_as clean up after itself Jonathan Nieder
2017-11-20 21:47 ` Brandon Williams
2017-11-21 1:24 ` Junio C Hamano
2017-11-21 1:49 ` [PATCH 1/8 v2] " Jonathan Nieder
2017-11-21 23:42 ` Stefan Beller
2017-11-20 21:22 ` [PATCH 2/8] connect: move no_fork fallback to git_tcp_connect Jonathan Nieder
2017-11-20 21:23 ` [PATCH 3/8] connect: split git:// setup into a separate function Jonathan Nieder
2017-11-20 21:52 ` Brandon Williams
2017-11-20 22:04 ` Jonathan Nieder
2017-11-20 22:29 ` Brandon Williams
2017-11-20 21:25 ` [PATCH 4/8] connect: split ssh command line options into " Jonathan Nieder
2017-11-20 21:54 ` Brandon Williams
2017-11-20 22:09 ` Jonathan Nieder
2017-11-20 22:28 ` Brandon Williams
2017-11-20 22:19 ` [PATCH v4 " Jonathan Nieder
2017-11-20 21:26 ` [PATCH 5/8] connect: split ssh option computation to its own function Jonathan Nieder
2017-11-21 1:31 ` Junio C Hamano
2017-11-20 21:30 ` [PATCH 6/8] ssh: 'auto' variant to select between 'ssh' and 'simple' Jonathan Nieder
2017-11-20 22:25 ` Brandon Williams [this message]
2017-11-21 1:48 ` Junio C Hamano
2017-11-21 2:01 ` Jonathan Nieder
2017-11-20 21:30 ` [PATCH 7/8] ssh: 'simple' variant does not support -4/-6 Jonathan Nieder
2017-11-20 21:31 ` [PATCH 8/8] ssh: 'simple' variant does not support --port Jonathan Nieder
2017-11-20 22:32 ` [PATCH v3 0/8] Coping with unrecognized ssh wrapper scripts in GIT_SSH Brandon Williams
2017-11-22 0:00 ` Stefan Beller
2017-11-22 1:52 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171120222521.GD92506@google.com \
--to=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.com \
--cc=sbeller@google.com \
--cc=segev208@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).