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

Allow conversion of std::process::Command into a openssh::Command given a openssh::Session. #112

Merged
merged 27 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
e314422
Add OverSsh trait to allow conversion of std::process::Command into a…
aalekhpatel07 Jan 31, 2023
d9ba8d3
Add a doc comment and example usage.
aalekhpatel07 Jan 31, 2023
dd64633
Add tests for OverSsh's with_session
aalekhpatel07 Jan 31, 2023
9743a5e
Use raw_command and raw_args. Refactor the name to 'over_ssh'.
aalekhpatel07 Jan 31, 2023
0a69ed1
Remove a dangling test import. Use better name for test.
aalekhpatel07 Jan 31, 2023
974cd52
Clean up doc comment.
aalekhpatel07 Jan 31, 2023
cca22b1
Apply suggestions of code review.
aalekhpatel07 Jan 31, 2023
66cecba
Update src/command.rs
aalekhpatel07 Jan 31, 2023
511c62c
Apply more suggestions from the code review.
aalekhpatel07 Jan 31, 2023
d14a4c2
Apply more suggestions from the code review.
aalekhpatel07 Jan 31, 2023
32a8960
Update src/command.rs
aalekhpatel07 Jan 31, 2023
08b77a5
Update src/lib.rs
aalekhpatel07 Jan 31, 2023
5d9a112
Update src/escape.rs
aalekhpatel07 Jan 31, 2023
7d84871
Apply more suggestions. Add a test for non-utf8 chars for escape.
aalekhpatel07 Jan 31, 2023
c4baafd
Slight refactor of escape.
CdnCentreForChildProtection Jan 31, 2023
7e84beb
Run cargo clippy --fix and persist all modifications to escape.rs
CdnCentreForChildProtection Jan 31, 2023
7fcfaf2
Apply rustfmt to command.rs and escape.rs
CdnCentreForChildProtection Jan 31, 2023
13d865a
Lint over_session test
CdnCentreForChildProtection Jan 31, 2023
c4ded38
Update src/escape.rs
aalekhpatel07 Feb 1, 2023
67d0885
Update src/escape.rs
aalekhpatel07 Feb 1, 2023
6970753
Update src/escape.rs
aalekhpatel07 Feb 1, 2023
8207907
Apply more suggestions.
aalekhpatel07 Feb 6, 2023
10687fb
Rustfmt fix.
aalekhpatel07 Feb 6, 2023
020366a
Fix a doctest for overssh.
aalekhpatel07 Feb 11, 2023
6842e5a
Add a test for overssh that requires escaping the argument passed to …
aalekhpatel07 Feb 11, 2023
e09d2fd
Update overssh require-escaping-arguments test to contain a space.
aalekhpatel07 Feb 11, 2023
f30d34e
Apply rustfmt to tests/openssh.rs
aalekhpatel07 Feb 11, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 126 additions & 0 deletions src/command.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use crate::escape::escape;

use super::stdio::TryFromChildIo;
use super::RemoteChild;
use super::Stdio;
Expand Down Expand Up @@ -49,6 +51,130 @@
}};
}

/// If a command is `OverSsh` then it can be executed over an SSH session.
///
/// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`.
pub trait OverSsh {
/// Given an ssh session, return a command that can be executed over that ssh session.
///
/// ### Notes
///
/// The command to be executed on the remote machine should not explicitly
/// set environment variables or the current working directory. It errors if the source command
/// has environment variables or a current working directory set, since `openssh` doesn't (yet) have
/// a method to set environment variables and `ssh` doesn't support setting a current working directory
/// outside of `bash/dash/zsh` (which is not always available).
///
/// ### Examples
///
/// 1. Consider the implementation of `OverSsh` for `std::process::Command`. Let's build a
/// `ls -l -a -h` command and execute it over an SSH session.
///
/// ```no_run
/// # #[tokio::main(flavor = "current_thread")]
/// async fn main() -> Result<(), Box<dyn std::error::Error>> {
/// use std::process::Command;
/// use openssh::{Session, KnownHosts, OverSsh};
///
/// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?;
/// let ls =
/// Command::new("ls")
/// .arg("-l")
/// .arg("-a")
/// .arg("-h")
/// .over_ssh(&session)?
/// .output()
/// .await?;
///
/// assert!(String::from_utf8(ls.stdout).unwrap().contains("total"));
/// # Ok(())
/// }
///
/// ```
/// 2. Building a command with environment variables or a current working directory set will
/// results in an error.
///
/// ```no_run
/// # #[tokio::main(flavor = "current_thread")]
/// async fn main() -> Result<(), Box<dyn std::error::Error>> {
/// use std::process::Command;
/// use openssh::{Session, KnownHosts, OverSsh};
///
/// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?;
/// let echo =
/// Command::new("echo")
/// .env("MY_ENV_VAR", "foo")
/// .arg("$MY_ENV_VAR")
jonhoo marked this conversation as resolved.
Show resolved Hide resolved
/// .over_ssh(&session);
/// assert!(matches!(echo, Err(openssh::Error::CommandHasEnv)));
///
/// # Ok(())
/// }
///
/// ```
fn over_ssh<'session>(
&self,
session: &'session Session,
) -> Result<crate::Command<'session>, crate::Error>;
}

NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
impl OverSsh for std::process::Command {
NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
fn over_ssh<'session>(
&self,
session: &'session Session,
) -> Result<Command<'session>, crate::Error> {
// I'd really like `!self.get_envs().is_empty()` here, but that's
// behind a `exact_size_is_empty` feature flag.
if self.get_envs().len() > 0 {
return Err(crate::Error::CommandHasEnv);
}

if self.get_current_dir().is_some() {
return Err(crate::Error::CommandHasCwd);
}

let program_escaped: Cow<'_, OsStr> = escape(self.get_program());
let mut command = session.raw_command(program_escaped);

Check warning on line 137 in src/command.rs

View workflow job for this annotation

GitHub Actions / check

variable does not need to be mutable

let args = self.get_args().map(escape);
command.raw_args(args);
jonhoo marked this conversation as resolved.
Show resolved Hide resolved
Ok(command)
}
}

impl OverSsh for tokio::process::Command {
fn over_ssh<'session>(
&self,
session: &'session Session,
) -> Result<Command<'session>, crate::Error> {
self.as_std().over_ssh(session)
}
}

impl<S> OverSsh for &S
where
S: OverSsh,
{
fn over_ssh<'session>(
&self,
session: &'session Session,
) -> Result<Command<'session>, crate::Error> {
<S as OverSsh>::over_ssh(self, session)
}
}

impl<S> OverSsh for &mut S
where
S: OverSsh,
{
fn over_ssh<'session>(
&self,
session: &'session Session,
) -> Result<Command<'session>, crate::Error> {
<S as OverSsh>::over_ssh(self, session)
}
}

/// A remote process builder, providing fine-grained control over how a new remote process should
/// be spawned.
///
Expand Down
10 changes: 10 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,16 @@ pub enum Error {
/// IO Error when creating/reading/writing from ChildStdin, ChildStdout, ChildStderr.
#[error("failure while accessing standard i/o of remote process")]
ChildIo(#[source] io::Error),

/// The command has some env variables that it expects to carry over ssh.
/// However, OverSsh does not support passing env variables over ssh.
#[error("rejected runing a command over ssh that expects env variables to be carried over to remote.")]
CommandHasEnv,

/// The command expects to be in a specific working directory in remote.
/// However, OverSsh does not support setting a working directory for commands to be executed over ssh.
#[error("rejected runing a command over ssh that expects a specific working directory to be carried over to remote.")]
CommandHasCwd,
}

#[cfg(feature = "native-mux")]
Expand Down
91 changes: 91 additions & 0 deletions src/escape.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
//! Escape characters that may have special meaning in a shell, including spaces.
//! This is a modified version of the [`shell-escape::unix`] module of [`shell-escape`] crate.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we submit a PR to shell-escape to add a version supporting OsStr there directly? I'm okay with us landing our own version initially, but it'd be way better if we could just use a version from them in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! I've initiated a PR to shell-escape and if it gets merged, we can avoid having a src/escape.rs altogether.

//!
//! [`shell-escape`]: https://crates.io/crates/shell-escape
//! [`shell-escape::unix`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101

use std::{
borrow::Cow,
ffi::{OsStr, OsString},
os::unix::ffi::OsStrExt,
os::unix::ffi::OsStringExt,
};

fn allowed(byte: u8) -> bool {
matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'=' | b'/' | b',' | b'.' | b'+')
}

/// Escape characters that may have special meaning in a shell, including spaces.
///
/// **Note**: This function is an adaptation of [`shell-escape::unix::escape`].
/// This function exists only for type compatibility and the implementation is
/// almost exactly the same as [`shell-escape::unix::escape`].
///
/// [`shell-escape::unix::escape`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101
///
pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> {
let as_bytes = s.as_bytes();
let all_allowed = as_bytes.iter().copied().all(allowed);

if !as_bytes.is_empty() && all_allowed {
return Cow::Borrowed(s);
}

let mut escaped = Vec::with_capacity(as_bytes.len() + 2);
escaped.push(b'\'');

for &b in as_bytes {
match b {
b'\'' | b'!' => {
escaped.reserve(4);
escaped.push(b'\'');
NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
aalekhpatel07 marked this conversation as resolved.
Show resolved Hide resolved
escaped.push(b'\\');
escaped.push(b);
escaped.push(b'\'');
}
_ => escaped.push(b),
}
}
escaped.push(b'\'');
OsString::from_vec(escaped).into()
}

#[cfg(test)]
mod tests {
use super::*;

fn test_escape_case(input: &str, expected: &str) {
NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
test_escape_from_bytes(input.as_bytes(), expected.as_bytes())
}

fn test_escape_from_bytes(input: &[u8], expected: &[u8]) {
let input_os_str = OsStr::from_bytes(input);
let observed_os_str = escape(input_os_str);
let expected_os_str = OsStr::from_bytes(expected);
assert_eq!(observed_os_str, expected_os_str);
}

// These tests are courtesy of the `shell-escape` crate.
#[test]
fn test_escape() {
test_escape_case(
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+",
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+",
);
test_escape_case("--aaa=bbb-ccc", "--aaa=bbb-ccc");
test_escape_case(
"linker=gcc -L/foo -Wl,bar",
r#"'linker=gcc -L/foo -Wl,bar'"#,
);
test_escape_case(r#"--features="default""#, r#"'--features="default"'"#);
test_escape_case(r#"'!\$`\\\n "#, r#"''\'''\!'\$`\\\n '"#);
test_escape_case("", r#"''"#);
test_escape_case(" ", r#"' '"#);
test_escape_case("*", r#"'*'"#);

test_escape_from_bytes(
&[0x66, 0x6f, 0x80, 0x6f],
&[b'\'', 0x66, 0x6f, 0x80, 0x6f, b'\''],
);
}
}
4 changes: 3 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,9 @@ mod builder;
pub use builder::{KnownHosts, SessionBuilder};

mod command;
pub use command::Command;
pub use command::{Command, OverSsh};

mod escape;

mod child;
pub use child::RemoteChild;
Expand Down
84 changes: 84 additions & 0 deletions tests/openssh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@
path
}

async fn session_builder_connect(mut builder: SessionBuilder, addr: &str) -> Vec<Session> {

Check warning on line 38 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

unused variable: `addr`
let mut sessions = Vec::with_capacity(2);

Check warning on line 39 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

variable does not need to be mutable

builder.user_known_hosts_file(get_known_hosts_path());

Expand All @@ -54,7 +54,7 @@
}

async fn connects_with_name() -> Vec<(Session, &'static str)> {
let mut sessions = Vec::with_capacity(2);

Check warning on line 57 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

variable does not need to be mutable

let mut builder = SessionBuilder::default();

Expand Down Expand Up @@ -87,12 +87,12 @@
session_builder_connects_err(host, SessionBuilder::default()).await
}

async fn session_builder_connects_err(host: &str, mut builder: SessionBuilder) -> Vec<Error> {

Check warning on line 90 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

unused variable: `host`
builder
.user_known_hosts_file(get_known_hosts_path())
.known_hosts_check(KnownHosts::Accept);

let mut errors = Vec::with_capacity(2);

Check warning on line 95 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

variable does not need to be mutable

#[cfg(feature = "process-mux")]
{
Expand Down Expand Up @@ -292,6 +292,90 @@
}
}

#[tokio::test]
#[cfg_attr(not(ci), ignore)]
async fn over_session_ok() {
for session in connects().await {
let mut command = std::process::Command::new("echo")
.arg("foo")
.over_ssh(&session)
.expect("No env vars or current working dir is set.");

let child = command.output().await.unwrap();
assert_eq!(child.stdout, b"foo\n");

let child = session
.command("echo")
.arg("foo")
.raw_arg(">")
.arg("/dev/stderr")
.output()
.await
.unwrap();
assert!(child.stdout.is_empty());

session.close().await.unwrap();
}
}

#[tokio::test]
#[cfg_attr(not(ci), ignore)]
async fn over_session_ok_require_escaping_arguments() {
for session in connects().await {
let mut command = std::process::Command::new("echo")
.arg("\"\'\' foo \'\'\"")
.over_ssh(&session)
.expect("No env vars or current working dir is set.");

let child = command.output().await.unwrap();
assert_eq!(child.stdout, b"\"\'\' foo \'\'\"\n");

let child = session
.command("echo")
.arg("foo")
.raw_arg(">")
.arg("/dev/stderr")
.output()
.await
.unwrap();
assert!(child.stdout.is_empty());

session.close().await.unwrap();
}
}

/// Test that `over_ssh` errors if the source command has env vars specified.
#[tokio::test]
#[cfg_attr(not(ci), ignore)]
async fn over_session_err_because_env_var() {
for session in connects().await {
let command_with_env = std::process::Command::new("printenv")
.arg("MY_ENV_VAR")
.env("MY_ENV_VAR", "foo")
.over_ssh(&session);
assert!(matches!(
command_with_env,
Err(openssh::Error::CommandHasEnv)
));
}
}

/// Test that `over_ssh` errors if the source command has a `current_dir` specified.
#[tokio::test]
#[cfg_attr(not(ci), ignore)]
async fn over_session_err_because_cwd() {
for session in connects().await {
let command_with_current_dir = std::process::Command::new("echo")
.arg("foo")
.current_dir("/tmp")
.over_ssh(&session);
assert!(matches!(
command_with_current_dir,
Err(openssh::Error::CommandHasCwd)
));
}
}

#[tokio::test]
#[cfg_attr(not(ci), ignore)]
async fn shell() {
Expand Down Expand Up @@ -436,13 +520,13 @@

#[tokio::test]
async fn connect_timeout() {
use std::time::{Duration, Instant};

Check warning on line 523 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

unused import: `Instant`

let mut sb = SessionBuilder::default();
sb.connect_timeout(Duration::from_secs(1))
.user_known_hosts_file(get_known_hosts_path());

let host = "192.0.0.8";

Check warning on line 529 in tests/openssh.rs

View workflow job for this annotation

GitHub Actions / check

unused variable: `host`

// Test process_impl
#[cfg(feature = "process-mux")]
Expand Down
Loading