From 0efb5aee4c3a513012554b2fee048373101313a6 Mon Sep 17 00:00:00 2001 From: Chip Senkbeil Date: Thu, 13 Jul 2023 01:55:57 -0500 Subject: [PATCH] Add --shell support to CLI (#218) --- CHANGELOG.md | 19 ++ Cargo.lock | 8 + Cargo.toml | 1 + .../src/api/state/process/instance.rs | 1 + distant-protocol/Cargo.toml | 1 + distant-protocol/src/common/cmd.rs | 16 -- distant-protocol/src/lib.rs | 12 +- distant-ssh2/src/lib.rs | 49 +++-- src/cli/commands/client.rs | 51 ++++- src/options.rs | 9 + src/options/common.rs | 2 + src/options/common/shell.rs | 196 ++++++++++++++++++ 12 files changed, 319 insertions(+), 46 deletions(-) create mode 100644 src/options/common/shell.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index b5d3757..0125fcd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,29 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Support for `--shell` with optional path to an explicit shell as an option + when executing `distant spawn` in order to run the command within a shell + rather than directly + +### Changed + +- `distant_protocol::PROTOCOL_VERSION` now uses the crate's major, minor, and + patch version at compile-time (parsed via `const-str` crate) to streamline + version handling between crate and protocol + ### Fixed - CLI commands like `distant manager select` will now output errors in a JSON format when configured to communicate using JSON +- `distant-ssh2` no longer caches the remote family globally, but instead + caches it per `Ssh` instance + +### Removed + +- `Cmd::program` and `Cmd::arguments` functions as they were misleading (didn't + do what `distant-local` or `distant-ssh2` do) ## [0.20.0-alpha.12] diff --git a/Cargo.lock b/Cargo.lock index 5a96fd5..3fd371b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -571,6 +571,12 @@ version = "0.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "520fbf3c07483f94e3e3ca9d0cfd913d7718ef2483d2cfd91c0d9e91474ab913" +[[package]] +name = "const-str" +version = "0.5.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aca749d3d3f5b87a0d6100509879f9cf486ab510803a4a4e1001da1ff61c2bd6" + [[package]] name = "convert_case" version = "0.4.0" @@ -844,6 +850,7 @@ dependencies = [ "test-log", "tokio", "toml_edit", + "typed-path", "which", "whoami", "windows-service", @@ -952,6 +959,7 @@ name = "distant-protocol" version = "0.20.0-alpha.12" dependencies = [ "bitflags 2.3.1", + "const-str", "derive_more", "regex", "rmp", diff --git a/Cargo.toml b/Cargo.toml index 2fb3bf8..6a2943a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -59,6 +59,7 @@ tokio = { version = "1.28.2", features = ["full"] } toml_edit = { version = "0.19.10", features = ["serde"] } terminal_size = "0.2.6" termwiz = "0.20.0" +typed-path = "0.3.2" which = "4.4.0" winsplit = "0.1.0" whoami = "1.4.0" diff --git a/distant-local/src/api/state/process/instance.rs b/distant-local/src/api/state/process/instance.rs index 8049c5c..abc2a1d 100644 --- a/distant-local/src/api/state/process/instance.rs +++ b/distant-local/src/api/state/process/instance.rs @@ -85,6 +85,7 @@ impl ProcessInstance { let args = cmd_and_args.split_off(1); let cmd = cmd_and_args.into_iter().next().unwrap(); + debug!("Spawning process: {cmd} {args:?}"); let mut child: Box = match pty { Some(size) => Box::new(PtyProcess::spawn( cmd.clone(), diff --git a/distant-protocol/Cargo.toml b/distant-protocol/Cargo.toml index 604fc6f..e10d357 100644 --- a/distant-protocol/Cargo.toml +++ b/distant-protocol/Cargo.toml @@ -17,6 +17,7 @@ tests = [] [dependencies] bitflags = "2.3.1" +const-str = "0.5.6" derive_more = { version = "0.99.17", default-features = false, features = ["deref", "deref_mut", "display", "from", "error", "into", "into_iterator", "is_variant"] } regex = "1.8.3" serde = { version = "1.0.163", features = ["derive"] } diff --git a/distant-protocol/src/common/cmd.rs b/distant-protocol/src/common/cmd.rs index 786531b..073f7d0 100644 --- a/distant-protocol/src/common/cmd.rs +++ b/distant-protocol/src/common/cmd.rs @@ -12,22 +12,6 @@ impl Cmd { pub fn new(cmd: impl Into) -> Self { Self(cmd.into()) } - - /// Returns reference to the program portion of the command - pub fn program(&self) -> &str { - match self.0.split_once(' ') { - Some((program, _)) => program.trim(), - None => self.0.trim(), - } - } - - /// Returns reference to the arguments portion of the command - pub fn arguments(&self) -> &str { - match self.0.split_once(' ') { - Some((_, arguments)) => arguments.trim(), - None => "", - } - } } impl Deref for Cmd { diff --git a/distant-protocol/src/lib.rs b/distant-protocol/src/lib.rs index 1da41ad..8f0907c 100644 --- a/distant-protocol/src/lib.rs +++ b/distant-protocol/src/lib.rs @@ -17,7 +17,11 @@ pub use response::*; /// Protocol version indicated by the tuple of (major, minor, patch). /// -/// This is different from the crate version, which matches that of the complete suite of distant -/// crates. Rather, this verison is used to provide stability indicators when the protocol itself -/// changes across crate versions. -pub const PROTOCOL_VERSION: SemVer = (0, 1, 0); +/// This should match the version of this crate such that any significant change to the crate +/// version will also be reflected in this constant that can be used to verify compatibility across +/// the wire. +pub const PROTOCOL_VERSION: SemVer = ( + const_str::parse!(env!("CARGO_PKG_VERSION_MAJOR"), u8), + const_str::parse!(env!("CARGO_PKG_VERSION_MINOR"), u8), + const_str::parse!(env!("CARGO_PKG_VERSION_PATCH"), u8), +); diff --git a/distant-ssh2/src/lib.rs b/distant-ssh2/src/lib.rs index f8cbd7d..aefe3be 100644 --- a/distant-ssh2/src/lib.rs +++ b/distant-ssh2/src/lib.rs @@ -16,7 +16,6 @@ use std::str::FromStr; use std::time::Duration; use async_compat::CompatExt; -use async_once_cell::OnceCell; use async_trait::async_trait; use distant_core::net::auth::{AuthHandlerMap, DummyAuthHandler, Verifier}; use distant_core::net::client::{Client, ClientConfig}; @@ -25,6 +24,7 @@ use distant_core::net::server::{Server, ServerRef}; use distant_core::{DistantApiServerHandler, DistantClient, DistantSingleKeyCredentials}; use log::*; use smol::channel::Receiver as SmolReceiver; +use tokio::sync::Mutex; use wezterm_ssh::{ ChildKiller, Config as WezConfig, MasterPty, PtySize, Session as WezSession, SessionEvent as WezSessionEvent, @@ -325,17 +325,20 @@ impl SshAuthHandler for LocalSshAuthHandler { } } -/// Represents an ssh2 client +/// Represents an ssh2 client. pub struct Ssh { session: WezSession, events: SmolReceiver, host: String, port: u16, authenticated: bool, + + /// Cached copy of the family representing the remote machine. + cached_family: Mutex>, } impl Ssh { - /// Connect to a remote TCP server using SSH + /// Connect to a remote TCP server using SSH. pub fn connect(host: impl AsRef, opts: SshOpts) -> io::Result { debug!( "Establishing ssh connection to {} using {:?}", @@ -416,15 +419,16 @@ impl Ssh { host: host.as_ref().to_string(), port, authenticated: false, + cached_family: Mutex::new(None), }) } - /// Host this client is connected to + /// Host this client is connected to. pub fn host(&self) -> &str { &self.host } - /// Port this client is connected to on remote host + /// Port this client is connected to on remote host. pub fn port(&self) -> u16 { self.port } @@ -434,7 +438,7 @@ impl Ssh { self.authenticated } - /// Authenticates the [`Ssh`] if not already authenticated + /// Authenticates the [`Ssh`] if not already authenticated. pub async fn authenticate(&mut self, handler: impl SshAuthHandler) -> io::Result<()> { // If already authenticated, exit if self.authenticated { @@ -499,10 +503,10 @@ impl Ssh { Ok(()) } - /// Detects the family of operating system on the remote machine + /// Detects the family of operating system on the remote machine. + /// + /// Caches the result such that subsequent checks will return the same family. pub async fn detect_family(&self) -> io::Result { - static INSTANCE: OnceCell = OnceCell::new(); - // Exit early if not authenticated as this is a requirement if !self.authenticated { return Err(io::Error::new( @@ -511,18 +515,23 @@ impl Ssh { )); } - INSTANCE - .get_or_try_init(async move { - let is_windows = utils::is_windows(&self.session).await?; + let mut family = self.cached_family.lock().await; - Ok(if is_windows { - SshFamily::Windows - } else { - SshFamily::Unix - }) - }) - .await - .copied() + // Family value is not present, so we retrieve it now and populate our cache + if family.is_none() { + // Check if we are windows, otherwise assume unix, returning an error if encountered, + // which will also drop our lock on the cache + let is_windows = utils::is_windows(&self.session).await?; + + *family = Some(if is_windows { + SshFamily::Windows + } else { + SshFamily::Unix + }); + } + + // Cache should always be Some(...) by this point + Ok(family.unwrap()) } /// Consume [`Ssh`] and produce a [`DistantClient`] that is connected to a remote diff --git a/src/cli/commands/client.rs b/src/cli/commands/client.rs index 3ef4c01..84981c8 100644 --- a/src/cli/commands/client.rs +++ b/src/cli/commands/client.rs @@ -25,7 +25,10 @@ use crate::cli::common::{ Cache, Client, JsonAuthHandler, MsgReceiver, MsgSender, PromptAuthHandler, }; use crate::constants::MAX_PIPE_CHUNK_SIZE; -use crate::options::{ClientFileSystemSubcommand, ClientSubcommand, Format, NetworkSettings}; +use crate::options::{ + ClientFileSystemSubcommand, ClientSubcommand, Format, NetworkSettings, ParseShellError, + Shell as ShellOption, +}; use crate::{CliError, CliResult}; mod lsp; @@ -369,6 +372,7 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { environment, lsp, pty, + shell, network, } => { debug!("Connecting to manager"); @@ -383,20 +387,55 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { use_or_lookup_connection_id(&mut cache, connection, &mut client).await?; debug!("Opening channel to connection {}", connection_id); - let channel = client + let mut channel: DistantChannel = client .open_raw_channel(connection_id) .await - .with_context(|| format!("Failed to open channel to connection {connection_id}"))?; + .with_context(|| format!("Failed to open channel to connection {connection_id}"))? + .into_client() + .into_channel(); // Convert cmd into string let cmd = cmd_str.unwrap_or_else(|| cmd.join(" ")); + // Check if we should attempt to run the command in a shell + let cmd = match shell { + None => cmd, + + // Use default shell, which we need to figure out + Some(None) => { + let system_info = channel + .system_info() + .await + .context("Failed to detect remote operating system")?; + + // If system reports a default shell, use it, otherwise pick a default based on the + // operating system being windows or non-windows + let shell: ShellOption = if !system_info.shell.is_empty() { + system_info.shell.parse() + } else if system_info.family.eq_ignore_ascii_case("windows") { + "cmd.exe".parse() + } else { + "/bin/sh".parse() + } + .map_err(|x: ParseShellError| anyhow::anyhow!(x))?; + + shell + .make_cmd_string(&cmd) + .map_err(|x| anyhow::anyhow!(x))? + } + + // Use explicit shell + Some(Some(shell)) => shell + .make_cmd_string(&cmd) + .map_err(|x| anyhow::anyhow!(x))?, + }; + if let Some(scheme) = lsp { debug!( "Spawning LSP server (pty = {}, cwd = {:?}): {}", pty, current_dir, cmd ); - Lsp::new(channel.into_client().into_channel()) + Lsp::new(channel) .spawn(cmd, current_dir, scheme, pty, MAX_PIPE_CHUNK_SIZE) .await?; } else if pty { @@ -404,7 +443,7 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { "Spawning pty process (environment = {:?}, cwd = {:?}): {}", environment, current_dir, cmd ); - Shell::new(channel.into_client().into_channel()) + Shell::new(channel) .spawn( cmd, environment.into_map(), @@ -421,7 +460,7 @@ async fn async_run(cmd: ClientSubcommand) -> CliResult { .environment(environment.into_map()) .current_dir(current_dir) .pty(None) - .spawn(channel.into_client().into_channel(), &cmd) + .spawn(channel, &cmd) .await .with_context(|| format!("Failed to spawn {cmd}"))?; diff --git a/src/options.rs b/src/options.rs index 8ac9e54..d77821b 100644 --- a/src/options.rs +++ b/src/options.rs @@ -463,6 +463,11 @@ pub enum ClientSubcommand { #[clap(long)] pty: bool, + /// If specified, will spawn the process in the specified shell, defaulting to the + /// user-configured shell. + #[clap(long, name = "SHELL")] + shell: Option>, + /// Alternative current directory for the remote process #[clap(long)] current_dir: Option, @@ -1938,6 +1943,7 @@ mod tests { current_dir: None, environment: map!(), lsp: Some(None), + shell: Some(None), pty: true, cmd_str: None, cmd: vec![String::from("cmd")], @@ -1977,6 +1983,7 @@ mod tests { current_dir: None, environment: map!(), lsp: Some(None), + shell: Some(None), pty: true, cmd_str: None, cmd: vec![String::from("cmd")], @@ -2003,6 +2010,7 @@ mod tests { current_dir: None, environment: map!(), lsp: Some(None), + shell: Some(None), pty: true, cmd_str: None, cmd: vec![String::from("cmd")], @@ -2042,6 +2050,7 @@ mod tests { current_dir: None, environment: map!(), lsp: Some(None), + shell: Some(None), pty: true, cmd_str: None, cmd: vec![String::from("cmd")], diff --git a/src/options/common.rs b/src/options/common.rs index 6f1efb9..1d34ed5 100644 --- a/src/options/common.rs +++ b/src/options/common.rs @@ -3,6 +3,7 @@ mod cmd; mod logging; mod network; mod search; +mod shell; mod time; mod value; @@ -11,5 +12,6 @@ pub use cmd::*; pub use logging::*; pub use network::*; pub use search::*; +pub use shell::*; pub use time::*; pub use value::*; diff --git a/src/options/common/shell.rs b/src/options/common/shell.rs new file mode 100644 index 0000000..bb23e56 --- /dev/null +++ b/src/options/common/shell.rs @@ -0,0 +1,196 @@ +use derive_more::{Display, Error}; +use std::str::FromStr; +use typed_path::{Utf8UnixPath, Utf8WindowsPath}; + +/// Represents a shell to execute on the remote machine. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Shell { + /// Represents the path to the shell on the remote machine. + pub path: String, + + /// Represents the kind of shell. + pub kind: ShellKind, +} + +impl Shell { + #[inline] + pub fn is_posix(&self) -> bool { + self.kind.is_posix() + } + + /// Wraps a `cmd` such that it is invoked by this shell. + /// + /// * For `cmd.exe`, this wraps in double quotes such that it can be invoked by `cmd.exe /S /C "..."`. + /// * For `powershell.exe`, this wraps in single quotes and escapes single quotes by doubling + /// them such that it can be invoked by `powershell.exe -Command '...'`. + /// * For `rc` and `elvish`, this wraps in single quotes and escapes single quotes by doubling them. + /// * For rc and elvish, this uses `shell -c '...'`. + /// * For **POSIX** shells, this wraps in single quotes and uses the trick of `'\''` to fake escape. + /// * For `nu`, this wraps in single quotes or backticks where possible, but fails if the cmd contains single quotes and backticks. + /// + pub fn make_cmd_string(&self, cmd: &str) -> Result { + let path = self.path.as_str(); + + match self.kind { + ShellKind::CmdExe => Ok(format!("{path} /S /C \"{cmd}\"")), + + // NOTE: Powershell does not work directly because our splitting logic for arguments on + // distant-local does not handle single quotes. In fact, the splitting logic + // isn't designed for powershell at all. To get around that limitation, we are + // using cmd.exe to invoke powershell, which fits closer to our parsing rules. + // Crazy, I know! Eventually, we should switch to properly using powershell + // and escaping single quotes by doubling them. + ShellKind::PowerShell => Ok(format!( + "cmd.exe /S /C \"{path} -Command {}\"", + cmd.replace('"', "\"\""), + )), + + ShellKind::Rc | ShellKind::Elvish => { + Ok(format!("{path} -c '{}'", cmd.replace('\'', "''"))) + } + + ShellKind::Nu => { + let has_single_quotes = cmd.contains('\''); + let has_backticks = cmd.contains('`'); + + match (has_single_quotes, has_backticks) { + // If we have both single quotes and backticks, fail + (true, true) => { + Err("unable to escape single quotes and backticks at the same time with nu") + } + + // If we only have single quotes, use backticks + (true, false) => Ok(format!("{path} -c `{cmd}`")), + + // Otherwise, we can safely use single quotes + _ => Ok(format!("{path} -c '{cmd}'")), + } + } + + // We assume anything else not specially handled is POSIX + _ => Ok(format!("{path} -c '{}'", cmd.replace('\'', "'\\''"))), + } + } +} + +#[derive(Clone, Debug, PartialEq, Eq, Display, Error)] +pub struct ParseShellError(#[error(not(source))] String); + +impl FromStr for Shell { + type Err = ParseShellError; + + fn from_str(s: &str) -> Result { + let s = s.trim(); + + let kind = ShellKind::identify(s) + .ok_or_else(|| ParseShellError(format!("Unsupported shell: {s}")))?; + + Ok(Self { + path: s.to_string(), + kind, + }) + } +} + +/// Supported types of shells. +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +#[non_exhaustive] +pub enum ShellKind { + Ash, + Bash, + CmdExe, + Csh, + Dash, + Elvish, + Fish, + Ksh, + Loksh, + Mksh, + Nu, + Pdksh, + PowerShell, + Rc, + Scsh, + Sh, + Tcsh, + Zsh, +} + +impl ShellKind { + /// Returns true if shell represents a POSIX-compliant implementation. + pub fn is_posix(&self) -> bool { + matches!( + self, + Self::Ash + | Self::Bash + | Self::Csh + | Self::Dash + | Self::Fish + | Self::Ksh + | Self::Loksh + | Self::Mksh + | Self::Pdksh + | Self::Scsh + | Self::Sh + | Self::Tcsh + | Self::Zsh + ) + } + + /// Identifies the shell kind from the given string. This string could be a Windows path, Unix + /// path, or solo shell name. + /// + /// The process is handled by these steps: + /// + /// 1. Check if the string matches a shell name verbatim + /// 2. Parse the path as a Unix path and check the file name for a match + /// 3. Parse the path as a Windows path and check the file name for a match + /// + pub fn identify(s: &str) -> Option { + Self::from_name(s) + .or_else(|| Utf8UnixPath::new(s).file_name().and_then(Self::from_name)) + .or_else(|| { + Utf8WindowsPath::new(s) + .file_name() + .and_then(Self::from_name) + }) + } + + fn from_name(name: &str) -> Option { + macro_rules! map_str { + ($($name:literal -> $value:expr),+ $(,)?) => {{ + $( + if name.trim().eq_ignore_ascii_case($name) { + return Some($value); + } + + )+ + + None + }}; + } + + map_str! { + "ash" -> Self::Ash, + "bash" -> Self::Bash, + "cmd" -> Self::CmdExe, + "cmd.exe" -> Self::CmdExe, + "csh" -> Self::Csh, + "dash" -> Self::Dash, + "elvish" -> Self::Elvish, + "fish" -> Self::Fish, + "ksh" -> Self::Ksh, + "loksh" -> Self::Loksh, + "mksh" -> Self::Mksh, + "nu" -> Self::Nu, + "pdksh" -> Self::Pdksh, + "powershell" -> Self::PowerShell, + "powershell.exe" -> Self::PowerShell, + "rc" -> Self::Rc, + "scsh" -> Self::Scsh, + "sh" -> Self::Sh, + "tcsh" -> Self::Tcsh, + "zsh" -> Self::Zsh, + } + } +}