Add choice for external OpenSSH

[Okeanos]

This pull request addresses git-for-windows/git/issues/1683 and git-for-windows/git/issues/2944 by adding an additional detecting mechanism for Windows OpenSSH to the choice page formerly known as PuTTYPage and allows the user to "skip" the installation of the bundled SSH related binaries that Windows also supplies.

When selected this allows interaction between for example KeePassXC OpenSSH agent integration and Git. This may partially address keepassxreboot/keepassxc/issues/4681 I suppose.

There is a small problem with the choice page, though, at the moment because the installer window is not tall enough to fully display the choices.
I suppose rewording the texts or omitting the PuTTY/Plink option if it wasn't successfully detected would work?

Also I have no 32Bit system to test this with 🤷‍♂️

Window supplies the following OpenSSH related binaries as documented here.

Microsoft built OpenSSH independent from Windows is available here and releases are tracked here.

[Okeanos]

Another thought … instead of explicitly checking for the Windows supplied OpenSSH binary it might be better to scan the PATH for any OpenSSH binary because users can for example use the portable versions from the official Windows OpenSSH repo and manually install them (wherever?):

if FileExists(ExpandConstant('{sys}\OpenSSH\ssh.exe')) then begin
  //...
end;

vs.

SshExePath := FileSearch('ssh.exe', GetEnv('PATH'));
if SshExePath <> '' then
 //...
end;

That'd change the whole thing from "Windows OpenSSH" to "Detected 3rd Party OpenSSH" text wise, functionality wise it'd be the same, just a tad more flexible.

[dscho]

it might be better to scan the PATH for any OpenSSH binary

That's a good idea! We would then reword the first option to "Use bundled OpenSSH".

the installer window is not tall enough to fully display the choices.
I suppose rewording the texts or omitting the PuTTY/Plink option if it wasn't successfully detected would work?

Yes. I don't think we want to bore the user with implementation details like ssh.variant and GIT_SSH. Something like this should be plenty sufficient:

PuTTY sessions were found. To use them specify the path
to an existing copy of (Tortoise)Plink.exe

[dscho]

Also I have no 32Bit system to test this with 🤷‍♂️

I don't think that will be necessary.

[dscho]

This looks awesome! I'd like to ask just for the following changes:

• please wrap the commit message at <=76 columns/line
• we need to dynamically generate the list of OpenSSH files to delete in case of option 3
• Option 3 should be "Use ssh.exe found in the PATH" or something like that

[Okeanos]

So, I fixed some of the outstanding issues; in particular the display and wording of the choice page should now work as desired:

Additionally, I now use the proposed find on path solution to trigger the appearance of the choice page.

I also updated the commit message (<=76 characters per line) to reflect the slightly different scope than initially proposed.

The "what binaries to delete/keep" section in the post install part needs to be updated, still. I am not entirely sure how to do that.

[Okeanos]

@dscho I updated the "skip" (deletion) logic – still not fully in the place you want, though. Is there any example of using the please.sh output you mentioned within the install.iss?

The screenshot below should show that /usr/bin/* and /usr/lib/ssh/* are now correctly stripped of conflicting binaries.

[dscho]

Hopefully this (quite lengthy) description how to generate the function to delete the OpenSSH files works for you.

By the way, I strongly advise not to special-case ssh-copy-id: There is no guarantee whatsoever that using a copy of this script from MSYS2's OpenSSH will work with the Win32 variant of OpenSSH at all.

[Okeanos]

@dscho thank you so much for your input – I believe I have addressed the necessary structural changes and also verified that they work as expected. Actually even checking out a private repository using Win32 SSH with git clone via the git bash.

There are some remnants:

• the ssh folders are not deleted 🤷‍♂️
• ssh-pageant.exe survives – is that a problem?

Other than that I believe I only have to address the separate commits for the PuTTY description changes, the SSHChoice Page renaming, and finally the actual 3rd option being added with the additional new functionality. Would that be fine in a single pull request or would you like to receive this in different ones?

[dscho]

Other than that I believe I only have to address the separate commits for the PuTTY description changes, the SSHChoice Page renaming, and finally the actual 3rd option being added with the additional new functionality. Would that be fine in a single pull request or would you like to receive this in different ones?

I would prefer the separate commits as "preparatory" ones, i.e. they should be extracted into their own commits that come before the commit that adds that 3rd option (which should be the tip commit of your branch).

Also, could I ask for one more favor? I'd like the commit messages to all begin with installer:, so that browsing the output of git log is less confusing (there are quite a few more things in build-extra than just the installer definition).

[dscho]

I would prefer the separate commits as "preparatory" ones, i.e. they should be extracted into their own commits that come before the commit that adds that 3rd option (which should be the tip commit of your branch).

Right, I did not explicitly state this: I would like the commits to all live in this here PR. Thank you!

[dscho]

the ssh folders are not deleted 🤷‍♂️

No problem, I did not expect that to happen anyway.

ssh-pageant.exe survives – is that a problem?

Not a problem.

[dscho]

I only have to address the separate commits for the PuTTY description changes, the SSHChoice Page renaming, and finally the actual 3rd option being added with the additional new functionality.

BTW if you have never done this before, I hope https://git-scm.com/docs/git-rebase#_splitting_commits will be helpful.

[Okeanos]

I have now pushed the requested changes (the link did indeed help, thanks!) to this pull request:

1. update descriptions
2. rename page
3. add external ssh support

Each commit is, I think, correctly signed-off and contains only the relevant changes with a 'why' description.

As discussed and acknowledged the empty ssh folder(s) remain(s) and ssh-pageant.exe is also left untouched.

I also verified once again that the changes work as expected by building the installer and using it as an upgrade to an existing Git installation. The SSH Choice Page is only displayed if the checkbox for "Only show new options" is unchecked, i.e. all pages are shown, including previous choices. Should that be changed as well? If so, how do I do that? Is that lines 1077-1079 in install.iss?

Other than that, the integration works as I expected it to: git (bash) picks up the Win32 OpenSSH binaries and uses them, also making use of e.g. an SSH Key managed via KeePassXC in the Win32 ssh-agent.
Sadly, on the Git UI side this is a little less well supported 🤷‍♂️ :

• GitHub Desktop doesn't support it: Desktop does not use OpenSSH on Windows if running, favours embedded SSH desktop/desktop#5641
• SourceTree can be set to use OpenSSH (do not launch the agent requested, do not set a key file) but will throw a warning dialog on each start and when preferences are changed complaining about broken ssh-agent setup. It does work, though.
• Sublime Merge works if system provided Git is used; bundled Git does not talk to Windows OpenSSH (Support Windows supplied OpenSSH with bundled Git sublimehq/sublime_merge#1232)
• ...

However, on the bright side editors/IDEs such as Visual Studio Code's default Git integration and IntelliJ IDEA both were able to successfully use it out of the box :)

In essence, it's a mixed bag depending on individual tools but I wanted to at least verify this change works across some tools and can be used to improve developer experiences.

[dscho]

Thank you for patiently addressing my feedback!

[Okeanos]

You are welcome; thank you so much for your support and patience when dealing with my questions as well. Making sure your feedback is addressed was the least I could do :) – so cool this happened!

[dscho]

FWIW I quickly added a commit on top that will hide the PuTTY option again, unless at least one PuTTY session was found. I'm in the process of verifying that it works as intended, and will probably push the result shortly.

[dscho]

Forgot to send this:

The SSH Choice Page is only displayed if the checkbox for "Only show new options" is unchecked

I think that is still correct because technically the user saw this page, even if there is now a new option available.

[dscho]

FWIW I quickly added a commit on top that will hide the PuTTY option again, unless at least one PuTTY session was found. I'm in the process of verifying that it works as intended, and will probably push the result shortly.

Pushed: 53f76b9

[Okeanos]

Forgot to send this:

The SSH Choice Page is only displayed if the checkbox for "Only show new options" is unchecked

I think that is still correct because technically the user saw this page, even if there is now a new option available.

I am not entirely sure: the PuTTY Page only appeared if PuTTY was found to begin with. Now it should appear if PuTTY is found or an alternative ssh.exe exists on the PATH. The latter is entirely new and should be shown to all upgrading users, even if they never had PuTTY on their system but do have an alternative ssh.exe. I would fall into the latter category and wouldn't be shown this page unless I explicitly replay all choices, i.e. uncheck "only show new options".

I'll defer to your choice, though.

[carenas]

Not sure what might had gone wrong with the integration of this series, but using the rc2 installer as both an upgrade from 2.32.0 and clean install still leaves the bundled ssh files behind even if the option to use the external ssh was selected and it is available in path.

@dscho: I am setting up a development environment to see if I can debug this, but thought it was worth a heads up.

@Okeanos: did you confirm this works with the installer from?:

https://github.com/git-for-windows/git/releases/tag/v2.33.0-rc2.windows.1

FWIW, an installer build with the SDK from the latest commit (using 2.33.0-rc2) seems to work. unlike the official build

[Okeanos]

@carenas I did not try the official RC builds yet but did use SDK built installers as documented in this pull request's comments. I'll certainly check on this later today and give additional feedback here.

[dscho]

Oy. The only thing I could think of to go wrong is that maybe the function in config.inc.iss isn't generated at all, for whatever reason :-(

@carenas would you have time to analyze the logs of the Git artifacts check of the -rc2 PR? I am on a phone, and technically offline today, which makes it a bit difficult for me to investigate.

[carenas]

@dscho: sure, but I am not really sure what to look at
@Okeanos: FWIW I couldn't get it to work either with the rc1 installer

[dscho]

@dscho: sure, but I am not really sure what to look at

(I briefly interrupted my day off to look at this.) What I did was to open https://dev.azure.com/git-for-windows/git/_build/results?buildId=82662, go to the job where the installer was built, then selected the step where the installer was built, then clicked on the magnifying glass symbol above the logs and typed in "openssh", then continued to search until I found the correct spot.

I am fairly certain that the problem lies with https://dev.azure.com/git-for-windows/git/_build/results?buildId=82662&view=logs&j=fd84893c-d8cf-515f-6f6b-748e5f0b9fc1&t=53291ad3-d296-5424-da40-80462f49966c&l=31232:

D:/a/1/s/git-sdk-64-build-installers/usr/src/build-extra/installer/release.sh: command substitution: line 284: syntax error near unexpected token `('
D:/a/1/s/git-sdk-64-build-installers/usr/src/build-extra/installer/release.sh: command substitution: line 284: `comm -12 <(echo "$LIST" | sort) <(pacman -Ql openssh | sed -n 's|^openssh /\(.*[^/]\)$|\1|p' | sort) |'

[dscho]

So the problem lies with sh.exe vs bash.exe (even if they are identical over here, they must do something slightly different based on their basename):

$ sh -c 'comm -12 <(printf "%s\n" 1 2) <(printf "%s\n" 2 3)'
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `comm -12 <(printf "%s\n" 1 2) <(printf "%s\n" 2 3)'

$ bash -c 'comm -12 <(printf "%s\n" 1 2) <(printf "%s\n" 2 3)'
2

[dscho]

I hope that this is fixed in 863470a and I started https://dev.azure.com/git-for-windows/git/_build/results?buildId=82722&view=results. @carenas could I ask you to monitor that build and test?

[dscho]

It probably tries to follow POSIX more strictly when called as sh

That's my guess, too, but this is the first time I see it in action (that I remember).

and the <(foo) construct isn't POSIX.

Indeed!

We could try to emulate it with fifos, though.

I "emulated" it by writing to intermediate files instead. That definitely works always, even if you mix and match MSYS with MINGW executables (the latter cannot handle MSYS-emulated fifos).

[dscho]

I hope that this is fixed in 863470a and I started https://dev.azure.com/git-for-windows/git/_build/results?buildId=82722&view=results. @carenas could I ask you to monitor that build and test?

Darn it. It failed, of course, with:

Already tagged: v2.33.0-rc2.windows.1

Which is technically correct.

So I kicked off the GitHub workflow that should be equivalent: https://github.com/git-for-windows/git/runs/3320018811?check_suite_focus=true.

[carenas]

@dscho: failed again (x86 first and canceled x86_64), IMHO might be a good idea to use a new tag

[dscho]

I kicked off an installer-x86_64-only run: https://github.com/git-for-windows/git/actions/runs/1126976271

[dscho]

And I pushed a dummy revision so that I could kick off https://dev.azure.com/git-for-windows/git/_build/results?buildId=82726&view=results.

[carenas]

@dscho: failed again in a similar way than the last x86 run:

+ cp PKGBUILD.v2.33.0-rc2.windows.1-20210813082640 PKGBUILD
cp: cannot stat 'PKGBUILD.v2.33.0-rc2.windows.1-20210813082640': No such file or directory
Error: Process completed with exit code 1.

[carenas]

And I pushed a dummy revision so that I could kick off https://dev.azure.com/git-for-windows/git/_build/results?buildId=82726&view=results.

this one still running, and hopefully complete since it has a different "tag"

[dscho]

I think this one worked: https://dev.azure.com/git-for-windows/git/_build/results?buildId=82726&view=logs&j=fd84893c-d8cf-515f-6f6b-748e5f0b9fc1&t=53291ad3-d296-5424-da40-80462f49966c&l=37234

[carenas]

and has been tested and confirmed golden!

[dscho]

@carenas thank you so much for noticing, pointing out, and verifying the fix.

[dscho]

Having said that, I think we have another problem on our hands:

$ git ls-remote <ssh-host>
Warning: Permanently added the RSA host key for IP address '40.74.28.12' to the list of known hosts.
CreateProcessW failed error:193
ssh_askpass: posix_spawnp: Unknown error
Permission denied, please try again.
CreateProcessW failed error:193
ssh_askpass: posix_spawnp: Unknown error
Permission denied, please try again.
CreateProcessW failed error:193
ssh_askpass: posix_spawnp: Unknown error
git@ssh.dev.azure.com: Permission denied (password,publickey).
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

This happens when I try to access an SSH host without having a private key. And I think the reason is this:

$ env | grep ASKPASS
SSH_ASKPASS=/mingw64/libexec/git-core/git-gui--askpass

The git-gui--askpass file implements a Tcl/Tk script, with a #! line that Windows' OpenSSH obviously doesn't know how to handle.

I think what we can do is to use GCM Core's askpass helper (which is a .exe that Windows' OpenSSH can spawn without problems). Like this:

$ SSH_ASKPASS=/mingw64/libexec/git-core/git-askpass.exe

After that, I am greeted with this dialog when I try to connect:

The SSH_ASKPASS variable gets defined in /etc/profile.d/env.sh (a file which is tracked here: https://github.com/git-for-windows/build-extra/blob/HEAD/git-extra/env.sh).

I guess we could improve things by testing whether /mingw64/libexec/git-core/git-askpass.exe exists here and prefer it over the Tcl/Tk script.

[rimrul]

Avoiding a Tcl/Tk based askpass might be a good idea in general, when I think back to #234.

[rimrul]

Looking at the GCM core repo, they don't seem to ship a git-askpass.exe, yet. I think the one you found is from the classic GCM for Windows.

[Okeanos]

@dscho thank you so much for spending the time and fixing this on so short notice and everything ❤️ . If there is anything I can do to try and help with the git-askpass.exe (which I obviously failed to notice during my tests as well) problem please say so.

[dscho]

Looking at the GCM core repo, they don't seem to ship a git-askpass.exe, yet. I think the one you found is from the classic GCM for Windows.

Oh, I think you're right... 🤦

Avoiding a Tcl/Tk based askpass might be a good idea in general, when I think back to #234.

That's a good point. We should reimplement a pure Win32 askpass.exe. For obvious reasons, we cannot call it git-askpass.exe (because it would clash with GCM's executable).

[dscho]

we cannot call it git-askpass.exe (because it would clash with GCM's executable).

Oh, now that I said it: I think we actually can call it git-askpass.exe, because we will install it into bin, not into libexec/git-core...

[rimrul]

I've created a pretty barebones Win32 askpass in #371.

[PetSerAl]

Should also check for SSHChoicePage<>NIL.

[carenas]

Isn't it implied?, how could you get this Checked if there was no form to select it?

[PetSerAl]

There is third option RdbSSH[GS_ExternalOpenSSH] was never initialized, thus referencing to Checked property is not allowed. git-for-windows/git#3368

[Okeanos]

Sorry about that; I didn't have a Windows 7 installation (or any Windows installation prior to the Win32 SSH rollout) around to test this with and it didn't occur to me that I could've used the IE Developer VMs to test this, until now. Interestingly, the Win7 IE 11 VM I downloaded comes with OpenSSH preinstalled.