diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a808b35..37ee592f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Make `sftp.root`/`scp.root` chroot usable by re-anchoring client paths under the chroot root** (#214). With a chroot configured, directory listing worked but every path resolution failed: `cd subdir`, `get file`, and even `get` of a file sitting directly at the chroot root were rejected as `path outside root` or `not found`, so only bare `/` and `readdir` functioned. Under chroot the client's coordinate space is rooted at `/` (this is what `realpath` reports and what the client sends back), but `resolve_chroot` treated a client absolute path such as `/subdir` as a *host* path and rejected anything not starting with the host root, so `/subdir` (meaning `/subdir`) never matched. A prior change had introduced this to avoid "path doubling", but real clients never send the host path; they send chroot-relative absolute paths, so the guard broke the normal case. The SFTP and SCP resolvers now both interpret absolute and relative client paths relative to `root` (OpenSSH `ChrootDirectory` re-rooting), ignoring a leading `/`, dropping `.`, and clamping `..` so traversal still cannot escape. Containment is verified: `..` stays pinned at the root and a client `/etc/passwd` maps to `/etc/passwd` inside the jail, never the host file. SCP previously kept the old "reject absolute outside root" behavior, so it is unified here to match `sftp.root`. Verified end-to-end with the OpenSSH `sftp` client (`cd`/`get`/root-level files/Unicode names all work; escape attempts stay confined) and covered by updated unit and integration tests. - **Advertise only `none` SSH compression so clients that negotiate `zlib@openssh.com` no longer drop mid-session** (#215). Cyberduck, OpenSSH `sftp -C`, and any client that prefers delayed zlib completed the SFTP handshake (`INIT` / `REALPATH` / `STAT` all succeeded) and then the connection died, with the server logging `SshEncoding: length invalid` on the next inbound packet. The root cause is russh's delayed-zlib (`zlib@openssh.com`) transport: the flate2 stream desyncs a few packets after compression activates post-auth, so russh decodes the following packet's length prefix out of corrupted plaintext. It reproduces on both russh 0.61.1 and 0.62.1, which rules out the channel-close path and pins it to compression rather than the SFTP layer (OpenSSH's default `sftp`, FileZilla, and paramiko all negotiate `none` and were unaffected). `build_russh_config` now sets `russh::Preferred { compression: Cow::Borrowed(&[compression::NONE]), ..DEFAULT }`, so the server advertises only `none` and every client falls back to the uncompressed transport, matching the Dropbear / OpenSSH `sftp-server` defaults used in Backend.AI kernel containers, where compression was never in play. Verified with `sftp -C` (now negotiates `compression: none` and lists/downloads cleanly) and a live Cyberduck 9.5 session. The underlying russh delayed-zlib desync is left to be fixed upstream. +- **Wire the ssh_config `Compression` directive into the russh client instead of silently ignoring it** (#219). `Compression yes`/`no` was parsed and resolved (`SshHostConfig::compression`) but `to_russh_config` never read it, so it had no effect on the actual connection regardless of the setting. `SshConnectionConfig` gains a `compression` field and `with_compression` builder, and `to_russh_config` now sets `russh::Preferred.compression` from it: `false` (the default, matching `Compression no`/unset) advertises only `none`; `true` (`Compression yes`) advertises eager `zlib` ahead of `none`. `zlib@openssh.com` is deliberately never advertised even when compression is enabled, because the delayed-zlib desync fixed server-side in #215 lives in russh's codec and would equally corrupt a bssh client session. `build_ssh_connection_config` in the dispatcher now resolves `Compression` from ssh_config via the new `SshConfig::get_compression` and feeds it through, so the setting reaches every connection path that shares `SshConnectionConfig` (direct connections, jump-host chains, interactive sessions). ### Security - **Confine absolute SFTP symlink targets to the chroot** (#214). Once `resolve_chroot` stopped rejecting out-of-root absolute paths, the SFTP `symlink` handler's containment guard became a no-op, so a chrooted client could create a link whose on-disk target was an absolute *host* path (for example `symlink /link /etc/passwd`). Because bssh uses a virtual chroot with no `chroot(2)`, that link resolved to the real host filesystem. Absolute symlink targets are now re-anchored under the chroot before the link is written, so a created link can never point outside the jail; relative targets keep OpenSSH-compatible verbatim storage. diff --git a/docs/architecture/ssh-config-parser.md b/docs/architecture/ssh-config-parser.md index 4a63e64f..b652e306 100644 --- a/docs/architecture/ssh-config-parser.md +++ b/docs/architecture/ssh-config-parser.md @@ -207,6 +207,7 @@ bssh supports 40+ SSH configuration directives organized into categories: - `ConnectTimeout` - Connection timeout in seconds - `ServerAliveInterval` - Keepalive interval - `ServerAliveCountMax` - Keepalive retry count +- `Compression` - Advertise transport compression to the server (yes/no, default: no). `yes` offers eager `zlib` ahead of `none`; `no`/unset offers only `none`. `zlib@openssh.com` is never advertised regardless of this setting, because russh's delayed-zlib codec desyncs the flate2 stream a few packets after activation (see the server-side fix in #215); wiring is in `SshConnectionConfig::to_russh_config` (`src/ssh/tokio_client/connection.rs`). **Authentication Options:** - `IdentityFile` - SSH private key file (multiple allowed) diff --git a/src/app/dispatcher.rs b/src/app/dispatcher.rs index 4eee8938..8cfe3d4e 100644 --- a/src/app/dispatcher.rs +++ b/src/app/dispatcher.rs @@ -38,8 +38,9 @@ use super::initialization::determine_use_keychain; use super::initialization::{AppContext, determine_ssh_key_path}; use super::utils::format_duration; -/// Build SSH connection config with keepalive settings. -/// Precedence: CLI > SSH config > YAML config > defaults +/// Build SSH connection config with keepalive and compression settings. +/// Precedence: CLI > SSH config > YAML config > defaults. +/// `Compression` has no CLI or YAML override; it is read from ssh_config only. fn build_ssh_connection_config( cli: &Cli, ctx: &AppContext, @@ -66,18 +67,25 @@ fn build_ssh_connection_config( .or_else(|| ctx.config.get_server_alive_count_max(cluster_name)) .unwrap_or(DEFAULT_KEEPALIVE_MAX); + let compression = ctx + .ssh_config + .get_compression(hostname.unwrap_or("*")) + .unwrap_or(false); + let ssh_connection_config = SshConnectionConfig::new() .with_keepalive_interval(if keepalive_interval == 0 { None } else { Some(keepalive_interval) }) - .with_keepalive_max(keepalive_max); + .with_keepalive_max(keepalive_max) + .with_compression(compression); tracing::debug!( - "SSH keepalive config: interval={:?}s, max={}", + "SSH keepalive config: interval={:?}s, max={}, compression={}", ssh_connection_config.keepalive_interval, - ssh_connection_config.keepalive_max + ssh_connection_config.keepalive_max, + ssh_connection_config.compression ); ssh_connection_config diff --git a/src/ssh/ssh_config/mod.rs b/src/ssh/ssh_config/mod.rs index 779ce032..e7f9e45e 100644 --- a/src/ssh/ssh_config/mod.rs +++ b/src/ssh/ssh_config/mod.rs @@ -143,6 +143,14 @@ impl SshConfig { resolver::get_proxy_jump(&self.hosts, hostname) } + /// Get the effective `Compression` value (`yes`/`no`) for a hostname. + /// + /// Returns `None` when the directive is unset for the resolved host, + /// which callers should treat the same as `Compression no`. + pub fn get_compression(&self, hostname: &str) -> Option { + resolver::get_compression(&self.hosts, hostname) + } + /// Get an integer option value for a hostname. /// /// Currently supports: @@ -864,4 +872,24 @@ Host test let config = SshConfig::parse(config_content).unwrap(); assert_eq!(config.hosts[0].number_of_password_prompts, Some(3)); } + + #[test] + fn test_get_compression_option() { + // Issue #219: the resolved `Compression` directive must be readable + // through the public API so callers can wire it into the russh + // client config (see SshConnectionConfig::to_russh_config). + let config_content = r#" +Host compressed.example.com + Compression yes + +Host plain.example.com + Compression no +"#; + let config = SshConfig::parse(config_content).unwrap(); + + assert_eq!(config.get_compression("compressed.example.com"), Some(true)); + assert_eq!(config.get_compression("plain.example.com"), Some(false)); + // No matching Host block and no directive at all: unset. + assert_eq!(config.get_compression("unconfigured.example.com"), None); + } } diff --git a/src/ssh/ssh_config/resolver.rs b/src/ssh/ssh_config/resolver.rs index 22456b46..6b64d2bf 100644 --- a/src/ssh/ssh_config/resolver.rs +++ b/src/ssh/ssh_config/resolver.rs @@ -428,6 +428,12 @@ pub(super) fn get_strict_host_key_checking( config.strict_host_key_checking } +/// Get the effective Compression value +pub(super) fn get_compression(hosts: &[SshHostConfig], hostname: &str) -> Option { + let config = find_host_config(hosts, hostname); + config.compression +} + /// Get ProxyJump configuration pub(super) fn get_proxy_jump(hosts: &[SshHostConfig], hostname: &str) -> Option { let config = find_host_config(hosts, hostname); diff --git a/src/ssh/tokio_client/connection.rs b/src/ssh/tokio_client/connection.rs index e2283aa7..900e9439 100644 --- a/src/ssh/tokio_client/connection.rs +++ b/src/ssh/tokio_client/connection.rs @@ -72,6 +72,15 @@ pub struct SshConnectionConfig { /// before considering the connection dead. /// Default: 3 pub keepalive_max: usize, + + /// Whether the ssh_config `Compression` directive resolved to `yes`. + /// + /// `false` (the default) matches `Compression no`/unset and advertises + /// only `none` compression to the server. `true` matches `Compression + /// yes` and advertises eager `zlib` ahead of `none`. See + /// [`to_russh_config`](Self::to_russh_config) for why `zlib@openssh.com` + /// is never advertised regardless of this flag. + pub compression: bool, } impl Default for SshConnectionConfig { @@ -79,10 +88,31 @@ impl Default for SshConnectionConfig { Self { keepalive_interval: Some(DEFAULT_KEEPALIVE_INTERVAL), keepalive_max: DEFAULT_KEEPALIVE_MAX, + compression: false, } } } +/// Compression preference advertised when ssh_config resolves `Compression +/// no` (or the directive is unset). This matches the current effective +/// behavior and the server-side fix in #215. +const NONE_ONLY_COMPRESSION: &[russh::compression::Name] = &[russh::compression::NONE]; + +/// Compression preference advertised when ssh_config resolves `Compression +/// yes`. +/// +/// This deliberately omits `russh::compression::ZLIB_LEGACY` +/// (`zlib@openssh.com`). #215 found that russh's delayed-zlib transport +/// desyncs the flate2 stream a few packets after compression activates +/// post-auth, corrupting the next packet's length prefix (reproducible on +/// russh 0.61.1 and 0.62.1). That bug lives in russh's compression codec, not +/// the server role, so a bssh client that advertised `zlib@openssh.com` would +/// be just as exposed when talking to a server that selects it. Eager `zlib` +/// (activated immediately after key exchange, not delayed) does not exhibit +/// the same desync and is offered ahead of `none`. +const COMPRESSED_ORDER: &[russh::compression::Name] = + &[russh::compression::ZLIB, russh::compression::NONE]; + impl SshConnectionConfig { /// Create a new configuration with default values. pub fn new() -> Self { @@ -104,6 +134,15 @@ impl SshConnectionConfig { self } + /// Set whether SSH transport compression should be advertised, mirroring + /// the ssh_config `Compression` directive (`yes` -> `true`, `no`/unset -> + /// `false`). + #[must_use] + pub fn with_compression(mut self, enabled: bool) -> Self { + self.compression = enabled; + self + } + /// Convert this configuration to a russh client Config. /// /// `inactivity_timeout` stays disabled for client sessions. A healthy @@ -111,11 +150,27 @@ impl SshConnectionConfig { /// long time, so inactivity must not be treated as a local reason to close /// it. When keepalive is enabled, russh's keepalive counter is the liveness /// detector; when it is disabled, the client leaves idle sessions alone. + /// + /// `preferred.compression` is derived from `self.compression`: `false` + /// advertises only `none`; `true` advertises `zlib` ahead of `none`. + /// `zlib@openssh.com` is never advertised; see [`COMPRESSED_ORDER`] for + /// why. pub fn to_russh_config(&self) -> Config { + let compression_order = if self.compression { + COMPRESSED_ORDER + } else { + NONE_ONLY_COMPRESSION + }; + let preferred = russh::Preferred { + compression: std::borrow::Cow::Borrowed(compression_order), + ..russh::Preferred::DEFAULT + }; + Config { keepalive_interval: self.keepalive_interval.map(Duration::from_secs), keepalive_max: self.keepalive_max, inactivity_timeout: None, + preferred, ..Default::default() } } diff --git a/src/ssh/tokio_client/connection_tests.rs b/src/ssh/tokio_client/connection_tests.rs new file mode 100644 index 00000000..f044b47a --- /dev/null +++ b/src/ssh/tokio_client/connection_tests.rs @@ -0,0 +1,99 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Tests for [`super::connection::SshConnectionConfig`]'s compression mapping. +//! +//! Regression coverage for #219: the ssh_config `Compression` directive was +//! parsed and resolved but never consumed when building the russh client +//! config, so `Compression yes`/`no` had no effect on the actual connection. + +use super::connection::SshConnectionConfig; + +#[test] +fn test_default_compression_advertises_none_only() { + // `Compression` unset (and the struct default) must match the current + // effective behavior: only `none` is advertised. + let config = SshConnectionConfig::default(); + assert!(!config.compression); + + let russh_config = config.to_russh_config(); + assert_eq!( + russh_config.preferred.compression.as_ref(), + [russh::compression::NONE], + "Compression no/unset must advertise only `none`" + ); +} + +#[test] +fn test_compression_no_advertises_none_only() { + let config = SshConnectionConfig::new().with_compression(false); + + let russh_config = config.to_russh_config(); + assert_eq!( + russh_config.preferred.compression.as_ref(), + [russh::compression::NONE], + "Compression no must advertise only `none`" + ); +} + +#[test] +fn test_compression_yes_advertises_zlib_then_none() { + let config = SshConnectionConfig::new().with_compression(true); + assert!(config.compression); + + let russh_config = config.to_russh_config(); + assert_eq!( + russh_config.preferred.compression.as_ref(), + [russh::compression::ZLIB, russh::compression::NONE], + "Compression yes must advertise zlib ahead of none" + ); +} + +#[test] +fn test_compression_yes_never_advertises_delayed_zlib() { + // Regression guard tied to #215: russh's delayed-zlib (`zlib@openssh.com`) + // transport desyncs the flate2 stream a few packets after compression + // activates post-auth. That bug lives in russh's codec, so it applies to + // bssh acting as a client just as much as it did to bssh acting as a + // server. `Compression yes` must never cause the client to advertise + // `zlib@openssh.com`, even though eager `zlib` is offered. + let config = SshConnectionConfig::new().with_compression(true); + let russh_config = config.to_russh_config(); + + assert!( + !russh_config + .preferred + .compression + .contains(&russh::compression::ZLIB_LEGACY), + "Compression yes must never advertise zlib@openssh.com (see #215)" + ); +} + +#[test] +fn test_with_compression_is_chainable_with_keepalive_settings() { + let config = SshConnectionConfig::new() + .with_keepalive_interval(Some(15)) + .with_keepalive_max(5) + .with_compression(true); + + assert_eq!(config.keepalive_interval, Some(15)); + assert_eq!(config.keepalive_max, 5); + assert!(config.compression); + + let russh_config = config.to_russh_config(); + assert_eq!( + russh_config.preferred.compression.as_ref(), + [russh::compression::ZLIB, russh::compression::NONE] + ); +} diff --git a/src/ssh/tokio_client/mod.rs b/src/ssh/tokio_client/mod.rs index 832da4f3..971b0f9e 100644 --- a/src/ssh/tokio_client/mod.rs +++ b/src/ssh/tokio_client/mod.rs @@ -17,6 +17,8 @@ pub mod authentication; pub mod channel_manager; pub mod connection; +#[cfg(test)] +mod connection_tests; pub mod error; pub mod file_transfer; mod to_socket_addrs_with_hostname;