diff --git a/.config/nextest.toml b/.config/nextest.toml index b3bdf3a..400b353 100644 --- a/.config/nextest.toml +++ b/.config/nextest.toml @@ -2,3 +2,5 @@ fail-fast = false retries = 2 slow-timeout = { period = "60s", terminate-after = 3 } +status-level = "fail" +final-status-level = "fail" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2d43ce..1bd5007 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,16 +26,18 @@ jobs: git config --system core.autocrlf false git config --system core.eol lf if: matrix.os == 'windows-latest' - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Rust (clippy) uses: actions-rs/toolchain@v1 with: profile: minimal toolchain: stable components: clippy - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Check Cargo availability run: cargo --version + - name: distant-net (all features) + run: cargo clippy -p distant-net --all-targets --verbose --all-features - name: distant-core (all features) run: cargo clippy -p distant-core --all-targets --verbose --all-features - name: distant-ssh2 (all features) @@ -57,20 +59,22 @@ jobs: git config --system core.autocrlf false git config --system core.eol lf if: matrix.os == 'windows-latest' - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Rust (rustfmt) uses: actions-rs/toolchain@v1 with: profile: minimal toolchain: stable components: rustfmt - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Check Cargo availability run: cargo --version - run: cargo fmt --all -- --check tests: name: "Test Rust ${{ matrix.rust }} on ${{ matrix.os }}" runs-on: ${{ matrix.os }} + env: + RUSTFLAGS: --cfg ci strategy: fail-fast: false matrix: @@ -80,7 +84,7 @@ jobs: - { rust: stable, os: ubuntu-latest } - { rust: 1.61.0, os: ubuntu-latest } steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Rust ${{ matrix.rust }} uses: actions-rs/toolchain@v1 with: @@ -90,7 +94,7 @@ jobs: - uses: taiki-e/install-action@v1 with: tool: cargo-nextest - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Check Cargo availability run: cargo --version - name: Install OpenSSH on Windows @@ -142,22 +146,24 @@ jobs: shell: pwsh if: matrix.os == 'windows-latest' - name: Run net tests (default features) - run: cargo nextest run --profile ci --release --verbose -p distant-net - - name: Run core tests (default features) - run: cargo nextest run --profile ci --release --verbose -p distant-core + run: cargo nextest run --profile ci --release -p distant-net + - name: Build core (default features) + run: cargo build --release -p distant-core - name: Run core tests (all features) - run: cargo nextest run --profile ci --release --verbose --all-features -p distant-core + run: cargo nextest run --profile ci --release --all-features -p distant-core - name: Ensure /run/sshd exists on Unix run: mkdir -p /run/sshd if: matrix.os == 'ubuntu-latest' - - name: Run ssh2 client tests (default features) - run: cargo nextest run --profile ci --release --verbose -p distant-ssh2 ssh2::client + - name: Build ssh2 (default features) + run: cargo build --release -p distant-ssh2 - name: Run ssh2 client tests (all features) - run: cargo nextest run --profile ci --release --verbose --all-features -p distant-ssh2 ssh2::client - - name: Run CLI tests - run: cargo nextest run --profile ci --release --verbose - - name: Run CLI tests (no default features) - run: cargo nextest run --profile ci --release --verbose --no-default-features + run: cargo nextest run --profile ci --release --all-features -p distant-ssh2 ssh2::client + - name: Build CLI (no default features) + run: cargo build --release --no-default-features + - name: Build CLI (default features) + run: cargo build --release + - name: Run CLI tests (all features) + run: cargo nextest run --profile ci --release --all-features ssh-launch-tests: name: "Test ssh launch using Rust ${{ matrix.rust }} on ${{ matrix.os }}" runs-on: ${{ matrix.os }} @@ -167,9 +173,8 @@ jobs: include: - { rust: stable, os: macos-latest } - { rust: stable, os: ubuntu-latest } - - { rust: 1.61.0, os: ubuntu-latest } steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Rust ${{ matrix.rust }} uses: actions-rs/toolchain@v1 with: @@ -178,14 +183,12 @@ jobs: - uses: taiki-e/install-action@v1 with: tool: cargo-nextest - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Check Cargo availability run: cargo --version - name: Install distant cli for use in launch tests run: | cargo install --path . echo "DISTANT_PATH=$HOME/.cargo/bin/distant" >> $GITHUB_ENV - - name: Run ssh2 launch tests (default features) - run: cargo nextest run --profile ci --release --verbose -p distant-ssh2 ssh2::launched - name: Run ssh2 launch tests (all features) - run: cargo nextest run --profile ci --release --verbose --all-features -p distant-ssh2 ssh2::launched + run: cargo nextest run --profile ci --release --all-features -p distant-ssh2 ssh2::launched diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index f75fe53..4b660e3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -20,7 +20,7 @@ jobs: BUILD_BIN: distant UNIVERSAL_REL_BIN: distant-macos steps: - - uses: actions/checkout@v2 + - uses: actions/checkout@v3 - name: Install Rust (x86) uses: actions-rs/toolchain@v1 with: @@ -33,7 +33,7 @@ jobs: profile: minimal toolchain: stable target: ${{ env.ARM_ARCH }} - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Build binary (x86_64) run: | cargo build --release --all-features --target ${{ env.X86_ARCH }} @@ -75,7 +75,7 @@ jobs: profile: minimal toolchain: stable target: ${{ env.X86_ARCH }} - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Build binary (x86_64) run: | cargo build --release --all-features --target ${{ env.X86_ARCH }} @@ -108,7 +108,7 @@ jobs: profile: minimal toolchain: stable target: ${{ env.X86_GNU_ARCH }} - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Build binary (GNU x86_64) run: | cargo build --release --all-features --target ${{ env.X86_GNU_ARCH }} @@ -145,7 +145,7 @@ jobs: - name: Install Rust (MUSL) run: | curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal - - uses: Swatinem/rust-cache@v1 + - uses: Swatinem/rust-cache@v2 - name: Build binary (MUSL x86_64) run: | source $HOME/.cargo/env diff --git a/CHANGELOG.md b/CHANGELOG.md index 584bfe4..82a6a72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `SystemInfo` via ssh backend now detects and reports username and shell +- `SystemInfo` via ssh backend now reports os when windows detected + ### Changed - `SystemInfo` data type now includes two additional fields: `username` and @@ -23,6 +28,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 resolving the hanging that occurred for Windows `cmd.exe` and `powershell.exe` upon exit - ssh launch with login shell now only uses `sh` when remote family is `unix` +- ssh backend implementation of copy now works more widely across windows + systems by switching to `powershell.exe` to perform copy ## [0.18.0] - 2022-08-18 ### Changed diff --git a/Cargo.lock b/Cargo.lock index 7f3f4dd..fcff2fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -834,6 +834,7 @@ dependencies = [ "shell-words", "smol", "tokio", + "typed-path", "wezterm-ssh", "which", "whoami", @@ -3043,6 +3044,12 @@ dependencies = [ "once_cell", ] +[[package]] +name = "typed-path" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0303bfe6ef379273be7ce99d8a6a2e54261fd110a01cf70986ec0fe855ff075e" + [[package]] name = "typenum" version = "1.15.0" diff --git a/distant-ssh2/Cargo.toml b/distant-ssh2/Cargo.toml index 3c0e217..a856ef9 100644 --- a/distant-ssh2/Cargo.toml +++ b/distant-ssh2/Cargo.toml @@ -29,6 +29,7 @@ rpassword = "7.0.0" shell-words = "1.1.0" smol = "1.2.5" tokio = { version = "1.20.1", features = ["full"] } +typed-path = "0.1.0" wezterm-ssh = { version = "0.4.0", default-features = false } winsplit = "0.1.0" diff --git a/distant-ssh2/src/api.rs b/distant-ssh2/src/api.rs index fbd578f..2f6f9e4 100644 --- a/distant-ssh2/src/api.rs +++ b/distant-ssh2/src/api.rs @@ -3,6 +3,7 @@ use crate::{ utils::{self, to_other_error}, }; use async_compat::CompatExt; +use async_once_cell::OnceCell; use async_trait::async_trait; use distant_core::{ data::{ @@ -14,12 +15,16 @@ use log::*; use std::{ collections::{HashMap, HashSet}, io, - path::{Component, PathBuf}, + path::PathBuf, sync::{Arc, Weak}, + time::Duration, }; use tokio::sync::{mpsc, RwLock}; use wezterm_ssh::{FilePermissions, OpenFileType, OpenOptions, Session as WezSession, WriteMode}; +/// Time after copy completes to wait for stdout/stderr to close +const COPY_COMPLETE_TIMEOUT: Duration = Duration::from_secs(1); + #[derive(Default)] pub struct ConnectionState { /// List of process ids that will be killed when the connection terminates @@ -52,6 +57,17 @@ impl SshDistantApi { processes: Arc::new(RwLock::new(HashMap::new())), } } + + /// Checks if the remote server is a Windows machine + async fn is_windows(&self) -> io::Result { + // We cache the request as it should not change for the lifetime of the ssh connection + static IS_WINDOWS: OnceCell = OnceCell::new(); + + // Look up whether the remote system is windows + Ok(*IS_WINDOWS + .get_or_try_init(utils::is_windows(&self.session)) + .await?) + } } #[async_trait] @@ -521,44 +537,39 @@ impl DistantApi for SshDistantApi { ); // NOTE: SFTP does not provide a remote-to-remote copy method, so we instead execute - // a program and hope that it applies, starting with the Unix/BSD/GNU cp method - // and switch to Window's xcopy if the former fails - - // Unix cp -R - let unix_result = self - .session - .exec(&format!("cp -R {:?} {:?}", src, dst), None) - .compat() - .await; - - let failed = unix_result.is_err() || { - let exit_status = unix_result.unwrap().child.async_wait().compat().await; - exit_status.is_err() || !exit_status.unwrap().success() + // a program based on the platform and hope that it applies + let is_windows = self.is_windows().await?; + let output = if is_windows { + utils::powershell_output( + &self.session, + &format!("Copy-Item -Path {src:?} -Destination {dst:?} -Recurse"), + COPY_COMPLETE_TIMEOUT, + ) + .await? + } else { + utils::execute_output( + &self.session, + &format!("cp -R {src:?} {dst:?}"), + COPY_COMPLETE_TIMEOUT, + ) + .await? }; - // Windows xcopy /s /e - if failed { - let exit_status = self - .session - .exec(&format!("xcopy {:?} {:?} /s /e", src, dst), None) - .compat() - .await - .map_err(to_other_error)? - .child - .async_wait() - .compat() - .await - .map_err(to_other_error)?; + // NOTE: For some reason, powershell.exe is not returning an error upon failure, so we + // have to check if we got some stderr as output and consider that a failure + let success = output.success && (!is_windows || output.stderr.is_empty()); - if !exit_status.success() { - return Err(io::Error::new( - io::ErrorKind::Other, - "Unix and windows copy commands failed", - )); - } + if success { + Ok(()) + } else { + Err(io::Error::new( + io::ErrorKind::Other, + format!( + "Copy command failed: {}", + String::from_utf8_lossy(&output.stderr) + ), + )) } - - Ok(()) } async fn rename( @@ -810,38 +821,56 @@ impl DistantApi for SshDistantApi { } async fn system_info(&self, ctx: DistantCtx) -> io::Result { + // We cache each of these requested values since they should not change for the + // lifetime of the ssh connection + static CURRENT_DIR: OnceCell = OnceCell::new(); + static USERNAME: OnceCell = OnceCell::new(); + static SHELL: OnceCell = OnceCell::new(); + debug!("[Conn {}] Reading system information", ctx.connection_id); + // Look up whether the remote system is windows + let is_windows = self.is_windows().await?; + // Look up the current directory - let current_dir = utils::canonicalize(&self.session.sftp(), ".").await?; - - // TODO: Ideally, we would determine the family using something like the following: - // - // cmd.exe /C echo %OS% - // - // Determine OS by printing OS variable (works with Windows 2000+) - // If it matches Windows_NT, then we are on windows - // - // However, the above is not working for whatever reason (always has success == false); so, - // we're purely using a check if we have a drive letter on the canonicalized path to - // determine if on windows for now. - let is_windows = current_dir - .components() - .any(|c| matches!(c, Component::Prefix(_))); - - let family = if is_windows { "windows" } else { "unix" }.to_string(); + let current_dir = CURRENT_DIR + .get_or_try_init(async move { + let current_dir: PathBuf = utils::canonicalize(&self.session.sftp(), ".").await?; + + // If windows, we need to see if we got a weird directory from ssh in the form of + // /C:/... or /C/... as examples. Easiest way is to convert into a WindowsPath, + // check if the first component is a root dir, and then make a new windows path to + // see if it now starts with a prefix. + let current_dir: PathBuf = current_dir + .to_str() + .and_then(utils::convert_to_windows_path_string) + .map(PathBuf::from) + .unwrap_or(current_dir); + + Result::<_, io::Error>::Ok(current_dir) + }) + .await? + .clone(); + + // Look up username and shell + let username = USERNAME + .get_or_try_init(utils::query_username(&self.session, is_windows)) + .await? + .clone(); + + let shell = SHELL + .get_or_try_init(utils::query_shell(&self.session, is_windows)) + .await? + .clone(); Ok(SystemInfo { - family, - os: "".to_string(), + family: if is_windows { "windows" } else { "unix" }.to_string(), + os: if is_windows { "windows" } else { "" }.to_string(), arch: "".to_string(), current_dir, main_separator: if is_windows { '\\' } else { '/' }, - - // TODO: We should be able to calculate these once the problem described with SIGPIPE - // is resolved, but for now we will just return empty strings - username: "".to_string(), - shell: "".to_string(), + username, + shell, }) } } diff --git a/distant-ssh2/src/utils.rs b/distant-ssh2/src/utils.rs index 4d6947b..c89dfd5 100644 --- a/distant-ssh2/src/utils.rs +++ b/distant-ssh2/src/utils.rs @@ -4,8 +4,11 @@ use std::{ path::{Path, PathBuf}, time::Duration, }; +use typed_path::{windows::WindowsComponent, Components, WindowsPath, WindowsPathBuf}; use wezterm_ssh::{ExecResult, Session, Sftp}; +const SSH_EXEC_TIMEOUT: Option = Some(Duration::from_secs(1)); + #[allow(dead_code)] const READER_PAUSE_MILLIS: u64 = 100; @@ -35,12 +38,21 @@ impl fmt::Debug for ExecOutput { } } -#[allow(dead_code)] +pub async fn powershell_output( + session: &Session, + cmd: &str, + timeout: impl Into>, +) -> io::Result { + let cmd = format!("powershell.exe -NonInteractive -Command \"& {{{cmd}}}\""); + execute_output(session, &cmd, timeout).await +} + pub async fn execute_output( session: &Session, cmd: &str, - timeout: Option, + timeout: impl Into>, ) -> io::Result { + let timeout = timeout.into(); let ExecResult { mut child, mut stdout, @@ -52,8 +64,14 @@ pub async fn execute_output( .await .map_err(to_other_error)?; + // NOTE: There is a bug where if the ssh backend is libssh, the non-blocking readers + // will never report Ok(0) and are always Err(WouldBlock). So, we want to track + // when a process exits and then cancel the readers if we receive Err(Wouldblock) + let (tx, rx) = tokio::sync::watch::channel(false); + macro_rules! spawn_reader { ($reader:ident) => {{ + let rx = rx.clone(); $reader.set_non_blocking(true).map_err(to_other_error)?; tokio::spawn(async move { use std::io::Read; @@ -64,6 +82,11 @@ pub async fn execute_output( Ok(n) if n > 0 => bytes.extend(&buf[..n]), Ok(_) => break Ok(bytes), Err(x) if x.kind() == io::ErrorKind::WouldBlock => { + // NOTE: This only exists because of the above bug with libssh! + if *rx.borrow() { + break Ok(bytes); + } + tokio::time::sleep(Duration::from_millis(READER_PAUSE_MILLIS)).await; } Err(x) => break Err(x), @@ -80,6 +103,9 @@ pub async fn execute_output( // Wait for process to conclude let status = child.async_wait().compat().await.map_err(to_other_error)?; + // Notify our handles that we are done + let _ = tx.send(true); + // Wait for our handles to conclude (max of timeout if provided) let (stdout, stderr) = match timeout { Some(duration) => { @@ -109,12 +135,12 @@ where io::Error::new(io::ErrorKind::Other, err) } -/// Determines if using windows by checking the canonicalized path of '.' +/// Determines if using windows by checking the OS environment variable pub async fn is_windows(session: &Session) -> io::Result { - let output = execute_output( + let output = powershell_output( session, - "cmd.exe /C echo %OS%", - Some(Duration::from_secs(1)), + "[Environment]::GetEnvironmentVariable('OS')", + SSH_EXEC_TIMEOUT, ) .await?; @@ -136,6 +162,89 @@ pub async fn is_windows(session: &Session) -> io::Result { || contains_subslice(&output.stderr, b"Windows_NT")) } +/// Query remote system for name of current user +pub async fn query_username(session: &Session, is_windows: bool) -> io::Result { + if is_windows { + // Will get DOMAIN\USERNAME as output -- needed because USERNAME isn't set on + // Github's Windows CI (it sets USER instead) + let output = powershell_output( + session, + "[System.Security.Principal.WindowsIdentity]::GetCurrent().Name", + SSH_EXEC_TIMEOUT, + ) + .await?; + + let output = String::from_utf8_lossy(&output.stdout); + let output = match output.split_once('\\') { + Some((_, username)) => username, + None => output.as_ref(), + }; + + Ok(output.trim().to_string()) + } else { + let output = execute_output(session, "/bin/sh -c whoami", SSH_EXEC_TIMEOUT).await?; + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) + } +} + +/// Query remote system for the default shell of current user +pub async fn query_shell(session: &Session, is_windows: bool) -> io::Result { + let output = if is_windows { + powershell_output( + session, + "[Environment]::GetEnvironmentVariable('ComSpec')", + SSH_EXEC_TIMEOUT, + ) + .await? + } else { + execute_output(session, "/bin/sh -c 'echo $SHELL'", SSH_EXEC_TIMEOUT).await? + }; + + Ok(String::from_utf8_lossy(&output.stdout).trim().to_string()) +} + +/// Attempts to convert UTF8 str into a path compliant with Windows +pub fn convert_to_windows_path_string(s: &str) -> Option { + let path = WindowsPath::new(s); + let mut components = path.components(); + + // If we start with a root directory, we may have the weird path + match components.next() { + // Something weird like /C:/... or /C/... that we need to convert to C:\... + Some(WindowsComponent::RootDir) => { + let path = WindowsPath::new(components.as_bytes()); + + // If we have a prefix, then that means we had something like /C:/... + if let Some(WindowsComponent::Prefix(_)) = path.components().next() { + std::str::from_utf8(path.as_bytes()) + .ok() + .map(ToString::to_string) + } else if let Some(WindowsComponent::Normal(filename)) = components.next() { + // If we have a drive letter, convert it into a path, e.g. /C/... -> C:\... + if filename.len() == 1 && (filename[0] as char).is_alphabetic() { + let mut path_buf = WindowsPathBuf::from(format!("{}:", filename[0])); + for component in components { + path_buf.push(component); + } + std::str::from_utf8(path.as_bytes()) + .ok() + .map(ToString::to_string) + } else { + None + } + } else { + None + } + } + + // Already is a Windows path, so just return string + Some(WindowsComponent::Prefix(_)) => Some(s.to_string()), + + // Not a reliable Windows path, so return None + _ => None, + } +} + /// Performs canonicalization of the given path using SFTP pub async fn canonicalize(sftp: &Sftp, path: impl AsRef) -> io::Result { sftp.canonicalize(path.as_ref().to_path_buf()) diff --git a/distant-ssh2/tests/ssh2/client.rs b/distant-ssh2/tests/ssh2/client.rs index 5bad515..042b7c9 100644 --- a/distant-ssh2/tests/ssh2/client.rs +++ b/distant-ssh2/tests/ssh2/client.rs @@ -9,6 +9,9 @@ use predicates::prelude::*; use rstest::*; use std::{io, path::Path, time::Duration}; +const SETUP_DIR_TIMEOUT: Duration = Duration::from_secs(1); +const SETUP_DIR_POLL: Duration = Duration::from_millis(50); + static TEMP_SCRIPT_DIR: Lazy = Lazy::new(|| TempDir::new().unwrap()); static SCRIPT_RUNNER: Lazy = Lazy::new(|| String::from("bash")); @@ -358,7 +361,9 @@ async fn dir_read_should_send_error_if_directory_does_not_exist( // /root/sub1/file2 async fn setup_dir() -> assert_fs::TempDir { let root_dir = assert_fs::TempDir::new().unwrap(); - root_dir.child("file1").touch().unwrap(); + + let file1 = root_dir.child("file1"); + file1.touch().unwrap(); let sub1 = root_dir.child("sub1"); sub1.create_dir_all().unwrap(); @@ -369,11 +374,35 @@ async fn setup_dir() -> assert_fs::TempDir { let link1 = root_dir.child("link1"); link1.symlink_to_file(file2.path()).unwrap(); + // Wait to ensure that everything was set up + tokio::time::timeout(SETUP_DIR_TIMEOUT, async { + macro_rules! all_exist { + () => {{ + let root_dir_exists = root_dir.exists(); + let sub1_exists = sub1.exists(); + let file1_exists = file1.exists(); + let file2_exists = file2.exists(); + let link1_exists = link1.exists(); + + root_dir_exists && sub1_exists && file1_exists && file2_exists && link1_exists + }}; + } + + while !all_exist!() { + tokio::time::sleep(SETUP_DIR_POLL).await; + } + }) + .await + .expect("Failed to setup dir"); + root_dir } +// NOTE: CI fails this on Windows, but it's running Windows with bash and strange paths, so ignore +// it only for the CI #[rstest] #[tokio::test] +#[cfg_attr(all(windows, ci), ignore)] async fn dir_read_should_support_depth_limits(#[future] client: Ctx) { let mut client = client.await; @@ -406,8 +435,11 @@ async fn dir_read_should_support_depth_limits(#[future] client: Ctx) { let mut client = client.await; @@ -1442,7 +1474,19 @@ async fn system_info_should_return_system_info_based_on_binary( let system_info = client.system_info().await.unwrap(); assert_eq!(system_info.family, std::env::consts::FAMILY.to_string()); - assert_eq!(system_info.os, ""); + + // We only support setting the os when the family is windows + if system_info.family == "windows" { + assert_eq!(system_info.os, "windows"); + } else { + assert_eq!(system_info.os, ""); + } + assert_eq!(system_info.arch, ""); assert_eq!(system_info.main_separator, std::path::MAIN_SEPARATOR); + + // We don't have an easy way to tell the remote username and shell in most cases, + // so we just check that they are not empty + assert_ne!(system_info.username, ""); + assert_ne!(system_info.shell, ""); } diff --git a/distant-ssh2/tests/ssh2/launched.rs b/distant-ssh2/tests/ssh2/launched.rs index baeacd3..2679598 100644 --- a/distant-ssh2/tests/ssh2/launched.rs +++ b/distant-ssh2/tests/ssh2/launched.rs @@ -1467,4 +1467,6 @@ async fn system_info_should_return_system_info_based_on_binary( assert_eq!(system_info.os, std::env::consts::OS.to_string()); assert_eq!(system_info.arch, std::env::consts::ARCH.to_string()); assert_eq!(system_info.main_separator, std::path::MAIN_SEPARATOR); + assert_ne!(system_info.username, ""); + assert_ne!(system_info.shell, ""); } diff --git a/distant-ssh2/tests/sshd/mod.rs b/distant-ssh2/tests/sshd/mod.rs index 8639337..61073bf 100644 --- a/distant-ssh2/tests/sshd/mod.rs +++ b/distant-ssh2/tests/sshd/mod.rs @@ -46,6 +46,12 @@ const PORT_RANGE: (u16, u16) = (49152, 65535); static USERNAME: Lazy = Lazy::new(whoami::username); +/// Time to wait after spawning sshd before continuing. Will check if still alive +const WAIT_AFTER_SPAWN: Duration = Duration::from_millis(300); + +/// Maximum times to retry spawning sshd when it fails +const SPAWN_RETRY_CNT: usize = 3; + pub struct SshKeygen; impl SshKeygen { @@ -414,7 +420,10 @@ impl Sshd { Err(x) if port == PORT_RANGE.1 => anyhow::bail!(x), // Otherwise, try next port - Err(_) | Ok(Err(_)) => continue, + Err(_) | Ok(Err(_)) => { + eprintln!("sshd could not spawn on port {port}, so trying next port"); + continue; + } } } } @@ -424,6 +433,13 @@ impl Sshd { config_path: impl AsRef, log_path: impl AsRef, ) -> anyhow::Result, String)>> { + // Sshd doesn't reliably fail when binding to a taken port, so we do a TCP check first + // to try to ensure it is available + drop( + std::net::TcpListener::bind((IpAddr::V4(Ipv4Addr::LOCALHOST), port)) + .with_context(|| format!("Port {port} already taken"))?, + ); + let child = Command::new(BIN_PATH.as_path()) .arg("-D") .arg("-p") @@ -449,6 +465,66 @@ impl Sshd { let result = check(child).context("Sshd encountered problems (after 200ms)")?; Ok(result) } + + /// Checks if still alive + fn check_is_alive(&self) -> bool { + // Check if our sshd process is still running, or if it died and we can report about it + let mut child_lock = self.child.lock().unwrap(); + if let Some(child) = child_lock.take() { + match check(child) { + Ok(Ok(child)) => { + child_lock.replace(child); + true + } + Ok(Err((code, msg))) => { + eprintln!( + "sshd died w/ exit code {}: {msg}", + if let Some(code) = code { + code.to_string() + } else { + "[missing]".to_string() + } + ); + false + } + Err(x) => { + eprintln!("Failed to check status of sshd: {x}"); + false + } + } + } else { + eprintln!("sshd is dead!"); + false + } + } + + fn print_log_file(&self) { + if let Ok(log) = std::fs::read_to_string(&self.log_file) { + eprintln!(); + eprintln!("===================="); + eprintln!("= SSHD LOG FILE "); + eprintln!("===================="); + eprintln!(); + eprintln!("{log}"); + eprintln!(); + eprintln!("===================="); + eprintln!(); + } + } + + fn print_config_file(&self) { + if let Ok(contents) = std::fs::read_to_string(&self.config_file) { + eprintln!(); + eprintln!("===================="); + eprintln!("= SSHD CONFIG FILE "); + eprintln!("===================="); + eprintln!(); + eprintln!("{contents}"); + eprintln!(); + eprintln!("===================="); + eprintln!(); + } + } } impl Drop for Sshd { @@ -500,7 +576,40 @@ impl SshAuthHandler for MockSshAuthHandler { #[fixture] pub fn sshd() -> Sshd { - Sshd::spawn(Default::default()).expect("Failed to spawn sshd") + let mut i = 0; + loop { + if i == SPAWN_RETRY_CNT { + panic!("Exceeded retry count!"); + } + + match Sshd::spawn(Default::default()) { + // Succeeded, so wait a bit, check that is still alive, and then continue + Ok(sshd) => { + std::thread::sleep(WAIT_AFTER_SPAWN); + + if !sshd.check_is_alive() { + // We want to print out the log file from sshd in case it sheds clues on problem + sshd.print_log_file(); + + // We want to print out the config file from sshd in case it sheds clues on problem + sshd.print_config_file(); + + // Skip this spawn and try again + continue; + } + + return sshd; + } + + // Last attempt failed, so panic with the error encountered + Err(x) if i + 1 == SPAWN_RETRY_CNT => panic!("{x}"), + + // Not last attempt, so sleep and then try again + Err(_) => std::thread::sleep(WAIT_AFTER_SPAWN), + } + + i += 1; + } } /// Fixture to establish a client to an SSH server @@ -608,54 +717,16 @@ async fn load_ssh_client(sshd: &Sshd) -> Ssh { } } - // We want to print out the log file from sshd in case it sheds clues on problem - if let Ok(log) = std::fs::read_to_string(&sshd.log_file) { - eprintln!(); - eprintln!("===================="); - eprintln!("= SSHD LOG FILE "); - eprintln!("===================="); - eprintln!(); - eprintln!("{log}"); - eprintln!(); - eprintln!("===================="); - eprintln!(); + // Check if still alive, which will print out messages + if sshd.check_is_alive() { + eprintln!("sshd is still alive, so something else is going on"); } + // We want to print out the log file from sshd in case it sheds clues on problem + sshd.print_log_file(); + // We want to print out the config file from sshd in case it sheds clues on problem - if let Ok(contents) = std::fs::read_to_string(&sshd.config_file) { - eprintln!(); - eprintln!("===================="); - eprintln!("= SSHD CONFIG FILE "); - eprintln!("===================="); - eprintln!(); - eprintln!("{contents}"); - eprintln!(); - eprintln!("===================="); - eprintln!(); - } - - // Check if our sshd process is still running, or if it died and we can report about it - let mut child_lock = sshd.child.lock().unwrap(); - if let Some(child) = child_lock.take() { - match check(child) { - Ok(Ok(child)) => { - eprintln!("sshd is still alive, so something else is going on"); - child_lock.replace(child); - } - Ok(Err((code, msg))) => eprintln!( - "sshd died w/ exit code {}: {msg}", - if let Some(code) = code { - code.to_string() - } else { - "[missing]".to_string() - } - ), - Err(x) => eprintln!("Failed to check status of sshd: {x}"), - } - } else { - eprintln!("sshd is dead!"); - } - drop(child_lock); + sshd.print_config_file(); let error = match errors.into_iter().reduce(|x, y| x.context(y)) { Some(x) => x.context(msg),