-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SSH: Use OpenSSH instead of libssh2
for authentication with Git hosts
#3191
base: main
Are you sure you want to change the base?
Conversation
|
2e422ea
to
4c08a85
Compare
Could you include a list of reasons for this switch in the PR description? Would be good for historical purposes, especially for elsewhere in Rust where people may try to make the case for switching. |
ebfee1a
to
3543cbd
Compare
Could you link said blocking PRs? |
@khionu updated! |
I'm wondering if other SSH implementations are susceptible also.
|
Not relevant to us, the exploit targeted the server-side daemon |
3fbe3a8
to
6152514
Compare
I anticipate that changing to OpenSSH will probably fix the same issue on Windows that #3554 is attempting to address. Hard to imagine it being anything other than strictly better. |
3a2a16a
to
bb55edf
Compare
The following patch fixes the Nix build for me: diff --git a/Cargo.lock b/Cargo.lock
index 4b1d4c56f8...9a1f618ca9 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1846,7 +1846,6 @@
dependencies = [
"cc",
"libc",
- "libssh2-sys",
"libz-sys",
"openssl-sys",
"pkg-config",
@@ -1864,20 +1863,6 @@
]
[[package]]
-name = "libssh2-sys"
-version = "0.3.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2dc8a030b787e2119a731f1951d6a773e2280c660f8ec4b0f5e1505a386e71ee"
-dependencies = [
- "cc",
- "libc",
- "libz-sys",
- "openssl-sys",
- "pkg-config",
- "vcpkg",
-]
-
-[[package]]
name = "libz-sys"
version = "1.1.16"
source = "registry+https://github.com/rust-lang/crates.io-index"
diff --git a/flake.nix b/flake.nix
index 4fba5310ff...c06feae22c 100644
--- a/flake.nix
+++ b/flake.nix
@@ -84,7 +84,7 @@
cargoLock.lockFile = ./Cargo.lock;
cargoLock.outputHashes = {
- "git2-0.18.3" = "sha256-Kfg3xWIAarAxeIo2wL30OFni7X4Thf9EzaXbFTWsehE=";
+ "git2-0.18.3" = "sha256-3g7ajPfLfuPWh46rIa70wQRWLZ+jZXBApkyPlJULi/I=";
};
nativeBuildInputs = with pkgs; [
gzip (An ironically retro way to share the change, I realize!) |
223c3c0
to
ca22436
Compare
a847da8
to
1d7aaa6
Compare
This merges PR jj-vcs#3191 (OpenSSH) and PR jj-vcs#2845 (Gerrit) in order to build from the combination.
This merges PR jj-vcs#3191 (OpenSSH) and PR jj-vcs#2845 (Gerrit) in order to build from the combination.
Just noting that I've been daily-driving this PR for a few days (rebased on main) and it's been working really beautifully for me! Thank you! |
I've been using this branch ever since I started using |
I can't get the mainline version to work with my yubikey-based ssh setup. This branch uses openssh, which should be fine. Here's a PR against the main repo: jj-vcs/jj#3191
Also wanted to dog-pile the praise here: This PR is what has allowed me to use JJ's |
77c23f6
to
10eda2c
Compare
10eda2c
to
3fa1ed1
Compare
3fa1ed1
to
3f73ba0
Compare
I've likewise been using this PR (rebased on main) and it's been very helpful! The only issue I've run into is that it doesn't seem to respect the "port" value in |
I would love to take this PR for a spin as well. I've just started using JJ and cannot get it to fetch/push etc because of libgit2 issues (my identities are tied to hosts through ssh config file but libgit can't even see my windows ssh environment initialized). I've tried building this PR from source, but unfortunately cmake isn't happy with my msbuild VC tools. Is there any way to get the artefacts from somewhere that the CI is already building up? PS: thank you very much for this PR work. Looking forward to it landing upstream. From my limited use of JJ, I can't wait to make it my daily driver! Thank you team <3 |
I'm afraid not. I think you'll have to fetch from Benjamin's repo and build from source. |
@Maverik what kind of trouble? I found this was enough on Windows for me:
(but I'm not sure what the difference in the rest of our non-Rust environments are like) |
I was having issue with I was able to compile both the branch directly and through above after repairing the install. Thank you for the swift help. |
The current system does actually build binaries from In theory we could extend this to the PRs pretty easily. I'd say I'm worried about users downloading random binaries, but in this case they're already compiling them anyway... So it's probably not that bad, I guess. |
It would also be wasteful to build release binaries that don't get used in 99.9% of cases. |
698789a
to
70e796a
Compare
This PR switches to OpenSSH instead of
libssh2
for authentication with Git hosts. It allows configuration specified in~/.ssh/config
to be read when connecting via SSH, e.g. specifying custom keys to use for different hosts, or using a different SSH agent. In general, the OpenSSH-based connection seems slightly more robust then thelibssh2
version, but I think there are still some issues which do not work well (eg., it didn't work for me when I hadn't connected to the host before, and didn't display the unknown host interaction that thessh
binary does).I still need to look into whether this affects other workflows (eg. git credentials).
Marking this as a draft since this also depends on upstream PRs to land (
rust-lang/git2-rs#1032 which bumps the version of(update: landed), and rust-lang/git2-rs#1031 which buildslibgit2
libgit2
using OpenSSH for SSH execution), but I'm creating the PR for improved visibility.Closes #63.
If you're interested in using this, the following code can be used to build this branch locally:
Checklist
If applicable:
CHANGELOG.md