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

Add git.ssh-key-file setting #5458

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dconeybe
Copy link

This PR addresses a specific problem resulting from ~/.ssh/config being ignored by jj (#63). Namely, without this PR, jj hardcodes 3 possible ssh key files that it considers: ~/.ssh/id_ed25519_sk, ~/.ssh/id_ed25519, and ~/.ssh/id_rsa:

for filename in ["id_ed25519_sk", "id_ed25519", "id_rsa"] {

Personally, I also have a file called ~/.ssh/id_ed25519_personal that I use for personal projects, but there is no way to get jj to use it. As a result, my jj git push commands always fail with this error:

$ jj git push
Changes to push to origin:
  Add bookmark tmp to d2f817604110
Error: failed to authenticate SSH session: Unable to extract public key from private key file: Wrong passphrase or invalid/unrecognized private key file format; class=Ssh (23)
Hint: Jujutsu uses libssh2, which doesn't respect ~/.ssh/config. Does `ssh -F /dev/null` to the host work?

With the patch in this PR, I can run this command to tell jj to use the correct ssh key when contacting the git server:

jj config set --repo git.ssh-key-file $HOME/.ssh/id_ed25519_personal

It may also make sense to add this configuration setting per remote, or possibly a different one for pushing and fetcing.

Disclaimer: I am a novice rust programmer and this is my first contribution to jj; if this idea is accepted I will also add proper unit testing and changelog entries.

@dconeybe
Copy link
Author

I will be OOO for a week, so I will respond to any feedback when I get back on Feb 03, 2025.

@bryceberger
Copy link
Member

The build failures in CI are due to rust-lang/rust#121346. CI is on 1.76, presumably locally you are running 1.79+.

lib/src/settings.rs Outdated Show resolved Hide resolved
@dconeybe
Copy link
Author

The build failures in CI are due to rust-lang/rust#121346. CI is on 1.76, presumably locally you are running 1.79+.

Yes I downgraded to 1.76 and I can repro the failures locally. I'm seeing what I can do for a fix.

@bryceberger
Copy link
Member

bryceberger commented Jan 24, 2025

Think fixing it on 1.76 would require moving the condition into the closure, ex:

diff --git a/cli/src/git_util.rs b/cli/src/git_util.rs
index f3a84c5099..60242ff0c0 100644
--- a/cli/src/git_util.rs
+++ b/cli/src/git_util.rs
@@ -311,12 +311,13 @@
             .map(|x| x as &mut dyn FnMut(&git::Progress));
         callbacks.sideband_progress =
             sideband_progress_callback.map(|x| x as &mut dyn FnMut(&[u8]));
-        let mut get_ssh_keys: &mut dyn FnMut(&str) -> Vec<PathBuf> =
-            match &git_settings.ssh_key_file {
-                None => &mut get_ssh_keys, // Coerce to unit fn type
-                Some(ssh_key_file) => &mut |_: &str| -> Vec<PathBuf> { vec![ssh_key_file.clone()] },
-            };
-        callbacks.get_ssh_keys = Some(&mut get_ssh_keys);
+        let get_ssh_keys: &mut dyn FnMut(&_) -> _ = &mut |username| {
+            git_settings
+                .ssh_key_file
+                .as_ref()
+                .map_or_else(|| get_ssh_keys(username), |key_file| vec![key_file.clone()])
+        };
+        callbacks.get_ssh_keys = Some(get_ssh_keys);
         let mut get_pw =
             |url: &str, _username: &str| pinentry_get_pw(url).or_else(|| terminal_get_pw(ui, url));
         callbacks.get_password = Some(&mut get_pw);

That said, we might be bumping the MSRV soon anyway (discord discussion).

@martinvonz
Copy link
Member

I'm not against this PR, but you probably don't need it if you build after #5228 and set git.subprocess = true

file, overriding the hardcoded defaults of ~/.ssh/id_ed25519_sk,
~/.ssh/id_ed25519, and ~/.ssh/id_rsa.
@dconeybe
Copy link
Author

Think fixing it on 1.76 would require moving the condition into the closure, ex:

I've applied your patch. The code looks pretty good either way IMO, so may as well get it to build :)

@dconeybe
Copy link
Author

I'm not against this PR, but you probably don't need it if you build after #5228 and set git.subprocess = true

I'll keep my eye out for that!

@@ -70,6 +71,17 @@ impl GitSettings {
abandon_unreachable_commits: settings.get_bool("git.abandon-unreachable-commits")?,
subprocess: settings.get_bool("git.subprocess")?,
executable_path: settings.get("git.executable-path")?,
ssh_key_file: match settings.get_string("git.ssh-key-file") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add config knob, I would expect it would be a list of paths which defaults to ["~/.ssh/id_..", ..].

And it's probably better to move config handling to CLI. jj_lib::expand_home_path() is half-baked. Ideally, library code shouldn't depend on environment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duly noted. If nobody gets to it I'll take a look on the week of Feb 03 when I return from vacation.

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.

4 participants