pygit2 icon indicating copy to clipboard operation
pygit2 copied to clipboard

Add SSH support on Windows platforms

Open omniproc opened this issue 4 years ago • 16 comments

So pygit2 now comes with a pre-compiled libgit2. That's nice. But on Windows platforms it seems that SSH support is not enabled in that pre-compiled libgit2 version. Even if one installs libssh2, the included libgit2 will not work with SSH.

E.g.:

bool(pygit2.features & pygit2.GIT_FEATURE_SSH)

Will still report false. Not sure if I'm missing something here but if I understand this correctly then it would be nice if the included libgit2 would enable SSH support and eventually even come with a pre-compiled libssh2 for Windows platforms.

I saw that this was on the todo list here: https://github.com/libgit2/pygit2/issues/902 But it's not ticked and I'm not sure it was ever done.

omniproc avatar Apr 01 '20 14:04 omniproc

So after some digging it should be able to build libssh for windows on the current appveyor plattform with something like:

git clone --depth=1 -b libssh2-1.9.0 https://github.com/libssh2/libssh2.git %LIBSSH2_SRC%
cd %LIBSSH2_SRC%
cmake -DCRYPTO_BACKEND=WinCNG -DBUILD_SHARED_LIBS=ON -DCMAKE_INSTALL_PREFIX=%LIBSSH2%
cmake --build . --target install

and then integrate it into libssh2 with the DEMBED_SSH_PATH e.g.:

git clone --depth=1 -b v1.0.0 https://github.com/libgit2/libgit2.git %LIBGIT2_SRC%
cd %LIBGIT2_SRC%
set CMAKE_INCLUDE_PATH=%LIBSSH2%/include
set CMAKE_LIBRARY_PATH=%LIBSSH2%/lib
cmake . -DBUILD_CLAR=OFF -DCMAKE_BUILD_TYPE=Release -DEMBED_SSH_PATH=%LIBSSH2% -DCMAKE_INSTALL_PREFIX="%LIBGIT2%" -G %GENERATOR%
cmake --build . --target install

Currently this fails in my local build environment due to unresolved linking issues but I was only able to test this with VS 2019, so it might have to do with my build env beeing too new.

omniproc avatar Apr 06 '20 14:04 omniproc

This would be very nice. Maybe you can make a branch and let AppVeyor build, see whether it works with VS 2015?

I know very little about Windows, maybe @fourplusone can help?

jdavid avatar Apr 08 '20 11:04 jdavid

External build already works even if I only fork. But the need to push and wait (re-building libgit2 every time) while debugging isn't an optimal workflow as you can imagine 😅 However I'll try to setup a reproducable Docker for Windows based build environment and can document that later on if I can get it running.

omniproc avatar Apr 10 '20 06:04 omniproc

Okay, the initial approachusing Docker for Windows failed. At least for me the current Windows Containers implementation, even on Server 2019, seems to unstable to use. I also fiddled around with vcpkg which would have potential to replace the current libgit2 build process for a process with less dependencies on the build env. However I wasn't able to embedd libssh2 into libgit2 with that approach.

So the main issue here seems to be that altought provided the libssh2 dependencies can't be resolved. You can check the last lines of my test build. and find the unresolved external symbol errors there.

I then noticed that libgit2 seems to support dynamic linking of libssh2 at runtime. So if libssh2 is present, It'll use it. This seems to be possible on Windows as well. However: that would have some implications since then we'd need to include libssh2 binaries and all the binaries that it depends on in the wheel for Windows as well. So far this would mean something like this:

if compiler_type == 'msvc':
            libgit2_dlls.append('git2.dll')
            libgit2_dlls.append('libssh2.dll')
            libgit2_dlls.append('zlib1.dll')
            libgit2_dlls.append('libcrypto-1_1-x64.dll')
            libgit2_dlls.append('libssl-1_1-x64.dll')

into the setup.py. This seems very error prone and thus I think it's still way cleaner to have libssh2 embedded into libgit2. We just have to figure out how to do it. The documentation on this is non existent or false at best. I guess this needs some deeper digging in the source of libgit2 and someone with more C++ and msvc/cmake exp. then I have.

omniproc avatar Apr 14 '20 13:04 omniproc

So I got this compiling finally. ssh support is available on Windows. But libgit2 seems to be failing to actually make any ssh connection.

key=".path/to/id.key"
crt="./path/to/id.pub"
origin = "ssh://[email protected]/some/repo/where/ssh/keys/have/been/added"
keypair = pygit2.Keypair("git", crt, key, "")
callbacks = pygit2.RemoteCallbacks(credentials=keypair)
pygit2.clone_repository(origin, path, callbacks=callbacks)

Works on Linux, but when I run it on Windows with the embedded libssh2 I currently get: _pygit2.GitError: failed to start SSH session: socket disconnect, which seems to be from here in libgit2. And this seems to come from libssh2 itself: LIBSSH2_ERROR_SOCKET_DISCONNECT.

Just not sure why the socket is beeing disconnected yet. So what I could provide is a build process for the libssh2 integration into the libgit2 binary of the windows build. But since libssh2 seems not to be working currently and fixing this is beyond me I'm not sure where to go from here. Still include it and open a new issue with libssh2?

omniproc avatar Apr 16 '20 07:04 omniproc

Cool, I think it's still worth building with libssh2.

Ideally I think libgit2 should provide binaries for Windows, so bindings don't have to. This would avoid duplicating the work in every binding, and also would speed up CI. Maybe it's worth checking what other bindings do, such as the one for Ruby.

ping @ethomson

jdavid avatar Apr 20 '20 11:04 jdavid

Cool, I think it's still worth building with libssh2.

To be clear up front: this is ultimately Your Decision, not mine. 😁 But I do want to give some background:

The LibGit2Sharp project has decided not to ship libssh2. The rationale is that the team decided that if they were to distribute the binaries of libssh2 then they would also has to do the work of responding to critical security releases. If there's a security vulnerability in libssh2 that they were shipping then that would become a security vulnerability in LibGit2Sharp that needed to be addressed. The team, at the time, was not eager to "own" that process.

Although that means - much like the status quo of pygit2 - that users will not get SSH support by default and would need to build their own binaries. In practice, nobody does this.

I don't have in-depth knowledge about any other bindings, I'm afraid. I don't think that any of them ship libssh2, but I could be mistaken.

I then noticed that libgit2 seems to support dynamic linking of libssh2 at runtime. So if libssh2 is present, It'll use it. This seems to be possible on Windows as well. However: that would have some implications since then we'd need to include libssh2 binaries and all the binaries that it depends on in the wheel for Windows as well.

It does not support dynamic loading during runtime. The PR you're linking to was not merged for a plurality of reasons.

Just not sure why the socket is beeing disconnected yet.

Nor am I, sorry. I know very little about libssh2.

ethomson avatar Apr 21 '20 08:04 ethomson

The LibGit2Sharp project has decided not to ship libssh2. The rationale is that the team decided that if they were to distribute the binaries of libssh2 then they would also has to do the work of responding to critical security releases. If there's a security vulnerability in libssh2 that they were shipping then that would become a security vulnerability in LibGit2Sharp that needed to be addressed. The team, at the time, was not eager to "own" that process.

Hm, but couldn't that process be much simplified and automated? E.g.: have a fixed tagged release and in addition to that build a nightly of that release that will use the latest release of libssh2. If it compiles (including unittests): fine. Ppl. concerned with the particular flaw can just grab the nightly that includes the patch. If it doesn't compile (including unittests) it's something that needs to be addressed anyways by libgit2.

omniproc avatar Apr 21 '20 10:04 omniproc

Thanks @ethomson for the feedback.

Actually we're statically linking libssh2 in Linux wheels. I'd rather do the opposite of what we do now: drop libssh2 from the Linux wheels and add it to the Windows wheels. Don't know about the situation with MacOS wheels.

Ping @webknjaz @rcoup

jdavid avatar Apr 24 '20 11:04 jdavid

@jdavid I think it's important to have libssh2 shipped in manylinux1 wheels. Users who want pygit2 linked against their system libssh2 would probably use the dists that are coming from OS package managers packaged by their respective maintainers (talking about homebrew, deb, rpm, ebuild etc). The main reason I contributed manylinux wheels was that I won't have to set up the env with a build toolchain for installing from sdist (nobody wants this usually).

Also, I agree with @m451 — if building and publishing new wheels is well-automated, it's pretty easy to release often, mostly targeting the latest version of libssh2. You could even do post-releases (as per PEP 440) if only the bundled libssh2 version has changed.

Another option would probably be to depend on something external like https://pypi.org/project/ssh2-python/#history but it looks poorly maintained.

webknjaz avatar Apr 24 '20 12:04 webknjaz

Ok, let's keep the current status quo then.

Just have to add libssh2 to the Windows wheels. No idea about the socket disconnect error though.

jdavid avatar Apr 24 '20 16:04 jdavid

Okay. I'll test and provide a working PR based on appveyor.

omniproc avatar Apr 24 '20 16:04 omniproc

When I was looking at macOS wheels I found this, which basically implied libssh2 isn't recommended, and doesn't do what people need (modern EC crypto, handling openssh config files, etc). I was planning to revisit once libgit2 switches to libssh.

https://github.com/libgit2/libgit2/issues/5225

rcoup avatar Apr 27 '20 14:04 rcoup

Has there been any movement on this? having SSH support in windows would be a huge help for me.

wmtorode avatar Sep 17 '20 05:09 wmtorode

I didn't find the time to add the wheel to the windows lib yet but even with it, it would not be working on Windows (yet), see socket disconnect error that would need some deeper digging.

omniproc avatar Sep 21 '20 21:09 omniproc

What is the current status on this? Has any work been done?

mg-christian-axelsson avatar May 24 '22 09:05 mg-christian-axelsson