Use ssh2 instead of std::process::Command #172

kjughx · 2025-02-03T18:44:15Z

Remove the use of std::process::Command("ssh") and use ssh2 create instead. It's a bit clunky when it comes to creating processes but it gets the job done.

apljungquist

Nice, I've been wanting to do this for a long time.

This review focused on the functionality of the binary crates and the interface of the library crate. I plan to take a closer look at the implementation once it is working.

crates/acap-ssh-utils/src/main.rs

crates/acap-ssh-utils/src/lib.rs

crates/acap-ssh-utils/src/main.rs

crates/cargo-acap-sdk/src/commands/run_command.rs

crates/acap-ssh-utils/src/lib.rs

apljungquist · 2025-02-13T08:15:53Z

With this we can probably remove sshpass from the dev container too

apljungquist

Apologies for the slow feedback.

crates/acap-ssh-utils/src/lib.rs

crates/cargo-acap-sdk/src/commands/run_command.rs

crates/acap-ssh-utils/src/lib.rs

* upstream/main: chore: Update rust toolchain to 1.85.1 (AxisCommunications#175)

apljungquist

Bed time; will take another look tomorrow.

apljungquist · 2025-03-28T16:47:02Z

crates/acap-ssh-utils/src/lib.rs

-        let mut stat = sftp.stat(path)?;
+        let sftp = session.sftp().context("Creating sftp session")?;
+        sftp.create(path)
+            .context(format!("Creating {:?}", path))?


Consider using with_context to avoid creating these strings eagerly.

apljungquist · 2025-03-28T16:49:05Z

crates/acap-ssh-utils/src/lib.rs

-        let sftp = session.sftp()?;
-        sftp.create(path)?.write_all(&std::fs::read(prog)?)?;
-        let mut stat = sftp.stat(path)?;
+        let sftp = session.sftp().context("Creating sftp session")?;


Nice! I'm usually too lazy to add context, but it really does help when troubleshooting.

crates/acap-ssh-utils/src/lib.rs

apljungquist · 2025-03-31T20:08:45Z

crates/acap-ssh-utils/src/lib.rs

-}
+        cmd.arg(executable);
+        if let Some(args) = args {
+            cmd.arg("--"); // They should be passed to the process, not to 'su'


Have you tested this?

I don't have a camera at hand, but su --help on Debian does not mention that -- can be used and experimenting seems to confirm this:

$ su --help Usage: su [options] [-] [<user> [<argument>...]] ... $ env --help env --help Usage: env [OPTION]... [-] [NAME=VALUE]... [COMMAND [ARG]...] ... $ su root -c env -- --help SHELL=/bin/bash ... I understand the camera might differ, so this is just to double check. I will make sure to get a camera set up in the next week so that I can check things like this myself and not have to bother you.

You're right. When I was running the tests it was complaining since we pass --test-threads=1 to the test. Adding the '--' suppressed this but that's not the correct way to do it. We need to pass the executable with its arguments as a whole, i.e. as a string:

su root -c "env --help"

I've fixed that now and added a test in inspect_env to make sure this stays working.

Copilot

Pull Request Overview

This PR replaces the usage of std::process::Command for SSH operations with the ssh2 crate, aiming to simplify and standardize remote command execution and file transfers. Key changes include:

Establishing SSH sessions using ssh2::Session and TcpStream in both test and run commands.
Updating acap-ssh-utils to use ssh2 for executing remote commands and performing SFTP operations.
Adding the ssh2 dependency across the workspace Cargo.toml files.

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/cargo-acap-sdk/src/commands/test_command.rs	Migrated SSH command execution to ssh2.
crates/cargo-acap-sdk/src/commands/run_command.rs	Converted process-based SSH interactions to ssh2 sessions.
crates/cargo-acap-sdk/Cargo.toml	Added ssh2 dependency.
crates/acap-ssh-utils/src/main.rs	Updated CLI execution to utilize ssh2 sessions.
crates/acap-ssh-utils/src/lib.rs	Redesigned remote command execution and file transfer routines using ssh2.
crates/acap-ssh-utils/Cargo.toml	Added ssh2 dependency.
Cargo.toml	Included ssh2 dependency for the main project.

Files not reviewed (1)

.devhost/install-system-packages.sh: Language not supported

Comments suppressed due to low confidence (1)

crates/acap-ssh-utils/src/lib.rs:50

Using debug formatting ("{cmd:?}") to construct the remote command string may introduce unintended quotes or formatting artifacts; consider using a standard string formatting method to build the command.

cmd: format("{cmd:?}"),

Copilot · 2025-04-01T17:00:30Z

crates/acap-ssh-utils/src/main.rs

+    sess.userauth_password(&cli.netloc.user, &cli.netloc.pass)
+        .unwrap();
+
+    match Cli::parse().exec(&sess, cli.netloc.user == "root") {


The CLI arguments are parsed twice; consider reusing the 'cli' variable that is already stored to avoid potential inconsistencies.

Suggested change

match Cli::parse().exec(&sess, cli.netloc.user == "root") {

match cli.exec(&sess, cli.netloc.user == "root") {

apljungquist · 2025-04-01T16:58:03Z

crates/acap-ssh-utils/src/lib.rs

+            if n == 0 {
+                break;
            }
+            print!("{}", std::str::from_utf8(&buf[..n])?);


Why is there a print here?

That's the stdout from the (remote) process being redirected to the stdout of the local process (acap-ssh-utils/cargo-acap-sdk)

apljungquist · 2025-04-01T17:21:11Z

crates/acap-ssh-utils/src/lib.rs

+/// `session` is a ssh2::Session connected to the remote host.
 /// `package` is the name of the ACAP app to emulate.
 /// `env` is a map of environment variables to override on the remote host.
+/// `as_root` is a boolean that indicates if the process should be run as root


Am I reading this right?

as_root=true means the program is never executed as root.

doh. It should of course be the other way around. There's no converse to then for false so I'll just invert the parameter: as_package_user.

apljungquist · 2025-04-01T17:35:21Z

crates/acap-ssh-utils/src/lib.rs

+                .context(format!("Creating {:?}", entry.path()?))?;
+            file.write_all(&buf)
+                .context(format!("Writing to {:?}", entry.path()?))?;
+            file.setstat(stat)


Does this work if the app contains files that are symlinks?

inspect_env has a couple of symlinks, so that could be useful for testing.

It does not. I'll fix that.

apljungquist

I'm sorry that I have been slow to review this PR repeatedly, I will try to be quicker in the future.

However, going forward I would like to ask that you change one thing at a time when feasible. That way it will be easier for me to respond in a timely manner and we will hopefully not need as many iterations. I think some of the suspected regressions that I have pointed out could have been avoided, or at least deferred, if this PR only replaced Command with ssh2 and did not also rework the way we build the command that is run on the remote.

I appreciate your contributions across this project and I hope the above blunt feedback does not discourage you from finishing this contribution and contributing again in the future 😊

apljungquist · 2025-04-02T17:25:08Z

crates/cargo-acap-sdk/src/commands/run_command.rs

When I try this, it doesn't work. Are you able to reproduce the error I see?

$ git checkout 5e8ff7a967a604fc16938de5daedb95e25498cc8 Previous HEAD position was efc9365 Provide a futures_lite::Stream implementation for event subscription (#174) HEAD is now at 5e8ff7a Use is_ok_and instead of expect $ cargo build --bin cargo-acap-sdk # ... $ ./target/debug/cargo-acap-sdk run -- -p inspect_env Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.50s Error: G_SLICE="always-malloc" RUST_LOG="debug" RUST_LOG_STYLE="always" "su" "acap-inspect_env" "-c" "/usr/local/packages/inspect_env/inspect_env" "--" exited with status 1

I hope the above blunt feedback does not discourage you ...

Of course not, I agree this review got a bit out of hand. If you want I can try and split it up into easier to digest PRs.

When I try this, it doesn't work. Are you able to reproduce the error I see?

No I'm not, however I have another issue: there's no output from the program. As you rightly pointed, patch_package didn't work with symlinks.

* upstream/main: fix(axstorage)! Return `GString` from `get_path` (AxisCommunications#170) fix(inspect_app): Print environment variables individually (AxisCommunications#181) chore(deps): bump thiserror from 1.0.63 to 2.0.12 (AxisCommunications#178) chore(deps): bump openssl from 0.10.64 to 0.10.71 (AxisCommunications#177) chore(deps): bump ghcr.io/devcontainers/features/common-utils (AxisCommunications#179) chore(deps): bump glib and glib-sys to 0.20.9 (AxisCommunications#180) Provide a futures_lite::Stream implementation for event subscription (AxisCommunications#174) chore(deps): bump the default group across 1 directory with 22 updates (AxisCommunications#173)

apljungquist

Just skimmed it this time because I'm not sure if it's ready for another round of reviews. So please let me know if/when it is.

apljungquist · 2025-04-17T07:51:03Z

apps/inspect_env/src/main.rs

        PathBuf::from("/usr/local/packages").join(PACKAGE_NAME)
    }

+    #[test]


I like the idea of adding more automated tests.

However, the intention behind these tests is not only to ensure that apps are invoked as expected using the run and test commands, but also to help ensure that the way these commands invoke our program is the same as the way the ACAP system in the camera invokes our program. As such, these should all pass if the app is installed and started the "normal" way e.g. using the GUI.

My thinking is roughly:

We create tests to verify some functionality

The important thing is whether that functionality works as intended when the app is used as intended, i.e. installed and started via official non-dev APIs.

Therefore our tests should all pass when under the same conditions.

So these should give the same results (all tests passing):

cargo-acap-sdk install -- -p inspect_env --tests && cargo-acap-sdk start -p inspect_env

cargo-acap-sdk test -- -p inspect_env

That being said, as I was writing the above I realized that this is not true for tests that cannot run in parallel, which is why I added --test-threads=1 in the first place. My best idea for how to solve this right now is to establish the convention that tests that require --test-threads=1 should be ignored and that the test command also sets --ignored. Not fantastic, but could work.

What do you think?

Another idea is to rewrite mutually exclusive tests as integration tests. This probably works out of the box if the crate has an ACAP manifest in the Cargo manifest directory, as all the apps do.

kjughx marked this pull request as ready for review February 3, 2025 18:44

kjughx requested a review from a team as a code owner February 3, 2025 18:44

apljungquist requested changes Feb 8, 2025

View reviewed changes

Philip Johansson added 4 commits February 12, 2025 17:17

Use ssh2 instead of std::process::Command

ed6d619

Append ssh port if not already there

c5811d8

Add missing G_SLICE env variable

61d992c

Use process::Command to construct shell command

a566f08

kjughx force-pushed the rust-ssh branch from e75a5e5 to a566f08 Compare February 12, 2025 16:21

apljungquist requested changes Feb 12, 2025

View reviewed changes

Philip Johansson added 15 commits February 13, 2025 11:22

Set G_SLICE in acap-ssh-utils instead

ee2828c

Handle the exit_status of remote program

327dc82

Channels can't be reused so create it only when needed

7bc38a0

Use bail! macro

c236986

Return Vec<u8> from exec_capture_output

38a275d

Make session blocking

537e846

Change println! to debug!

1cbac37

Handle root/acap user differently

1d8fd32

Change unwrap to expect for clarity

ede5781

Remove unnecessary assignment

193268f

Always append port 22 to host

6cb7f19

Handle directories with sftp

f832a11

Handle arguments to remote programs when using 'su'

5580324

Add contexts to fallible sftp calls

6df6689

Fix lint

67ad2e5

apljungquist reviewed Feb 25, 2025

View reviewed changes

Philip Johansson added 4 commits March 27, 2025 09:56

Merge remote-tracking branch 'upstream/main' into rust-ssh

ac11ef6

* upstream/main: chore: Update rust toolchain to 1.85.1 (AxisCommunications#175)

Update docstrings

d1f8363

Remove sshpass from system requirements

1f2dd63

Use is_ok_and instead of expect

5e8ff7a

apljungquist reviewed Mar 31, 2025

View reviewed changes

apljungquist requested a review from Copilot April 1, 2025 16:59

Copilot AI reviewed Apr 1, 2025

View reviewed changes

apljungquist reviewed Apr 1, 2025

View reviewed changes

apljungquist requested changes Apr 2, 2025

View reviewed changes

Philip Johansson added 8 commits April 16, 2025 16:38

Change as_root to as_package_user

ecd277e

Handle symlinks

29aa98b

Change CWD when running ACAP

3020e38

Use with_context instead of context

7afa257

Avoid unnecessary unwraps

1757a27

Pass arguments correctly + test

020cfb9

Reformat

94ee183

apljungquist requested changes Apr 17, 2025

View reviewed changes

	match Cli::parse().exec(&sess, cli.netloc.user == "root") {
	match cli.exec(&sess, cli.netloc.user == "root") {

Use ssh2 instead of std::process::Command #172

Are you sure you want to change the base?

Use ssh2 instead of std::process::Command #172

Uh oh!

Conversation

kjughx commented Feb 3, 2025

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apljungquist commented Feb 13, 2025

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apljungquist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment