From a445b72fcdbb52e2f2ef0b240e41152fa92292f9 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 26 Jan 2025 09:12:40 -0500 Subject: [PATCH 1/4] fix: Honor `disallow_shell` in SSH client feature check When running an SSH client, the `disallow_shell` option determines whether the client command, before arguments, is to be run directly or if it is to be run by a shell. (One example of when it is run directly is if it comes from the `GIT_SSH` environment variable, while one example of when it is run by a shell is if it comes from the `GIT_SSH_COMMAND` environment variable.) When invoking the client in the most central and common case of actually attempting to connect to a remote server, `disallow_shell` was already followed. However, in some cases we are not sure what kind of SSH client program we have, and so to find that out (so we know how to run it to connect to a server), we run a test command, to see if it recognizes `-G` as OpenSSH clients do. Often we can tell what kind of client program we have without needing to do that. But if we do need to do it, we pre-run the client to check. In this use, the `disallow_shell` option was not followed, and instead the use of a shell was unconditionally treated as allowed. This fixes that by setting `prepare.use_shell = false` on a constructed `gix_command::Prepare` instance, which seems to be the prevailing style for achieving this elsewhere in `gix-transport`. --- gix-transport/src/client/blocking_io/ssh/mod.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 21680031712..2b5967dcc48 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -111,8 +111,8 @@ pub fn connect( let ssh_cmd = options.ssh_command(); let mut kind = options.kind.unwrap_or_else(|| ProgramKind::from(ssh_cmd)); if options.kind.is_none() && kind == ProgramKind::Simple { - let mut cmd = std::process::Command::from( - gix_command::prepare(ssh_cmd) + let mut cmd = std::process::Command::from({ + let mut prepare = gix_command::prepare(ssh_cmd) .stderr(Stdio::null()) .stdout(Stdio::null()) .stdin(Stdio::null()) @@ -122,8 +122,12 @@ pub fn connect( Usable(host) => host, Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?, Absent => panic!("BUG: host should always be present in SSH URLs"), - }), - ); + }); + if options.disallow_shell { + prepare.use_shell = false; + } + prepare + }); gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check"); kind = if cmd.status().ok().is_some_and(|status| status.success()) { ProgramKind::Ssh From 9255ccc8e043513329f3e78d9b0a2616e807121e Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 26 Jan 2025 09:24:36 -0500 Subject: [PATCH 2/4] Factor client feature check from `connect` to helpers This creates two helper functions and moves the logic for the SSH client feature check (determining the `ProgramKind`) to them. Further refactoring is possible, especially since some of the use of mutability might be replaceable by early return with no loss of readability. But for now this maintains the preexisting structure of the code covering the case where it is not necessary to pre-run the SSH client. --- .../src/client/blocking_io/ssh/mod.rs | 83 +++++++++++-------- 1 file changed, 50 insertions(+), 33 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 2b5967dcc48..83849b1fca7 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -1,8 +1,14 @@ -use std::process::Stdio; +use std::{ + ffi::OsStr, + process::{Command, Stdio}, +}; -use gix_url::ArgumentSafety::*; +use gix_url::{ArgumentSafety::*, Url}; -use crate::{client::blocking_io, Protocol}; +use crate::{ + client::{self, blocking_io::file::SpawnProcessOnDemand}, + Protocol, +}; /// The error used in [`connect()`]. #[derive(Debug, thiserror::Error)] @@ -100,44 +106,18 @@ pub mod connect { /// If `trace` is `true`, all packetlines received or sent will be passed to the facilities of the `gix-trace` crate. #[allow(clippy::result_large_err)] pub fn connect( - url: gix_url::Url, + url: Url, desired_version: Protocol, options: connect::Options, trace: bool, -) -> Result { +) -> Result { if url.scheme != gix_url::Scheme::Ssh || url.host().is_none() { return Err(Error::UnsupportedScheme(url)); } let ssh_cmd = options.ssh_command(); - let mut kind = options.kind.unwrap_or_else(|| ProgramKind::from(ssh_cmd)); - if options.kind.is_none() && kind == ProgramKind::Simple { - let mut cmd = std::process::Command::from({ - let mut prepare = gix_command::prepare(ssh_cmd) - .stderr(Stdio::null()) - .stdout(Stdio::null()) - .stdin(Stdio::null()) - .command_may_be_shell_script() - .arg("-G") - .arg(match url.host_as_argument() { - Usable(host) => host, - Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?, - Absent => panic!("BUG: host should always be present in SSH URLs"), - }); - if options.disallow_shell { - prepare.use_shell = false; - } - prepare - }); - gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check"); - kind = if cmd.status().ok().is_some_and(|status| status.success()) { - ProgramKind::Ssh - } else { - ProgramKind::Simple - }; - } - + let kind = determine_client_kind(options.kind, ssh_cmd, &url, options.disallow_shell)?; let path = gix_url::expand_path::for_shell(url.path.clone()); - Ok(blocking_io::file::SpawnProcessOnDemand::new_ssh( + Ok(SpawnProcessOnDemand::new_ssh( url, ssh_cmd, path, @@ -148,5 +128,42 @@ pub fn connect( )) } +fn determine_client_kind( + known_kind: Option, + ssh_cmd: &OsStr, + url: &Url, + disallow_shell: bool, +) -> Result { + let mut kind = known_kind.unwrap_or_else(|| ProgramKind::from(ssh_cmd)); + if known_kind.is_none() && kind == ProgramKind::Simple { + let mut cmd = build_client_feature_check_command(ssh_cmd, url, disallow_shell)?; + gix_features::trace::debug!(cmd = ?cmd, "invoking `ssh` for feature check"); + kind = if cmd.status().ok().is_some_and(|status| status.success()) { + ProgramKind::Ssh + } else { + ProgramKind::Simple + }; + } + Ok(kind) +} + +fn build_client_feature_check_command(ssh_cmd: &OsStr, url: &Url, disallow_shell: bool) -> Result { + let mut prepare = gix_command::prepare(ssh_cmd) + .stderr(Stdio::null()) + .stdout(Stdio::null()) + .stdin(Stdio::null()) + .command_may_be_shell_script() + .arg("-G") + .arg(match url.host_as_argument() { + Usable(host) => host, + Dangerous(host) => Err(Error::AmbiguousHostName { host: host.into() })?, + Absent => panic!("BUG: host should always be present in SSH URLs"), + }); + if disallow_shell { + prepare.use_shell = false; + } + Ok(prepare.into()) +} + #[cfg(test)] mod tests; From 1dd023063227f5ee4f7876ec262d9cebe308a6df Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 26 Jan 2025 10:36:04 -0500 Subject: [PATCH 3/4] Allow `result_large_err` on `connect`'s helpers This is with the idea that they're likely to be able to be inlined, since they each have exactly one call site. --- gix-transport/src/client/blocking_io/ssh/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 83849b1fca7..8ec4ffe86a8 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -128,6 +128,7 @@ pub fn connect( )) } +#[allow(clippy::result_large_err)] fn determine_client_kind( known_kind: Option, ssh_cmd: &OsStr, @@ -147,6 +148,7 @@ fn determine_client_kind( Ok(kind) } +#[allow(clippy::result_large_err)] fn build_client_feature_check_command(ssh_cmd: &OsStr, url: &Url, disallow_shell: bool) -> Result { let mut prepare = gix_command::prepare(ssh_cmd) .stderr(Stdio::null()) From c0cbc19f90022c79f137e2631f8b88982827d1ec Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Sun, 26 Jan 2025 10:44:08 -0500 Subject: [PATCH 4/4] Thanks clippy --- gix-transport/src/client/blocking_io/ssh/mod.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/gix-transport/src/client/blocking_io/ssh/mod.rs b/gix-transport/src/client/blocking_io/ssh/mod.rs index 8ec4ffe86a8..5e2206155e8 100644 --- a/gix-transport/src/client/blocking_io/ssh/mod.rs +++ b/gix-transport/src/client/blocking_io/ssh/mod.rs @@ -5,10 +5,7 @@ use std::{ use gix_url::{ArgumentSafety::*, Url}; -use crate::{ - client::{self, blocking_io::file::SpawnProcessOnDemand}, - Protocol, -}; +use crate::{client::blocking_io::file::SpawnProcessOnDemand, Protocol}; /// The error used in [`connect()`]. #[derive(Debug, thiserror::Error)]