search

rust-lang / git2-rs

libgit2 bindings for Rust
https://docs.rs/git2
Apache License 2.0
1.64k stars 380 forks source link

libgit2: Add support for OpenSSH instead of libssh2 #1031

Open bnjmnt4n opened 4 months ago

bnjmnt4n commented 4 months ago

This PR changes the original ssh feature into two new ones: ssh-libssh2 and ssh-openssh. By default, the ssh-libssh2 feature is enabled for backwards compatibility.

To use OpenSSH instead, the following listing in Cargo.toml can be used:

git2-rs = { version = "...", default-features = false, features = ["https", "ssh-openssh"] }

This PR is stacked on top of #1032.

Closes https://github.com/rust-lang/git2-rs/issues/1028.

bnjmnt4n commented 4 months ago

Hmm, let me try to see if I can figure out why the tests are failing...

Nemo157 commented 3 months ago

One issue I've noticed with this is when linking against a system library it doesn't detect whether it was compiled with openssh enabled, so the feature only really works when forcing the vendored library.

(EDIT: Well, also it shouldn't be allowing the system library since it's only 1.7.2, the 1.8.0 bump needs to update the pkg-config version range too).

bnjmnt4n commented 3 months ago

I can update the version range detected, but I'm not too sure how to proceed with detecting which version of the SSH backend libgit2 was compiled with, would appreciate some guidance on that.

ehuss commented 1 month ago

Let me try to summarize the possible problems with the issues of the system libgit2 being out of sync with the features here:

System git2-rs Result
neither neither attempts to use ssh is runtime error
neither libssh2 attempts to use ssh is runtime error, libssh2-sys gets initialized which should have no impact
libssh2 neither libgit2 calls libssh2_init(0), possibly causing issues with openssl on Unix (which wants LIBSSH2_INIT_NO_CRYPTO)
exec neither should "just work" with exec
neither exec attempts to use ssh is runtime error
exec libssh2 initializes ssh2, but uses exec, should be OK
libssh2 exec ssh2 may not be initialized properly (same as libssh2/neither above). libssh2::init() isn't called from the rust side, but is called by libgit2's init, which initializes OpenSSL on unix (because of missing LIBSSH2_INIT_NO_CRYPTO)

Would it be possible to dig into those two cases where the system has libssh2, but git2-rs thinks it is something else (exec or not enabled)? What is the consequence of libssh2 initializing OpenSSL a second time? Does that break assumptions in the rust-openssl crate's own initialization?

I assume another issue is just the user confusion. They may have the ssh-openssh feature enabled, but if the system git2 is not built with that, then it has no effect which could be confusing.

Are these cargo features only valid when using the vendored copy? In other words, they don't really have a meaningful behavior with a system libgit2?

Would it be helpful at all if libgit2 provided a function to indicate which ssh backend it was compiled with at runtime?

bnjmnt4n commented 1 month ago

Are these cargo features only valid when using the vendored copy? In other words, they don't really have a meaningful behavior with a system libgit2?

It seems so to me. There isn't any existing build-time detection of whether an enabled Cargo feature (HTTPS or SSH) matches the system libgit2's compiled features. So this is already an existing problem, although I suspect mitigated by the fact that most system libgit2s would be compiled with HTTPS + SSH.

I guess the simplest way would just be to detect during build-time if the SSH feature enabled by the user matches the SSH backend on system libgit2, and compile and use the vendored libgit2 if they don't match.

libgit2 can probably be modified to add the SSH backend type to https://github.com/libgit2/libgit2/blob/6c5520f334e5652d5f0476c00a3188d1d97754e7/src/libgit2/libgit2.c#L81-L97, but I'm not too sure how to compile the C code at build time to do this detection...

ethomson commented 3 weeks ago

libgit2 can probably be modified to add the SSH backend type to https://github.com/libgit2/libgit2/blob/6c5520f334e5652d5f0476c00a3188d1d97754e7/src/libgit2/libgit2.c#L81-L97, but I'm not too sure how to compile the C code at build time to do this detection...

I can help here, can you open an issue in https://github.com/libgit2/libgit2 and describe the outcome that you want? In other words, do you want to know if some HTTPS is enabled (IOW, keep GIT_FEATURE_SSH) and then also which HTTPS is enabled (IOW, add GIT_FEATURE_SSH_EXEC to disambiguate)?

extrawurst commented 1 week ago

@bnjmnt4n can you link this issue from when you opened one on the libgit2 repo, so we can track progress on this?

bnjmnt4n commented 2 days ago

I haven't created an issue, because as mentioned I'm not very familiar with the Rust build process and what would be the best way to detect if OpenSSH/libssh2 is enabled at build-time. I'm guessing that what @ethomson suggested (keep existing GIT_FEATURE_SSH flag and adding additional flags for SSH backend type e.g. GIT_FEATURE_SSH_EXEC) would work, but someone with more knowledge on how to do this might want to chime in.