feat: Implement SFTP server handler with path traversal prevention (#132) (#151)

* feat: Implement SFTP server handler with path traversal prevention

Implement the SFTP server subsystem using russh-sftp library for file
transfer operations. The handler provides secure file operations with
chroot-like path resolution that prevents clients from accessing files
outside their designated root directory.

* fix: Address critical security vulnerabilities in SFTP server

Fix multiple CRITICAL and HIGH security issues in the SFTP server implementation:

CRITICAL fixes:
- Validate symlink targets in symlink() to prevent path traversal via malicious symlinks
- Validate symlink targets in open() and stat() operations before following them
- Prevent symlinks from pointing outside the root directory

HIGH fixes:
- Redact absolute symlink targets in readlink() that point outside root
- Prevent ".." directory entry from leaking parent directory metadata outside root
- At root boundary, use root's own metadata for ".." instead of actual parent

MEDIUM fixes:
- Add maximum handle limit (1000) to prevent resource exhaustion
- Cap read buffer size to 64KB to prevent memory exhaustion

Security improvements:
- Extract resolve_path_static() helper for reuse in symlink validation
- Use symlink_metadata() instead of metadata() where appropriate
- Add comprehensive validation before creating or following symlinks
- Improve logging for security-related operations

All changes maintain backward compatibility and pass existing tests.

* test: Add comprehensive SFTP handler test coverage

- Add tests for symlink handling in build_longname
- Add tests for edge cases: empty paths, special characters, encoded paths
- Add tests for all SftpError helper methods and conversions
- Add tests for static path resolution method
- Add tests for metadata_to_attrs function
- Update ARCHITECTURE.md with SFTP handler documentation
- Update docs/architecture/README.md with SFTP handler reference
- Fix code formatting issues in sftp.rs

Author: inureyes

ARCHITECTURE.md

@@ -248,6 +248,7 @@ SSH server implementation using the russh library for accepting incoming connect
 - `handler.rs` - `SshHandler` implementing `russh::server::Handler` trait
 - `session.rs` - Session state management (`SessionManager`, `SessionInfo`, `ChannelState`)
 - `exec.rs` - Command execution for SSH exec requests
+- `sftp.rs` - SFTP subsystem handler with path traversal prevention
 - `auth/` - Authentication provider infrastructure
 
 **Key Components**:
@@ -301,6 +302,17 @@ SSH server implementation using the russh library for accepting incoming connect
   - Idle session management
   - Authentication state tracking
 
+- **SftpHandler**: SFTP subsystem handler (`src/server/sftp.rs`)
+  - Implements `russh_sftp::server::Handler` trait for file transfer operations
+  - Path traversal prevention with chroot-like isolation
+  - File operations: open, read, write, close
+  - Directory operations: opendir, readdir, mkdir, rmdir
+  - Attribute operations: stat, lstat, fstat, setstat, fsetstat
+  - Path operations: realpath, rename, remove, readlink, symlink
+  - Symlink validation ensures targets remain within root directory
+  - Handle limit enforcement to prevent resource exhaustion
+  - Read size capping to prevent memory exhaustion
+
 ### Server Authentication Module
 
 The authentication subsystem (`src/server/auth/`) provides extensible authentication for the SSH server:

docs/architecture/README.md

@@ -36,6 +36,7 @@ bssh is a high-performance parallel SSH command execution tool with SSH-compatib
 - **Server CLI (`bssh-server`)** - Server management commands including host key generation, password hashing, config validation (see main ARCHITECTURE.md)
 - **SSH Server Module** - SSH server implementation using russh (see main ARCHITECTURE.md)
 - **Server Authentication** - Authentication providers including public key verification (see main ARCHITECTURE.md)
+- **SFTP Handler** - SFTP subsystem with path traversal prevention and chroot-like isolation (see main ARCHITECTURE.md)
 
 ## Navigation
 

src/server/handler.rs

@@ -31,6 +31,7 @@ use super::auth::AuthProvider;
 use super::config::ServerConfig;
 use super::exec::CommandExecutor;
 use super::session::{ChannelState, PtyConfig, SessionId, SessionInfo, SessionManager};
+use super::sftp::SftpHandler;
 use crate::shared::rate_limit::RateLimiter;
 
 /// SSH handler for a single client connection.
@@ -186,11 +187,13 @@ impl russh::server::Handler for SshHandler {
         let channel_id = channel.id();
         tracing::debug!(
             peer = ?self.peer_addr,
+            channel = ?channel_id,
             "Channel opened for session"
         );
 
+        // Store the channel itself so we can use it for subsystems like SFTP
         self.channels
-            .insert(channel_id, ChannelState::new(channel_id));
+            .insert(channel_id, ChannelState::with_channel(channel));
         async { Ok(true) }
     }
 
@@ -626,7 +629,8 @@ impl russh::server::Handler for SshHandler {
 
     /// Handle subsystem request.
     ///
-    /// Placeholder implementation - will be implemented in a future issue.
+    /// Handles SFTP subsystem requests by creating an SftpHandler and running
+    /// the SFTP server on the channel stream.
     fn subsystem_request(
         &mut self,
         channel_id: ChannelId,
@@ -635,19 +639,106 @@ impl russh::server::Handler for SshHandler {
     ) -> impl std::future::Future<Output = Result<(), Self::Error>> + Send {
         tracing::debug!(
             subsystem = %name,
+            channel = ?channel_id,
+            peer = ?self.peer_addr,
             "Subsystem request"
         );
 
+        // Handle SFTP subsystem
         if name == "sftp" {
-            if let Some(channel_state) = self.channels.get_mut(&channel_id) {
-                channel_state.set_sftp();
+            // Check if SFTP is enabled (default: enabled)
+            // In future, this should check config.sftp.enabled
+
+            // Get the channel from our stored channels
+            let channel = self.channels.get_mut(&channel_id).and_then(|state| {
+                state.set_sftp();
+                state.take_channel()
+            });
+
+            let channel = match channel {
+                Some(ch) => ch,
+                None => {
+                    tracing::warn!(
+                        channel = ?channel_id,
+                        "SFTP request but channel not found or already taken"
+                    );
+                    let _ = session.channel_failure(channel_id);
+                    return async { Ok(()) }.boxed();
+                }
+            };
+
+            // Get authenticated user info
+            let username = match self.session_info.as_ref().and_then(|s| s.user.clone()) {
+                Some(user) => user,
+                None => {
+                    tracing::warn!(
+                        channel = ?channel_id,
+                        "SFTP request without authenticated user"
+                    );
+                    let _ = session.channel_failure(channel_id);
+                    return async { Ok(()) }.boxed();
+                }
+            };
+
+            // Clone what we need for the async block
+            let auth_provider = Arc::clone(&self.auth_provider);
+            let peer_addr = self.peer_addr;
+
+            // Signal success before spawning the SFTP handler
+            let _ = session.channel_success(channel_id);
+
+            return async move {
+                // Get user info from auth provider
+                let user_info = match auth_provider.get_user_info(&username).await {
+                    Ok(Some(info)) => info,
+                    Ok(None) => {
+                        tracing::error!(
+                            user = %username,
+                            "User not found after authentication for SFTP"
+                        );
+                        return Ok(());
+                    }
+                    Err(e) => {
+                        tracing::error!(
+                            user = %username,
+                            error = %e,
+                            "Failed to get user info for SFTP"
+                        );
+                        return Ok(());
+                    }
+                };
+
+                tracing::info!(
+                    user = %username,
+                    peer = ?peer_addr,
+                    home = %user_info.home_dir.display(),
+                    "Starting SFTP session"
+                );
+
+                // Create SFTP handler with user's home directory as root
+                let sftp_handler = SftpHandler::new(user_info.clone(), Some(user_info.home_dir));
+
+                // Run SFTP server on the channel stream
+                russh_sftp::server::run(channel.into_stream(), sftp_handler).await;
+
+                tracing::info!(
+                    user = %username,
+                    peer = ?peer_addr,
+                    "SFTP session ended"
+                );
+
+                Ok(())
             }
+            .boxed();
         }
 
-        // Placeholder - reject for now
-        // Will be implemented in #132 for SFTP
+        // Unknown subsystem - reject
+        tracing::debug!(
+            subsystem = %name,
+            "Unknown subsystem, rejecting"
+        );
         let _ = session.channel_failure(channel_id);
-        async { Ok(()) }
+        async { Ok(()) }.boxed()
     }
 
     /// Handle incoming data from the client.

src/server/mod.rs

@@ -49,6 +49,7 @@ pub mod config;
 pub mod exec;
 pub mod handler;
 pub mod session;
+pub mod sftp;
 
 use std::net::SocketAddr;
 use std::path::Path;

src/server/session.rs

@@ -30,7 +30,8 @@ use std::net::SocketAddr;
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::time::Instant;
 
-use russh::ChannelId;
+use russh::server::Msg;
+use russh::{Channel, ChannelId};
 
 /// Unique identifier for an SSH session.
 ///
@@ -184,11 +185,13 @@ impl PtyConfig {
 /// State of an SSH channel.
 ///
 /// Tracks the current mode and configuration of a channel.
-#[derive(Debug)]
 pub struct ChannelState {
     /// The channel ID.
     pub channel_id: ChannelId,
 
+    /// The underlying channel for subsystem communication.
+    channel: Option<Channel<Msg>>,
+
     /// Current operation mode.
     pub mode: ChannelMode,
 
@@ -199,17 +202,46 @@ pub struct ChannelState {
     pub eof_received: bool,
 }
 
+impl std::fmt::Debug for ChannelState {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("ChannelState")
+            .field("channel_id", &self.channel_id)
+            .field("has_channel", &self.channel.is_some())
+            .field("mode", &self.mode)
+            .field("pty", &self.pty)
+            .field("eof_received", &self.eof_received)
+            .finish()
+    }
+}
+
 impl ChannelState {
     /// Create a new channel state.
     pub fn new(channel_id: ChannelId) -> Self {
         Self {
             channel_id,
+            channel: None,
             mode: ChannelMode::Idle,
             pty: None,
             eof_received: false,
         }
     }
 
+    /// Create a new channel state with the underlying channel.
+    pub fn with_channel(channel: Channel<Msg>) -> Self {
+        Self {
+            channel_id: channel.id(),
+            channel: Some(channel),
+            mode: ChannelMode::Idle,
+            pty: None,
+            eof_received: false,
+        }
+    }
+
+    /// Take the underlying channel (consumes it for use with subsystems).
+    pub fn take_channel(&mut self) -> Option<Channel<Msg>> {
+        self.channel.take()
+    }
+
     /// Check if the channel has a PTY attached.
     pub fn has_pty(&self) -> bool {
         self.pty.is_some()