rust-lang / git2-rs

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

Update libssh #872

Closed darakeon closed 1 year ago

darakeon commented 2 years ago

Recently I opened an issue at libgit2 asking to upgrade libssh version, because after github updated their polices I could not fetch my repo anymore. But they pointed the the libraries that use libssh bring their own libssh.

I thought about opening a pull request, but got lost trying to upgrade it. Could not find a way to do this.

Original issue in libgit2: https://github.com/libgit2/libgit2/issues/6381

Reproduction steps

Create a SSH key to communicate with github.

ssh-keygen -t ecdsa -C "your-email"

Create a rust program with cargo that tries to pull a repository using fetch. Use the callback as:

fn git_credentials_callback(
    _url: &str,
    username: Option<&str>,
    _cred_type: CredentialType,
) -> Result<Cred, Error> {
    let public = get_key_path(Some("pub"));
    let private = get_key_path(None);

    let password = env::var("GIT_PASSWORD")
        .expect("No GIT_PASSWORD environment variable found"); // or some hardcoded password

    println!("{:?}", _cred_type);

    let cred = Cred::ssh_key(
        username.unwrap(),
        Some(public.as_path()),
        private.as_path(),
        Some(&password),
    );

    let result = cred.unwrap();
    println!("{:?}", result.credtype());

    return Ok(result);
}

The fetch:

pub fn update_local() {
    let repo = Repository::open("..").unwrap();
    let mut remote = repo.find_remote("origin").unwrap();

    let mut callbacks = RemoteCallbacks::new();
    callbacks.credentials(&git_credentials_callback);

    let mut options = FetchOptions::new();
    options.remote_callbacks(callbacks);
    options.prune(FetchPrune::On);

    remote.fetch(
        &[] as &[&str],
        Some(&mut options),
        Some("updating local")
    ).unwrap();
}

The get_key_path of mine:

fn get_key_path(extension: Option<&str>) -> PathBuf {
    let mut path = home_dir().unwrap();
    path.push(".ssh");
    path.push("cargo");

    if let Some(extension_value) = extension {
        path.set_extension(extension_value);
    }

    return path;
}

The entire code: https://github.com/darakeon/dfm/blob/main/version/src/git.rs

Expected behavior

Pull succeeds.

Actual behavior

Call the credentials function a bunch of times and gives the error:

 { code: -16, klass: 23, message: "Failed to retrieve list of SSH authentication methods: Failed getting response" }

Version of git2

0.15.0

Operating system(s) tested

Windows 10 (yeah, I now, almost every dev hates it). Trying to pull from github.

More details

Github updated their way to handle SSH in Sep 2021, and they recommend to use version 1.9.0 of libssh. Seems like the library here is using 0.2.23.

Github article: https://github.blog/2021-09-01-improving-git-protocol-security-github/#libgit2-and-other-git-clients

ehuss commented 2 years ago

I think there might be some confusion here. The version in Cargo.toml is not the version of the libssh2 C library. That is the version of the Cargo package (they are two unrelated things). I believe the latest libssh2-sys version 0.2.23 is using libssh2 1.10.0.

ehuss commented 2 years ago

FWIW, I tried your example code, and I was able to fetch from GitHub using SSH. I'm not sure what to suggest other than doing some double checks that you have the keys configured correctly, that the password is correct, etc.

WSSDude commented 2 years ago

From what I found out (as I too stumbled upon this issue), libssh2 uses WinCNG by default for Windows builds. Current implementation does not support new format GitHub requires.

Offending function is this one here (it can only load in PEM files which are unsupported by GitHub, so pretty useless...) https://github.com/libssh2/libssh2/blob/6c59eea5a9ea77127ec0fa3d6815c8adc743dba3/src/wincng.c#L691

It looks like you can force libssh2-sys crate to use OpenSSL on Windows via openssl-on-win32 feature, which works as a workaround currently for this issue, although you will need to convert your keys from OpenSSH to OpenSSL format for them to properly load.

WSSDude commented 2 years ago

Added this to my Cargo.toml dependencies alongside git2 and all seems to work as expected now. ssh2 = { version = "0.9.3", features = ["vendored-openssl", "openssl-on-win32"] } # using "vendored-openssl as I do not have local install of OpenSSL"

On a side note, I was not even required to convert OpenSSH key to OpenSSL variant as some other articles/guides/etc. mentioned would be the case (it probably is required for other key types than this one)

WSSDude commented 2 years ago

I have a feel that this option should be enabled by default for Windows builds honestly, as this is majorly breaking functionality otherwise at this moment (at least for this sort of usecase).

darakeon commented 2 years ago

@ehuss , I like triple checked or more. Created uncountable keys. Do not spread this info, but I use windows, did you tested this at windows?

Can be something of SSH that is inside my OS? I noticed the version number is not the same, even opened another issue in the rust lib that has the C thing inside

darakeon commented 2 years ago

https://github.com/alexcrichton/ssh2-rs/pull/259

darakeon commented 2 years ago

@WSSDude420 , received this:

Error { kind: NotFound, message: "program not found" }

Just got weirder...

WSSDude commented 2 years ago

That PR is just pure wrong - why you donwgrade it? It is already latest version...

Did you read my comments? I wrote what is the exact issue and also proposed a working solution. Yours is not solving anything.

WSSDude commented 2 years ago

I would bet it is because of the downgrade you did (provided you write output from that)

darakeon commented 2 years ago

Which one, @WSSDude420 ?

darakeon commented 2 years ago

(and the two PRs I opened was before you answered)

darakeon commented 2 years ago

And... They have no effect over my code, given they were not approved and are not in the main code, I'm still using the official libraries

WSSDude commented 2 years ago

True, but you could have read and tried at least what I wrote before continuing proposing downgrade basically. Dont know if you realised it, it could have been a mistake (downgrade), just wanted to mention it cause I dont see any way how that would solve anything.

WSSDude commented 2 years ago

Both of the PRs are wrong in different way... ssh2-rs one downgrades libssh2-sys lib version it uses, git2-rs has just wrong version of the package in both cases (you bumped it to 0.2.24 in that PR)

darakeon commented 2 years ago

I'm not proposing downgrade. The PRs was before your comment. I read what you said and tried it. The answer above was me trying what you said in a library that does not have any changes that I proposed on PRs

darakeon commented 2 years ago

git2-rs I noticed, first answer I gave to @ehuss was about I already noticed that. The PR is draft because the intention was fix it if the PR in the other repo was approved.

WSSDude commented 2 years ago

Well, then I probably misunderstood for which Im sorry. In any case, I dont know honestly what happened in your case.

I include them vendored like this in project Cargo.toml, did not test without. git2 = { version = "0.15.0", features = ["vendored-libgit2", "vendored-openssl"] }

And I use the patch standard way like this in my Cargo.toml (needs to be in workspace root)

[patch.crates-io]
git2 = { path = "thirdparty/git2-rs" }

I have git2-rs checked out in the <workspace_root>/thirdparty, using my branch (via .gitmodules)

darakeon commented 2 years ago

The other repo, there is no downgrade too: https://github.com/alexcrichton/ssh2-rs/pull/259/files

This updates the library version from 0.23 to 0.24 because if the PR is approved it needs to change number, and the submodule upgrades to a version with 109 more commits

darakeon commented 2 years ago

I'm not using this from a path, I'm using directly from cargo

WSSDude commented 2 years ago

That repo as I wrote in the comment there downgrades libssh2 submodule, just click on them here on Git from your fork and from original one, look at last commit date,.

darakeon commented 2 years ago

My cargo is like this:

[dependencies]
dirs = "4.0.0"
git2 = "0.15.0"
regex = "1.5.4"
reqwest = { version = "0.11.9", features = ["blocking"] }
json = "0.12.4"
darakeon commented 2 years ago

I'm a bit lost, just followed the repos at crates.io to know where to propose changes

WSSDude commented 2 years ago

I'm not using this from a path, I'm using directly from cargo

Well that won't work I guess as there is problem in how libgit2 includes libssh2 as mentioned (for Windows builds, due to broken default backend)

git2 = { version = "0.15.0",  features = ["vendored-libgit2", "vendored-openssl"] }
ssh2 = { version = "0.9.3", features = ["vendored-openssl", "openssl-on-win32"] }

This looks like a working workaround if you do not use the patch. It could cause issues I believe as the dependencies dont match wholy.

More proper way should be to use patched git2. You can clone my fork, checkout the fix branch, and tell cargo that you want to patch crate to point to path you cloned it in. (I have put it into thirdparty/git2-rs) Rest stays the same, you still include it like above (just without ssh2 line)

[patch.crates-io]
git2 = { path = "thirdparty/git2-rs" }

If you use workspaces, you have to have that in the root, where you define workspace members.

WSSDude commented 2 years ago

Hope it is a bit clearer, did few edits...

WSSDude commented 2 years ago

I basically followed guide here for reference https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

darakeon commented 2 years ago

Thanks a lot, @WSSDude420 ! If it was for a work, I would just change my project to use bash scripts for git path, seems a lot of work to make it work inside rust. But this is a sandbox project - it has a website and I'm even my own client, but one of the main reasons of the system is to learn more and more. And I thank you very much for the help, seems like an opportunity to learn stuff I don't know about Rust (I will have to research discover what is a workspace, for example).

Thanks a lot for the help.

ehuss commented 1 year ago

I'm going to close as libssh was updated in https://github.com/rust-lang/git2-rs/pull/919.