[WIP] libssh backend #5253
[WIP] libssh backend #5253
Conversation
8fd3847 to
33ea3d1
Compare
33ea3d1 to
57e1f8e
Compare
|
I forgot to tag the crux of the issue in the PR's cover, but please see #5225 for context. To clarify my perspective on #5307, ideally This is not currently handled here, 8000 because I hit one case where the interface we already provide cannot be ported to libssh (the custom signing mechanism), so here's a summary of "currently known issues caused by libssh2": apparently it's gone from Debian (#5225), some requested features are missing (#5258, #5219, #4452), or depend on the libssh2's backend and key types supported (#4289, OpenSSL/libgcrypt only), and IIRC there's another issue… |
There was a problem hiding this comment.
I have just a few notes to fix the libssh2 build.
After that, the testsuite passes for me with -DUSE_SSH=libssh2, but the online test fails with -DUSE_SSH=libssh:
3/6 Testing: online
3/6 Test: online
Command: "/home/jistone/src/libgit2/build-libssh/libgit2_clar" "-v" "-sonline"
Directory: /home/jistone/src/libgit2/build-libssh/tests
"online" start time: Feb 19 13:41 PST
Output:
----------------------------------------------------------
Loaded 383 suites:
Started (test status codes: OK='.' FAILURE='F' SKIPPED='S')
online::badssl...S
online::clone.........S..S.FS.SSF..SSS.S.
online::fetch...........
online::fetchhead......
online::pushSSSSSSSSSSSSSSSSSSSSS
online::remotes......
1) Failure:
online::clone::ssh_auth_methods [../tests/online/clone.c:505]
Function call failed: (git_clone(&g_repo, "ssh://github.com/libgit2/TestGitRepository", "./foo", &g_options))
error -1 (expected -7) - failed to start SSH session: Failed to process system configuration files
2) Failure:
online::clone::certificate_invalid [../tests/online/clone.c:703]
Function call failed: (GIT_ECERTIFICATE)
error -17 (expected -1) - failed to start SSH session: Failed to process system configuration files
<end of output>
Test time = 19.19 sec
----------------------------------------------------------
Test Failed.
"online" end time: Feb 19 13:41 PST
"online" time elapsed: 00:00:19
This is on Fedora 31, libssh-0.9.3-1.fc31.x86_64, if that makes a difference.
| @@ -52,7 +52,7 @@ OPTION(TAGS "Generate tags" OFF) | |||
| OPTION(PROFILE "Generate profiling information" OFF) | |||
| OPTION(ENABLE_TRACE "Enables tracing support" OFF) | |||
| OPTION(LIBGIT2_FILENAME "Name of the produced binary" OFF) | |||
| OPTION(USE_SSH "Link with libssh2 to enable SSH support" ON) | |||
| OPTION(USE_SSH "Enable SSH support. Can be set to a specific backend (libssh2)" ON) | |||
There was a problem hiding this comment.
I think this needs to be SET to properly cache the backend value, like:
| OPTION(USE_SSH "Enable SSH support. Can be set to a specific backend (libssh2)" ON) | |
| SET(USE_SSH "ON" CACHE STRING "Enable SSH support. Can be set to a specific backend (libssh2)") |
|
@tiennou is this still worked on? |
6d5a883 to
d5ef696
Compare
|
@extrawurst I've rebased it, updated CI, and the only showstopper now is that the @cuviper Thanks for the review, much appreciated ❤️. Issues (apart from the runtime config ones, and unforeseen bugs 😉) should be fixed. |
0dce737 to
02e7c42
Compare
There's a fundamental conflict between the goals of the library and the realities of how different all these libraries and network protocols are as we both want to
The former goal means things like auth can get awkward as most HTTP auth doesn't look like SSH auth (and some HTTP auth doesn't look like a lot of other HTTP auth) but the latter means that we want to avoid processing the data too much when the end-user application (or whatever ends up calling the library) needs to interact with the library implementing the network protocol. This is made even more important by the fact that we want to allow callers of the library to plug in their own network layer and then they can devise an arbitrarily complex authentication method as long as both agree. The wish to have the caller in control but also make it easy to perform the most common operations leads to weird things in the API like libssh2- and curl-specific which try to give the caller all the info they might need short of handing over a pointer to whatever context/handle each library has. At the same time it's rather disappointing how the information is presented by some libraries (it seems libcurl does the data processing we're trying not to do here), but that's a different discussion. As far as your question about the callback types having specific libssh2 structures, I remember doing this due to the methods needed for the more complex interactions like keyboard-interactive, seem to be tied enough to libssh2 that did not seem realistic that we could provide some generic method for this. And from what I can tell, libssh has a rather different mechanism for doing this. Maybe it's not very sensible for this library to support this. The same goes for certs where we only got two hashes from libssh2. It looks like the issue described in #5258 and #4452 has been fixed now, though it's not super helpful now that libssh2 isn't in some distros, I suppose. But we do have a place to put it so the caller can decide for themselves how they want to hash the cert if they need to. libssh expects you to ask for which hash you're interested in, which makes sense for it, but I don't know how much it makes for us to expose that detail. For what it's worth, I have come to think that the design with the callbacks for host and user authentication were a mistake, but changing it to work with returning control to the user by returning is not a small task. Ultimately, some of this probably isn't solvable as part of this library. We can try to keep it working with the features we have (and maybe not bother with keyboard-interactive for libssh?) and with libssh we can even fill in the info as to whether "we" think the ssh server is something you should trust. But we're still just not going to provide functionality for managing your All this to say that attaching auth logic into libgit2 is always going to leave us with something somewhat annoying and the real solution is probably to provide some simple options in the library and leave an escape hatch for something like ssh where the amount of complexity is high and also all of this should be very much outside of the things we should have to care about. Maybe at a later date we can have even built-in transports look like things you plug in so you can keep your own pointer to something if you need more than bog standard behaviour. But that's for when many other pressing problems have been solved. |
| * - GIT_FEATURE_SSH | ||
| * Libgit2 supports the SSH protocol for network operations. This requires | ||
| * the libssh2 library to be found when compiling libgit2 | ||
| * @return A combination of GIT_FEATURE_* values. |
There was a problem hiding this comment.
Are these changes meant to be in this PR?
There was a problem hiding this comment.
Not really. Arguably, I wanted to hide references to libssh2, it would look better IMHO in docurium to have the enum values documented directly, and that's DRYer.
| unsigned char *hash_ptr = (unsigned char *)&hash; | ||
| size_t hash_len; | ||
|
|
||
| if (ssh_get_server_publickey(s->session, &pkey) == SSH_OK) { |
There was a problem hiding this comment.
This block should also add the raw key like we do for libssh2
There was a problem hiding this comment.
Noted. IIRC, there's been some underlying changes to the main code while this was in-flight (SHA256 comes to mind).
There was a problem hiding this comment.
Fixed. Note that I'm putting the base64-encoded raw data, because that's what libssh provides.
| goto done; | ||
| } | ||
|
|
||
| /* We don't currently trust any hostkeys */ |
There was a problem hiding this comment.
We should ask libssh whether the hostkey is known and fill in that parameter in the callback.
src/transports/ssh_libssh.c
Outdated
| assert(c->username); | ||
| assert(c->privatekey); | ||
|
|
||
| rc = libssh2_userauth_publickey_frommemory( |
There was a problem hiding this comment.
There are a series of ssh_pki_import_privkey_ functions provided by libssh which let you specify an in-memory key in different formats. The one that would let us keep the same for both looks like it would be ssh_pki_import_privkey_base64 as it oddly doesn't seem to allow you just pass the raw data, so we'd have to first convert it into base64.
There was a problem hiding this comment.
Or it looks like they might mean base64 as in the format you would find on-disk
There was a problem hiding this comment.
Sorry, I'm confused because that's the libssh2 backend, and to be fair, Well, silly me, that's an unported #ifdef block 😅. I hadn't worked at all with libssh beforehand, so I'll have to revisit that. There's a bunch of things that can happen which makes key-handling inconsistent — it's mathematically possible to derive the pubkey from its private counterpart, but not all libssh2 backends handle that. I haven't checked if libssh handles key derivation (if my crypto-fu is correct 😉).
There was a problem hiding this comment.
A few notes, sadly it A3E2 's been a while so the context-switch is kinda cold on my side but that's what you get when you drop off the face of the earth.
Just to clarify, the libssh backend as it is should now handle "our" keyboard-interactive interface via a trampoline that just collects all challenges and converts them. It's just that actually setting up a test for that requires a harness/configuration which I haven't really looked at, so it's untested.
The thing that's (IIRC) completely missing from libssh is a way to support our custom hashing. Conversely, libssh seems to refer to GSSAPI mic authentication, which is absent from libssh2 (and gave me a good chuckle as a long-time Uplink player — I wasn't aware that voice authenticated SSH was a thing 😅).
I do agree that, if I was designing this from a blank slate, I'd go for a much more simple "try once with what the user gave us (pass, file/memory pkey, whatever, even a blanket this-asked-for-auth void * handle), or even and fail immediately with no retries", but API-wise that's not possible.
Anyway, I'll try to revisit that soonish, thanks for the review @carlosmn !
| * - GIT_FEATURE_SSH | ||
| * Libgit2 supports the SSH protocol for network operations. This requires | ||
| * the libssh2 library to be found when compiling libgit2 | ||
| * @return A combination of GIT_FEATURE_* values. |
There was a problem hiding this comment.
Not really. Arguably, I wanted to hide references to libssh2, it would look better IMHO in docurium to have the enum values documented directly, and that's DRYer.
| unsigned char *hash_ptr = (unsigned char *)&hash; | ||
| size_t hash_len; | ||
|
|
||
| if (ssh_get_server_publickey(s->session, &pkey) == SSH_OK) { |
There was a problem hiding this comment.
Noted. IIRC, there's been some underlying changes to the main code while this was in-flight (SHA256 comes to mind).
src/transports/ssh_libssh.c
Outdated
| assert(c->username); | ||
| assert(c->privatekey); | ||
|
|
||
| rc = libssh2_userauth_publickey_frommemory( |
There was a problem hiding this comment.
Sorry, I'm confused because that's the libssh2 backend, and to be fair, Well, silly me, that's an unported #ifdef block 😅. I hadn't worked at all with libssh beforehand, so I'll have to revisit that. There's a bunch of things that can happen which makes key-handling inconsistent — it's mathematically possible to derive the pubkey from its private counterpart, but not all libssh2 backends handle that. I haven't checked if libssh handles key derivation (if my crypto-fu is correct 😉).
Previously, we would have trampled over whatever "another" user of the session had set. Let's make sure we strive to behave correctly — I'm glancing over some details of libssh2, for things like timeouts.
This splits the SSH transport in two parts, a generic implementation, and a lower level interface to better handle implementation differences.
02e7c42 to
839e36a
Compare
| int git_ssh_session_server_is_known(git_ssh_session *s, int *valid) | ||
| { | ||
| GIT_UNUSED(s); | ||
| *valid = 0; |
There was a problem hiding this comment.
This ensures that libssh2's behavior doesn't change (default to "can't say it's valid", a.k.a 0), but now libssh checks for known_hosts validity, while libssh2 doesn't (and it looks messy to handle appropri
E26B
ately). I'm not even sure if it makes sense to update the known_host file afterwards…
|
Just to add my 2 cents. GitKraken (https://www.gitkraken.com/) is using libgit2, but due to the underlying usage of libssh2, it is currently not possible to use the client when talking to git remotes on ssh-hardened servers (e.g. only ed25519 host keys, or restricted allowed-cipher-sets, ...). Seeing this go through would probably improve things significantly on that front :-) |
|
We have openssh support now — I really like libssh (and not so much libssh2) but it feels like openssh is the thing to use for "better openssh compatibility" |
This adds support for libssh as a possible backend for the SSH transport. The code is untested — the test suite runs fine locally, but I want to see how CI reacts to it as I don't expect everything SSH to be covered by it (
keyboard-interactive🙄).