GUACAMOLE-745: Support OpenSSH private keys & ED25519

[roysjosh]

Let libssh2 parse PEM and ssh-native keys.

[roysjosh]

This was tested using docker builds with bookworm as the base. Older Debian releases lack support for ed25519 in libssh2 due to compilation against gcrypt insteadl of OpenSSL.

[necouchman]

A couple of comments, here:

• The pull request needs to be limited to a specific JIRA issue, or split out into two different pull requests. I'd recommend just picking one and going with it, and we can close out the other JIRA issue as "resolved by XXX".
• Does this introduce any minimum version requirements for libssh2 that might impact people currently using it? I'm thinking, most specifically, of the stock libssh2 included with things like CentOS/RHEL7, which is probably going to be a bit outdated. We should, at the very least, know if it introduces a version requirement, and, if it does, discuss if we're good having the code move to that minimum version requirement (and deal with it in configure, Makefile, etc.), or support both.

[roysjosh]

Hey @necouchman, thanks for taking a look. Switching to libssh2 parsing the keys technically solves both issues (so long as libssh2 is compiled with OpenSSL) so I'll arbitrarily pick 745.

libssh2_userauth_publickey_frommemory was added in libssh2 1.6.0 (June 12 2015) so I think we're okay from that perspective:
https://github.com/libssh2/libssh2/blob/5627b82be66135dbda6423ae10bcad1c9ac86f9d/docs/libssh2_userauth_publickey_frommemory.3#L54

ed25519 & OpenSSH key support were added in 1.9.0. It looks like CentOS 7 has 1.8.0. I'll try to find time this week to test this patch on CentOS 7 and see if anything explodes. Hopefully PEM RSA/DSA, both encrypted and plaintext, continue to function...

[roysjosh]

Oh, I think this PR will break the agent. I forgot about that. Please consider this PR as a draft until I figure that out.

[roysjosh]

It looks like the agent hasn't been touched in a long time and probably has been unable to compile since at least 0fcea27 in 2015. It also looks like the agent was relying on an out-of-tree patch for libssh2 since it was conceived back in 2013. This PR will break it more. Should I try to avoid that or are you okay with proceeding @necouchman ?

[roysjosh]

I have tested a hacked-up Dockerfile with centos:centos7 as the builder and runtime. RSA and DSA keys in the "legacy" PEM format, both with and without a password, function as expected.

[necouchman]

@roysjosh Interesting on the SSH agent bit - I don't think this PR need necessarily address that. We should probably spin up a separate JIRA issue for it.

Other than that, if nothing else significant is broken, I think we're probably okay - making 1.6.0 the minimum from 2015 doesn't seem terribly unreasonable to me.

[roysjosh]

I modified the libssh2 configure check to look for the libssh2_userauth_publickey_frommemory function. Let me know if you want to take another route.

[roysjosh]

@necouchman let me know if you need anything else here. I'd be happy to contribute the centos Dockerfile I used to test.

[routerino]

Out of curiosity, would this merge be a breaking change? IE: if this pull request is merged and guacamole is deployed with an older (1.8 or earlier) version of libssh2, is that gracefully handled? If so there shouldn't be any blocking reason to merge the pull request.

[mike-jumper]

@routerino It is gracefully handled here in the sense that SSH support will simply not be built if that core required function is absent. I think it should be OK to start requiring at least libssh2 1.6.0+.

[mike-jumper]

With these changes, is it possible to now remove those common-ssh/dsa-compat.h and common-ssh/rsa-compat.h headers? (And possibly other direct OpenSSL includes)?

If those are no longer needed as of these changes, then those could be removed and these changes would possibly have the additional, happy effect of fixing the Ubuntu devel build (which is currently failing due to the newer OpenSSL):

https://ci-builds.apache.org/job/Guacamole/job/guacamole-server-master-ubuntu/532/JENKINS_LABEL_EXPRESSION=ubuntu,UBUNTU_RELEASE=devel/console

[roysjosh]

@mike-jumper it looks like those can indeed be removed. I'll push that shortly.

[roysjosh]

@mike-jumper compile tested & pushed

[mike-jumper]

Thanks, @roysjosh!

[mike-jumper]

This is definitely looking much better.

[roysjosh]

@mike-jumper here's a WIP guac_strnstr. I can add some comments to the header file if the approach looks good to you + any other changes needed.

[mike-jumper]

@mike-jumper here's a WIP guac_strnstr. I can add some comments to the header file if the approach looks good to you + any other changes needed.

@roysjosh Looks great (including the unit test)! I'm happy with the approach and see no need for any changes beyond adding documentation in the header.

[mike-jumper]

The unit test, the new function, and the new logic leveraging that new function all look good to me here. Except for this documentation nitpick, I think we're good to go.

[roysjosh]

Thanks :)