From b03eb14f91f96f7462b0719001dfdc95afee1bae Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 17 Jul 2026 16:00:23 +0900 Subject: [PATCH] feat(server): make SSH compression configurable instead of hard-disabled The #215 workaround made build_russh_config advertise only `none` compression unconditionally, removing operator choice for deployments that may want compression once the underlying russh delayed-zlib bug is fixed. Add a `compression` option to the server configuration (YAML `server.compression`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) that threads through ServerFileConfig::into_server_config into ServerConfig and controls whether build_russh_config advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless an operator opts in, and enabling it logs a startup warning because russh's delayed-zlib transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it (Cyberduck, `sftp -C`) mid-session. Both settings are covered by unit tests (the existing #215 regression test now also asserts the off default; a new test asserts the opt-in advertisement includes zlib@openssh.com), env override parsing is tested for valid and invalid values, and the option plus the desync caveat are documented in the bssh-server man page, the config type docs, and the changelog. Refs #220 --- CHANGELOG.md | 3 ++ docs/man/bssh-server.8 | 15 +++++++- src/server/config/loader.rs | 30 +++++++++++++++ src/server/config/mod.rs | 43 +++++++++++++++++++++ src/server/config/types.rs | 41 ++++++++++++++++++++ src/server/mod.rs | 75 ++++++++++++++++++++++++++++++------- 6 files changed, 193 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a808b35..4979c15c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- **Make server-side SSH compression configurable instead of hard-disabled** (#220). The #215 workaround forced the server to advertise only `none` compression for every deployment, with no way for operators to opt back in. A new `server.compression` option (YAML `server.compression: true`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) now controls whether `build_russh_config` advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless explicitly enabled, and enabling it logs a warning: russh's delayed-zlib (`zlib@openssh.com`) transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it mid-session. Both settings are covered by unit tests, and the option plus the desync caveat are documented in the man page and config docs. + ### 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. diff --git a/docs/man/bssh-server.8 b/docs/man/bssh-server.8 index 74734376..be428727 100644 --- a/docs/man/bssh-server.8 +++ b/docs/man/bssh-server.8 @@ -146,6 +146,10 @@ Maximum concurrent connections (default: 100) .B BSSH_KEEPALIVE_INTERVAL Keepalive interval in seconds (default: 60) .TP +.B BSSH_COMPRESSION +Advertise SSH transport compression, "true" or "false" (default: false). +See the \fBserver\fR configuration section for the caveat about enabling it. +.TP .B BSSH_AUTH_METHODS Comma-separated authentication methods (publickey,password) .TP @@ -164,7 +168,16 @@ Command timeout in seconds (default: 3600) .SS Configuration Sections .TP .B server -Network and connection settings (bind_address, port, host_keys, max_connections, timeout, keepalive_interval) +Network and connection settings (bind_address, port, host_keys, +max_connections, timeout, keepalive_interval, compression). +\fBcompression\fR controls whether the server advertises SSH transport +compression (\fBzlib\fR, \fBzlib@openssh.com\fR). It defaults to \fBfalse\fR, +advertising only \fBnone\fR so clients use the uncompressed transport, +because the bundled russh library's delayed-zlib (\fBzlib@openssh.com\fR) +transport currently desyncs a few packets after compression activates, +dropping clients that negotiate it (Cyberduck, \fBsftp -C\fR) mid-session; +see https://github.com/lablup/bssh/issues/215. Enable it only if the +upstream russh bug is fixed or your clients never negotiate compression. .TP .B auth Authentication methods and settings (methods, publickey, password) diff --git a/src/server/config/loader.rs b/src/server/config/loader.rs index 7481d0fb..4a3f99f2 100644 --- a/src/server/config/loader.rs +++ b/src/server/config/loader.rs @@ -55,6 +55,7 @@ use std::path::{Path, PathBuf}; /// - `BSSH_HOST_KEY` - Comma-separated host key paths /// - `BSSH_MAX_CONNECTIONS` - Maximum concurrent connections /// - `BSSH_KEEPALIVE_INTERVAL` - Keepalive interval in seconds +/// - `BSSH_COMPRESSION` - Advertise SSH transport compression ("true"/"false"; default false, see issue #215) /// - `BSSH_AUTH_METHODS` - Comma-separated auth methods (e.g., "publickey,password") /// - `BSSH_AUTHORIZED_KEYS_DIR` - Directory for authorized_keys files /// - `BSSH_AUTHORIZED_KEYS_PATTERN` - Pattern for authorized_keys paths @@ -251,6 +252,17 @@ fn apply_env_overrides(mut config: ServerFileConfig) -> Result ); } + // BSSH_COMPRESSION + if let Ok(compression_str) = std::env::var("BSSH_COMPRESSION") { + config.server.compression = compression_str.parse().context(format!( + "Invalid BSSH_COMPRESSION value (expected \"true\" or \"false\"): {compression_str}" + ))?; + tracing::debug!( + compression = config.server.compression, + "Applied BSSH_COMPRESSION override" + ); + } + // BSSH_AUTH_METHODS (comma-separated: "publickey,password") if let Ok(methods_str) = std::env::var("BSSH_AUTH_METHODS") { use super::types::AuthMethod; @@ -471,6 +483,24 @@ auth: assert_eq!(config.server.host_keys[0], PathBuf::from("/key1")); } + #[test] + #[serial_test::serial] + fn test_env_override_compression() { + let _port = EnvGuard::remove("BSSH_PORT"); + let _compression = EnvGuard::set("BSSH_COMPRESSION", "true"); + let config = apply_env_overrides(ServerFileConfig::default()).unwrap(); + assert!(config.server.compression); + } + + #[test] + #[serial_test::serial] + fn test_env_override_compression_invalid() { + let _port = EnvGuard::remove("BSSH_PORT"); + let _compression = EnvGuard::set("BSSH_COMPRESSION", "maybe"); + let result = apply_env_overrides(ServerFileConfig::default()); + assert!(result.is_err()); + } + #[test] #[serial_test::serial] fn test_env_override_auth_methods() { diff --git a/src/server/config/mod.rs b/src/server/config/mod.rs index e140a020..df6eed04 100644 --- a/src/server/config/mod.rs +++ b/src/server/config/mod.rs @@ -217,6 +217,19 @@ pub struct ServerConfig { /// Default: 0 (disabled) #[serde(default)] pub session_timeout_secs: u64, + + /// Advertise SSH transport compression (`zlib`, `zlib@openssh.com`). + /// + /// When `false` (default), the server advertises only `none` so clients + /// fall back to the uncompressed transport. This is the safe default: + /// russh's delayed-zlib (`zlib@openssh.com`) transport desyncs a few + /// packets after compression activates post-auth, dropping clients that + /// negotiate it (Cyberduck, `sftp -C`) mid-session. See + /// . Enable only if the + /// upstream russh bug is fixed or your clients never negotiate + /// compression. + #[serde(default)] + pub compression: bool, } fn default_max_sessions_per_user() -> usize { @@ -323,6 +336,7 @@ impl Default for ServerConfig { blocked_ips: Vec::new(), max_sessions_per_user: default_max_sessions_per_user(), session_timeout_secs: 0, + compression: false, } } } @@ -618,6 +632,17 @@ impl ServerConfigBuilder { self } + /// Enable or disable SSH transport compression negotiation. + /// + /// Disabled by default: russh's delayed-zlib (`zlib@openssh.com`) + /// transport desyncs mid-session, so the server advertises only `none` + /// unless this is explicitly enabled (see + /// ). + pub fn compression(mut self, enabled: bool) -> Self { + self.config.compression = enabled; + self + } + /// Build the ServerConfig. pub fn build(self) -> ServerConfig { self.config @@ -688,6 +713,7 @@ impl ServerFileConfig { blocked_ips: self.security.blocked_ips, max_sessions_per_user: self.security.max_sessions_per_user, session_timeout_secs: self.security.session_timeout, + compression: self.server.compression, } } } @@ -790,6 +816,23 @@ mod tests { assert!(server_config.scp_root.is_none()); } + #[test] + fn compression_defaults_to_off_and_threads_into_server_config() { + // #220: compression must default to off (preserving the #215 + // workaround) and the file-config `server.compression` flag must + // propagate into ServerConfig. + let mut file_config = ServerFileConfig::default(); + file_config.server.host_keys = vec![PathBuf::from("/test/key")]; + assert!(!file_config.server.compression); + + let server_config = file_config.clone().into_server_config(); + assert!(!server_config.compression); + + file_config.server.compression = true; + let server_config = file_config.into_server_config(); + assert!(server_config.compression); + } + #[test] fn test_config_new() { let config = ServerConfig::new(); diff --git a/src/server/config/types.rs b/src/server/config/types.rs index b08d4694..a66827d1 100644 --- a/src/server/config/types.rs +++ b/src/server/config/types.rs @@ -121,6 +121,25 @@ pub struct ServerSettings { /// Default: 60 (1 minute) #[serde(default = "default_keepalive")] pub keepalive_interval: u64, + + /// Advertise SSH transport compression (`zlib`, `zlib@openssh.com`). + /// + /// When `false` (default), the server advertises only `none`, so every + /// client falls back to the uncompressed transport. This matches the + /// Dropbear / OpenSSH `sftp-server` defaults used in Backend.AI + /// containers. + /// + /// **Caveat:** russh's delayed-zlib (`zlib@openssh.com`) transport + /// currently desyncs a few packets after compression activates post-auth, + /// dropping clients that negotiate it (Cyberduck, `sftp -C`) mid-session + /// (reproduced on russh 0.61.1 and 0.62.1; see + /// ). Enable this only if you + /// have verified the underlying russh bug is fixed or your clients never + /// negotiate compression. + /// + /// Default: false + #[serde(default)] + pub compression: bool, } /// Authentication configuration. @@ -633,6 +652,7 @@ impl Default for ServerSettings { max_connections: default_max_connections(), timeout: default_timeout(), keepalive_interval: default_keepalive(), + compression: false, } } } @@ -705,6 +725,27 @@ mod tests { assert!(config.scp.enabled); assert!(!config.filter.enabled); assert!(!config.audit.enabled); + assert!(!config.server.compression); + } + + #[test] + fn test_yaml_parsing_compression() { + // #220: `server.compression` is parseable from YAML and defaults to + // false when omitted. + let yaml = r#" +server: + port: 2222 + compression: true +"#; + let config: ServerFileConfig = serde_yaml::from_str(yaml).unwrap(); + assert!(config.server.compression); + + let yaml = r#" +server: + port: 2222 +"#; + let config: ServerFileConfig = serde_yaml::from_str(yaml).unwrap(); + assert!(!config.server.compression); } #[test] diff --git a/src/server/mod.rs b/src/server/mod.rs index e9c3119f..a4b5197c 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -195,16 +195,27 @@ impl BsshServer { // russh's delayed zlib (`zlib@openssh.com`) compression desyncs and // corrupts the channel stream after a few packets, so any client that - // negotiates compression — Cyberduck, `sftp -C` — fails mid-session + // negotiates compression (Cyberduck, `sftp -C`) fails mid-session // with "SshEncoding: length invalid" (reproducible in russh 0.61.1 and - // 0.62.1). Until the upstream russh bug is fixed, advertise only "none" - // so clients fall back to the uncompressed transport, matching the - // Dropbear/OpenSSH sftp-server defaults used in Backend.AI containers. - // See https://github.com/lablup/bssh/issues/215. - const NO_COMPRESSION: &[russh::compression::Name] = &[russh::compression::NONE]; - let preferred = russh::Preferred { - compression: std::borrow::Cow::Borrowed(NO_COMPRESSION), - ..russh::Preferred::DEFAULT + // 0.62.1). By default, advertise only "none" so clients fall back to + // the uncompressed transport, matching the Dropbear/OpenSSH + // sftp-server defaults used in Backend.AI containers. Operators can + // opt back in via the `compression` config option once the upstream + // russh bug is fixed. See https://github.com/lablup/bssh/issues/215 + // and https://github.com/lablup/bssh/issues/220. + let preferred = if self.config.compression { + tracing::warn!( + "SSH transport compression enabled; russh's delayed-zlib \ + (zlib@openssh.com) desync may drop clients that negotiate \ + compression mid-session (see issue #215)" + ); + russh::Preferred::DEFAULT + } else { + const NO_COMPRESSION: &[russh::compression::Name] = &[russh::compression::NONE]; + russh::Preferred { + compression: std::borrow::Cow::Borrowed(NO_COMPRESSION), + ..russh::Preferred::DEFAULT + } }; Ok(russh::server::Config { @@ -435,15 +446,20 @@ mod tests { #[test] fn test_build_russh_config_advertises_only_none_compression() { - // Regression guard for #215: the server must advertise only `none` - // compression so clients that prefer `zlib@openssh.com` (Cyberduck, - // `sftp -C`) fall back to the uncompressed transport instead of - // hitting russh's delayed-zlib desync. + // Regression guard for #215: by default the server must advertise + // only `none` compression so clients that prefer `zlib@openssh.com` + // (Cyberduck, `sftp -C`) fall back to the uncompressed transport + // instead of hitting russh's delayed-zlib desync. The default config + // leaves `compression` off, so this covers the disabled setting. let key = concat!( env!("CARGO_MANIFEST_DIR"), "/test_keys/ssh_host_ed25519_key" ); let config = ServerConfig::builder().host_key(key).build(); + assert!( + !config.compression, + "compression must default to off (see #215/#220)" + ); let server = BsshServer::new(config); let russh_config = server @@ -456,6 +472,39 @@ mod tests { ); } + #[test] + fn test_build_russh_config_compression_opt_in_advertises_zlib() { + // #220: opting in via the `compression` config option restores the + // russh default advertisement (none, zlib, zlib@openssh.com) so + // clients may negotiate a compressed transport. + let key = concat!( + env!("CARGO_MANIFEST_DIR"), + "/test_keys/ssh_host_ed25519_key" + ); + let config = ServerConfig::builder() + .host_key(key) + .compression(true) + .build(); + let server = BsshServer::new(config); + + let russh_config = server + .build_russh_config() + .expect("config should build with a valid host key"); + assert_eq!( + russh_config.preferred.compression, + russh::Preferred::DEFAULT.compression, + "opt-in must advertise russh's default compression list" + ); + assert!( + russh_config + .preferred + .compression + .as_ref() + .contains(&russh::compression::ZLIB_LEGACY), + "opt-in advertisement must include zlib@openssh.com" + ); + } + #[tokio::test] async fn test_session_count() { let config = ServerConfig::builder().host_key("/nonexistent/key").build();