ssh: allow multiple -l options and user@host to override one another (fixes regression)

[dscho]

887669e (upstream commit, 2017-10-21) introduced a regression: while
OpenSSH v7.6p1's SSH client would happily let the last command-line
argument specifying a user name override previous ones, this is no longer
true in OpenSSH v7.7p1:

ssh -l narf -l egads pinkie@brain

would try to connect as pinkie in v7.6p1, but as narf in v7.7p1.

Since this change was not covered in the release notes of v7.7p1, it can
be considered an unwanted regression, and this patch fixes that
regression.

This fixes git-for-windows/git#1616.

Signed-off-by: Johannes Schindelin johannes.schindelin@gmx.de

[dscho]

Please note that I also sent an accompanying mail to openssh-unix-dev@mindrot.org, as https://www.openssh.com/report.html says:

openssh-unix-dev@mindrot.org This is a public list and is open to posting from non-subscribed users.

However, this appears to be a blatant lie, as I received a reply:

Posting by non-members to openssh-unix-dev@mindrot.org is currently disabled, sorry.

Thanks for the fish.

In any case, I will reproduce my mail here:

-- snipsnap --
Date: Thu, 5 Apr 2018 14:58:20 +0200 (DST)
From: @dscho
To: openssh-unix-dev@mindrot.org
cc: @MarkEWaite
Subject: BUG: 7.7 changes precedence of -l user over user@host

Hi team,

We noticed a regression in OpenSSH v7.7p1 that was not listed in the
"Potentially-incompatible changes", and that I could also not find on the
OpenSSH bug tracker. Consider the command-line

    ssh -l jenkins git@github.com

Now, it is clear that there is contradicting information here: is
jenkins the user name or git? Well, we have to pick one over the
other, that much is clear.

In OpenSSH v7.6p1, the user name specified in the URL won: it would
connect as git.

However, in OpenSSH v7.7p1, jenkins is picked instead. This is a change
in behavior that should have at least been mentioned in the release notes
as a definitely incompatible change.

At least it caught us by surprise.

The commit in question is this: 887669e (upstream commit, 2017-10-21). It
takes pains of not overriding options.user once it has been set. This
is the opposite behavior from v7.6p1's which would always override the
user when it encountered a command-line argument specifying a user name.
This change is not covered by the purpose of the commit (to introduce
parsing ssh://user@host/ style URIs).

While looking for the culprit, I also could not fail to notice another
regression, this one potentially even more serious. Consider an alias

    alias myssh='ssh -l default-name ...'

Clearly, the intent of this is to set a default user name for logging into
my many machines/VMs, and clearly I want to be able to use a different
user name (and still all the other options specified in the alias) using
myssh -l other-name host, taking advantage of the well-established best
practice to let later command-line options override earlier ones (which
OpenSSH v7.6p1 heeded).

However, this use case was also broken by OpenSSH v7.7p1, by this change
in ssh.c (in the same commit as mentioned above: 887669e):

-- snip --

                case 'l':
-                       options.user = optarg;
+                       if (options.user == NULL)
+                               options.user = optarg;
                        break;

-- snap --

This regression was found during the analysis of a bug reported on the Git
for Windows bug tracker where Jenkins' Git plugin demonstrates a problem
when using OpenSSH v7.7p1's SSH client:

    https://github.com/git-for-windows/git/issues/1616

Looking forward to hearing back from you,
Johannes

P.S.: For good measure, I opened a PR at
#91 (even if you do not
accept any Pull Requests there, it seems that you apply the patches at
least sometimes).

[dscho]

@MarkEWaite was kind enough to open a bug here: http://bugzilla.mindrot.org/show_bug.cgi?id=2849 (he apparently knew better than to follow the instructions at https://www.openssh.com/report.html, which did not work for me)

[dscho]

... and now I even tested this and realized that the pw_name fallback really needs to be just a fallback (as it is applied after command-line parsing). So I force-pushed with an updated patch.

[dscho]

@MarkEWaite BTW if there is no reaction from the OpenSSH folks to this here PR nor to the ticket you opened, I am willing to take this patch and release a Git for Windows v2.17.0(2) with an appropriately-patched ssh.exe. What do you think?

[MarkEWaite]

That would be really great for me. I'm not confident that I can safely make the change in the Jenkins git client plugin code to determine the correct username to pass as the value of the -l argument.

If you are willing to release a new Git for Windows with a patched ssh.exe, that makes my life much easier.

I'm willing to find and make that change in the Jenkins git client plugin code if the OpenSSH team declares that the new behavior is the "correct" behavior, but I think your description of the side effects of the OpenSSH 7.7 change persuade me that the old behavior was the correct behavior.

[MarkEWaite]

@dscho I'd appreciate your review and involvement in the discussion on OpenSSH 2849. I believe that I've correctly described the broader case that you found. If I've missed something, please add further details to that bug report.

[dscho]

@MarkEWaite will do. TBH I am a bit shocked how nonchalantly your case was dismissed as "this is not a bug", when the change in behavior was clearly unintended, and clearly has adverse real-world consequences. That's no way to run a successful software project.

[dscho]

@MarkEWaite seems that you have to jump through hoops. Sorry.

[ThomasAH]

I stumbled across this incompatible change few months ago as I'm using a small shell script rssh when using ssh for administrative purposes, which basically does this:
exec ssh -axl root -i $HOME/.ssh/admin "$@"

Yes, when using my admin key I often want to become root on that system.
But not always.
So sometimes I use something like: rssh admin@machine.example.com
Which now tries to connect as root.

Usually I notice the problem, temporarily load the key into my ssh-agent and just use ssh instead.

But sometimes I use this rssh script (or an equivalent call) in Makefiles or .hg/hgrc configurations, which some days ago resulted in a small disaster:
I used rsync with this rssh script and the --delete option to publish new website data (which were important enough to be protected by the admin key) to an account which is chrooted to the document root directory. But instead of replacing the website documents with their new version, I replaced everything below /root with these files, removing e.g. /root/.ssh/authorized_keys in the process and thus preventing quick access to the system.
After logging in in a different way I could repair everything, but still, this was data loss.

On a different system publishing the web pages happened without a chroot-locked account, so here the change just caused new files to be owned by root instead of the user, which in turn caused further updates to fail when using the intended user account.

So it seems I need an ssh wrapper which does much more parsing to solve this?

[MarkEWaite]

@ThomasAH you've described the exact conditions that caused concern for @dscho and for me. However, I agree with the decision by @dscho that it is more important that Git for Windows use as much of upstream as possible. It would be a maintenance and reliability challenge to maintain an OpenSSH fork in the Git for Windows code base because we disagree with how upstream handles command line arguments.

The OpenSSH team has decided that they will handle command line arguments in this way. Your best hope is to attempt to persuade them that they should handle command line arguments differently. @dscho and I tried to persuade them but were unsuccessful.

The OpenSSH team decision is described in the bug report on their issue tracker.