Skip to content
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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bnjmnt4n
Copy link
Collaborator

@bnjmnt4n bnjmnt4n commented Mar 2, 2024

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 the libssh2 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 the ssh 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 libgit2 (update: landed), and rust-lang/git2-rs#1031 which builds 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:

git clone git@github.com:bnjmnt4n/jj && cd jj
git checkout ssh-openssh
cargo install --path cli --locked --bin jj

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@bnjmnt4n
Copy link
Collaborator Author

bnjmnt4n commented Mar 3, 2024

Oddly enough, building this as a Nix derivation no longer works correctly with my IdentityAgent SSH config...

@khionu
Copy link
Collaborator

khionu commented Mar 22, 2024

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.

@bnjmnt4n bnjmnt4n force-pushed the ssh-openssh branch 3 times, most recently from ebfee1a to 3543cbd Compare March 29, 2024 09:12
@khionu
Copy link
Collaborator

khionu commented Mar 30, 2024

Could you link said blocking PRs?

@bnjmnt4n
Copy link
Collaborator Author

@khionu updated!

@joyously
Copy link

joyously commented Apr 4, 2024

I'm wondering if other SSH implementations are susceptible also.
https://www.wired.com/story/xz-backdoor-everything-you-need-to-know/

Malicious code added to xz Utils versions 5.6.0 and 5.6.1 modified the way the software functions. The backdoor manipulated sshd, the executable file used to make remote SSH connections. Anyone in possession of a predetermined encryption key could stash any code of their choice in an SSH login certificate, upload it, and execute it on the backdoored device.

Wait, How Can a Compression Utility Manipulate a Process as Security-Sensitive as SSH?

Any library can tamper with the inner workings of any executable it is linked against.
OpenSSH, the most popular sshd implementation, doesn’t link the liblzma library, but Debian and many other Linux distributions add a patch to link sshd to systemd, a program that loads a variety of services during the system bootup. Systemd, in turn, links to liblzma, and this allows XZ Utils to exert control over sshd.

@khionu
Copy link
Collaborator

khionu commented Apr 4, 2024

I'm wondering if other SSH implementations are susceptible also.

Not relevant to us, the exploit targeted the server-side daemon

@HybridEidolon
Copy link
Contributor

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.

@emilazy
Copy link
Collaborator

emilazy commented Jun 13, 2024

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!)

@bnjmnt4n bnjmnt4n force-pushed the ssh-openssh branch 2 times, most recently from 223c3c0 to ca22436 Compare July 3, 2024 16:55
@dbarnett
Copy link
Collaborator

Also closes #401.

Looks like one of the two upstream PRs has landed and we're just waiting on rust-lang/git2-rs#1031 to finalize this? Or are there other blockers?

@tim-janik
Copy link
Collaborator

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.

One scenario I just ran into with JJ is when connections are only possible when ~/.ssh/config must be read to make a connection. With local login sessions, ssh-agent takes care of reading ~/.ssh/config and providing keys, but that is not the case in remote ssh sessions, which have env variables that point to a remote ssh-agent. For example:

  • Machine B has a repo, local logins will use the local ssh-agent which reads ~/.ssh/config and thus has the key, so JJ commands from local login sessions work correctly.
  • Machine A has its own ssh-agent, when doing ssh B, the ssh session has SSH_* variables that point to the ssh-agent from machine A.
  • So using libssh2 on B in an ssh session originating at machine A will never read B:~/.ssh/config and thus not find the needed key.
  • To inspect the SSH setup, try env | fgrep SSH in a local session and then in an ssh session.

Examples for scenarios where ~/.ssh/config must be read in order to establish a connection are:

  • Network topology forces connections through a proxy, like using ProxyCommand for a corporate VPN
  • You have several ssh keys. When ssh connects it tries a default set in some order and then gives up, unless .ssh/config associates a host with a specific key via IdentityFile and has IdentitiesOnly=yes in order to avoid Too many authentication failures

dln added a commit to dln/jj that referenced this pull request Sep 11, 2024
@ETCaton ETCaton mentioned this pull request Oct 5, 2024
4 tasks
@bnjmnt4n bnjmnt4n force-pushed the ssh-openssh branch 3 times, most recently from a847da8 to 1d7aaa6 Compare October 16, 2024 10:32
lilyball added a commit to lilyball/jj that referenced this pull request Oct 19, 2024
This merges PR martinvonz#3191 (OpenSSH) and PR martinvonz#2845 (Gerrit) in order to build
from the combination.
lilyball added a commit to lilyball/jj that referenced this pull request Oct 19, 2024
This merges PR martinvonz#3191 (OpenSSH) and PR martinvonz#2845 (Gerrit) in order to build
from the combination.
kivikakk pushed a commit to kivikakk/vyxos that referenced this pull request Oct 21, 2024
@charlottia
Copy link

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!

@codeslinger
Copy link

codeslinger commented Oct 25, 2024

I've been using this branch ever since I started using jj a month ago and its had no problems. Thanks!

jwoudenberg added a commit to jwoudenberg/nix that referenced this pull request Oct 26, 2024
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:
martinvonz/jj#3191
@ErichDonGubler
Copy link
Collaborator

Also wanted to dog-pile the praise here: This PR is what has allowed me to use JJ's jj git {fetch,push} on Windows in the same way as other platforms. It's let me enjoy JJ to make my life much simpler. Thank you, @bnjmnt4n!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

git: SSH config not respected
10 participants