agent: Add support for Windows OpenSSH agent

[yodaldevoid]

I threw this together over a few days loosely based on the corresponding code in OpenSSH.

Things left to clean up:

• test it
  • I have tested this with a quick program authenticating with git@github.com with just libssh2 and another program that clones a repo from Github over ssh using libgit2 (which uses libssh2). I don't see a way to run automated tests against the Windows OpenSSH agent currently as it has a hard-coded pipe path making it impossible to run two agents at the same time.
• Figure out license shenanigans
  • The license for the code modified from Portable OpenSSH is the two-clause BSD license under Markus Friedl and Microsoft Corp., separately. I have added the license information alongside the code section and added these two copyright holders to the COPYING file. There is an additional copyright by Tatu Ylonen that is (as far as I can tell) compatible and does not require any copy of outside of code, and as such it was also included alongside the code section. Please let me know if any of this has not been done correctly.
• Support async IO
  • Some of the underlying agent code supports async IO, but not all. As such, I have implemented support for async IO as best as I could, but the code is currently not used as the pipe is opened as synchronous. I've left a comment stating what needs to be done to enable asynchronous operations.

Fixes #501

[yodaldevoid]

I think this is on a technical level ready to merge now. Please let me know if you want me to clean up anything either code wise or commit wise.

[yodaldevoid]

Rebased on master and added support for partial transfers following #510.

[yodaldevoid]

I hate to be that guy, but I would like some forward movement with this sooner rather than later.

@willco007 @bagder @mback2k @vszakats

[bagder]

Wouldn't these commits be better squashed into a single one for readability and easier review? It makes it hard to review when the 5 subsequent commits change things in previous commits.

I don't know Windows specific things so I cannot comment on those specifics. In general this seems fine and @yodaldevoid says he's tested it. There should be very little risk.

[yodaldevoid]

I've squashed all commits as requested other than the one disabling overlapped IO. I feel that one commit should be left separate.

[yodaldevoid]

Once again being that guy.

I have resolved the issues brought up so far and there have been no further issues brought up within the past ~2 weeks. Would you folks be ok with merging this as-is or would you rather have a third party who knows Windows to take a look at it?

@willco007 @bagder @mback2k @vszakats

[willco007]

It looks OK to me at first glance, but I don't have a Windows box to test it with. It would be great if more Windows users would take a look at it.

[yodaldevoid]

I'm going to have to throw in the towel on finding someone else to test or review these changes. I've poked around off and on for the past month or so on various communities where I think I might find people who would be interested to help out, but I haven't gotten any bites. I'd rather not leave this PR to languish, but if we want to get more testing before it is merged, I don't see any good way forward.

[pkittenis]

I'd like to test this, and have an existing project that uses SSH agent and builds binary packages on Windows to test with.

I lack a Windows 10 machine to do the testing on at the moment, but should have some time this weekend to setup a VM for it.

[yodaldevoid]

@pkittenis Do you need any help making the changes required to test this or with setting up the test environment?

[gim913]

Hi, I have similar, possibly cleaner change that I've written from scratch.
I tested it by firing docker with ssh deamon, adding authorized_keys there and running example-ssh2_agent.exe against is.
transact is one-to-one copy of agent_transact_unix, with read/write ops changed.

master...gim913:openssh-windows

P.S. It does not support async io, as I think neither existing unix agent nor putty's pageant code support them.

P.S.2 It would be nice to have it included in any form, as libgit has dependency on libssh2...

[yodaldevoid]

@gim913 Would it be possible for you to test my set of patches using your docker method? Did you use any specific docker container?

I would prefer to get my set of patches merged, partially because they are more complete and partially because of how long I've waited to get these approved. That said, I don't see anything wrong with your patches, and as long as some sort of support for the Windows OpenSSH agent is added, I don't really care which set of patches are merged.

[gim913]

@yodaldevoid sure, will try to do this either tomorrow or this week 👍
P.S. just to let you know, I remember about this, just haven't got the time so far.

[gim913]

@yodaldevoid I've rebased your branch on top of master and I've made following test:

I'm using image that is generated for test purposes via Dockerfile in tests/openssh_server
First I generated public keys for the keys that are present there:

ssh-keygen.exe -y -f ssh_host_ed25519_key  > ssh_host_ed25519_key.pub
ssh-keygen.exe -y -f ssh_host_rsa_key > ssh_host_rsa_key.pub
ssh-keygen.exe -y -f ssh_host_ecdsa_key > ssh_host_ecdsa_key.pub

It just so happens, that ssh_host_ed25519_key.pub is also inside authorized_keys, how convenient!

Fire up the image in detatched mode:

docker run -d --name ssh-server -p 22:22 libssh2/openssh_server

Add ssh-key to running agent:

ssh-add tests\openssh_server\ssh_host_ed25519_key

Fire up example:

_build\example\Debug\example-ssh2_agent.exe 127.0.0.1 libssh2

Fingerprint: F3 CD 59 E2 91 3F 44 22 B8 0F 7B 0A 82 B2 B8 9E AE 44 93 87
Authentication methods: publickey,password,keyboard-interactive
        Authentication with username libssh2 and public key ***** ***
        Authentication with username libssh2 and public key ***** ***
        Authentication with username libssh2 and public key will@iCube.local succeeded!
all done!

[yodaldevoid]

I have put some time into creating a test ensuring that we can authenticate using a key stored in an agent. I have found that it is more complicated than it looks at first blush (at least for me). Long story short, I don't think it is currently feasible to create a test that would work across platforms for authenticating using an agent. I could add a test that just attempts to connect to an agent, but I'm not sure of the value of that.

The first problem is adding (and removing) a key to the agent. We can call commands from C easily enough so adding keys should be easy. All OpenSSH implementations work more or less the same and generally use a program named ssh-add so we should be fine there. The problem comes if we would like to support pageant for the test as it allows adding a key through the CLI, but not removing it.

The second problem then revolves around key file permissions. At least on Windows, there is a good chance that when the repo is checked out, the SSH private keys will be readable by too many users, thus causing ssh-add to refuse it. In Unix land this is easy enough to fix, but in Windows land the script to fix the permissions needs to be run in an admin command prompt. This prevents us from running this script in CI. There are possibly some work-arounds for this, but I have not found any yet. There is also the possibility that this is a moot point in CI as the repo could be checked out with the right permissions, though the possibility of this is slim.

Other than those two issues, the test runs without a problem. I have not pushed the code up due to the issues I have just listed and because the Windows CI seems to not be running the tests anyway so that needs to be resolved first. I will keep the code around for a future PR.

@willco007 @bagder @mback2k @vszakats
With the above information, how would you like to move forward?

[yodaldevoid]

@willco007 @bagder @mback2k @vszakats
I hate to be a broken record, but how do you folks want to move forward on this?

I see two options: block this on adding an ssh-agent test that can run on all platforms or merge it as is. As I think can be understood, I am partial to merging as is. That said if you, the maintainers, want to block this on some sort of automated test, I'd like to know that now so I can work towards that. To be clear, adding an automated test for this includes not only adding a cross-platform test (or a set of tests that are selected based on platform) but also fixing Windows tests in the CI runner as right now tests are not being run there.

Please provide some sort of direction forward. I have seen all but @mback2k voice some level of support for the code as is, and seeing as they are marked as "busy" I can understand why they have not chimed in.

[willco007]

At this point, short of writing Windows unit tests (and there are a lack of Windows devs on here), I think we should just take it. Any objections?

[yodaldevoid]

I've rebased on top of the current master. Not that it was needed since there were not merge conflicts, I just felt like making this merge as smoothly as possible.

[yodaldevoid]

It looks like a bunch of Linux tests failed due to Docker Hub rate-limiting. Not sure what to do with that.

[mback2k]

I will try to test this during this week.

[mback2k]

Except the structural code change suggested above (to make it easier to identify the code taken from another project) this looks good to me and I was successfully able to test this with the libssh2 example ssh2_agent. Thanks a lot!

[yodaldevoid]

I believe I have resolved all currently requested changes. Let me know if there is anything else that should be looked at.

[mback2k]

Yes, looks good to me. Thanks a lot! I also verified that it still works after the structural changes.

[BrianInglis]

Breaks Cygwin build in snapshots since 20210512 see issue #597.
Cygwin is a POSIX (was Solaris now Linux like) distro, and subsytem under Windows, supporting its own BSD sockets, network and ssh utilities and daemons running as Windows services, and X Window system display servers and remote clients.

[mback2k]

I guess we need to add some Cygwin CI builds then.

[yodaldevoid]

I am away for the next week and (probably) won't be able to work on this until then.

[mback2k]

I will try to work on it this weekend.

[BrianInglis]

Thanks for looking at this.
If you want to set up a Windows/Cygwin CI, the following may help you get started.
Cygwin may be set up semi-automatically by downloading the amd64/x86_64/64 bit and x86/32 bit package GUI installers and running them against a specified nearby mirror (no smarts here) to install the base packages into specified Windows directories used as Cygwin "root" directories for respective 32/64 bit installs. The default cross-built "native" Windows terminal is mintty which runs bash under Cygwin ptys, although MS cmd may also be used with limitations.
Cygwin and all the distro packages (including the base) are normally built using a package called cygport (based on Gentoo portage with some Fedora package build influences from the RedHat developers who are also current volunteers), a library of shell scripts, with a lot of builtin defaults and info about how Cygwin packages and build processes work, that checks assumptions and just "does the right thing".
Site/install/maintainer defaults may be customized using /etc/cygport.conf and ~/.cygport.conf.
Each package is built using the cygport package, which when installed pulls in all its dependencies which make up the default toolchain (autoconf, automake, bash, binutils, bzip2, coreutils, diffstat, diffutils, dos2unix, file, findutils, gawk, gcc-core, gcc-g++, grep, gzip, lftp, libtool, lndir, make, openssh, patch, perl-Authen-SASL, perl-MIME-tools, perl-Net-SMTP-SSL, rsync, sed, tar, texinfo, unzip, util-linux, wget, which, xz), and each package's own build dependencies specified in cygport (shell) variables DEPEND/BUILD_REQUIRES in each packages's $NAME.cygport shell source script.
In that script, build functions as well as variables may be overridden, for subpackages as well as main packages, used when building packages with libraries, where -devel and -doc subpackages may be built from the same .cygport script, and -debuginfo subpackages are automatically generated for binary library (shared DLLs) and (Win PE) executable packages.
Once the build dependencies have been manually pre-installed (also packages groff, help2man, and robodoc if docs need built), building and testing each package is as simple as running (under both Cygwin 64 and 32 bit install bash shells):

$ cygport libssh.cygport download all check

where download downloads upstream sources into a cache directory the first time it is used, all runs prep (create working directory $NAME-$VERSION.$ARCH, unpack sources and apply patches), compile (run all compilation steps including autoreconfig if autotools files are present, configure/make if those are present, make if a makefile is present), install (install into a DESTDIR, and run post-installation steps), and package (create binary and source packages as compressed tar files) steps.
For example, under my ~/src/cygwin/libssh2/ directory, I have:

$ l . libssh2-1.9.0-1.*86*
.:
cygwin-configure-ac.patch libssh2.cygport libssh2-1.9.0-1.i686/ libssh2-1.9.0-1.x86_64/

libssh2-1.9.0-1.i686:
build/ config/ CYGWIN-PATCHES@ dist/ inst/ log/ origsrc/ patch/ spkg/ src/ temp/

libssh2-1.9.0-1.x86_64:
build/ config/ CYGWIN-PATCHES@ dist/ inst/ log/ origsrc/ patch/ spkg/ src/ temp/

I've hacked up the production Cygwin libssh2.cygport to allow me to manually specify snapshots to check those builds, but the date expression should normally be used, as it compensates for time zones to download the latest available snapshot, commented out to download release tarballs, or further tweaked to use git repos as sources.
For more info see Cygport docs or you can find my DM on any of the Cygwin MLs so feel free to contact me at that address for help.

[yodaldevoid]

We should probably keep further discussion of the cygwin issue in #597 or a new issue.