Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<root>/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 `<root>/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.
Expand Down
15 changes: 14 additions & 1 deletion docs/man/bssh-server.8
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
30 changes: 30 additions & 0 deletions src/server/config/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -251,6 +252,17 @@ fn apply_env_overrides(mut config: ServerFileConfig) -> Result<ServerFileConfig>
);
}

// 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;
Expand Down Expand Up @@ -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() {
Expand Down
43 changes: 43 additions & 0 deletions src/server/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// <https://github.com/lablup/bssh/issues/215>. 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 {
Expand Down Expand Up @@ -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,
}
}
}
Expand Down Expand Up @@ -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
/// <https://github.com/lablup/bssh/issues/215>).
pub fn compression(mut self, enabled: bool) -> Self {
self.config.compression = enabled;
self
}

/// Build the ServerConfig.
pub fn build(self) -> ServerConfig {
self.config
Expand Down Expand Up @@ -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,
}
}
}
Expand Down Expand Up @@ -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();
Expand Down
41 changes: 41 additions & 0 deletions src/server/config/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// <https://github.com/lablup/bssh/issues/215>). 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.
Expand Down Expand Up @@ -633,6 +652,7 @@ impl Default for ServerSettings {
max_connections: default_max_connections(),
timeout: default_timeout(),
keepalive_interval: default_keepalive(),
compression: false,
}
}
}
Expand Down Expand Up @@ -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]
Expand Down
75 changes: 62 additions & 13 deletions src/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand Down