From 5b97757d2fd332c434ca98c659a7aad5d6de6d5f Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Jan 2026 16:57:07 +0900 Subject: [PATCH 1/3] feat: Implement public key authentication for server Add authentication provider infrastructure for bssh-server: - Create AuthProvider trait for extensible auth backends - Implement PublicKeyVerifier with OpenSSH authorized_keys parsing - Support both directory and pattern-based authorized_keys locations - Integrate auth provider with SSH handler for auth_publickey - Add rate limiting for authentication attempts - Include comprehensive security features: - Username validation to prevent path traversal - File permission checks on Unix systems - Logging for auth attempts (success/failure) Configuration supports two modes: - Directory mode: {dir}/{username}/authorized_keys - Pattern mode: /home/{user}/.ssh/authorized_keys Closes #126 --- src/server/auth/mod.rs | 56 +++ src/server/auth/provider.rs | 251 +++++++++++++ src/server/auth/publickey.rs | 662 +++++++++++++++++++++++++++++++++++ src/server/config.rs | 99 ++++++ src/server/handler.rs | 224 +++++++++++- src/server/mod.rs | 2 + 6 files changed, 1281 insertions(+), 13 deletions(-) create mode 100644 src/server/auth/mod.rs create mode 100644 src/server/auth/provider.rs create mode 100644 src/server/auth/publickey.rs diff --git a/src/server/auth/mod.rs b/src/server/auth/mod.rs new file mode 100644 index 00000000..84d9845f --- /dev/null +++ b/src/server/auth/mod.rs @@ -0,0 +1,56 @@ +// 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. + +//! Authentication provider infrastructure for bssh-server. +//! +//! This module provides the authentication framework for the SSH server, +//! including traits for authentication providers and implementations for +//! public key authentication. +//! +//! # Architecture +//! +//! The authentication system is designed around the [`AuthProvider`] trait, +//! which allows for extensible authentication methods. Currently supported: +//! +//! - **Public Key Authentication**: Via [`PublicKeyVerifier`] +//! +//! # Security Features +//! +//! - Username validation to prevent path traversal attacks +//! - Rate limiting integration +//! - Logging of authentication attempts (success/failure) +//! - Timing attack mitigation where possible +//! +//! # Usage +//! +//! ```no_run +//! use bssh::server::auth::{AuthProvider, PublicKeyVerifier, PublicKeyAuthConfig}; +//! use std::path::PathBuf; +//! +//! // Create a public key verifier +//! let config = PublicKeyAuthConfig::with_directory("/etc/bssh/authorized_keys"); +//! let verifier = PublicKeyVerifier::new(config); +//! +//! // Use with SSH handler +//! // verifier.verify("username", &public_key).await +//! ``` + +pub mod provider; +pub mod publickey; + +pub use provider::AuthProvider; +pub use publickey::{AuthKeyOptions, AuthorizedKey, PublicKeyAuthConfig, PublicKeyVerifier}; + +// Re-export shared auth types for convenience +pub use crate::shared::auth_types::{AuthResult, UserInfo}; diff --git a/src/server/auth/provider.rs b/src/server/auth/provider.rs new file mode 100644 index 00000000..ddf3a85e --- /dev/null +++ b/src/server/auth/provider.rs @@ -0,0 +1,251 @@ +// 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. + +//! Authentication provider trait and implementations. +//! +//! This module defines the core [`AuthProvider`] trait that all authentication +//! backends must implement. The trait is designed to be: +//! +//! - **Async**: All methods are async for I/O operations +//! - **Extensible**: New auth methods can be added +//! - **Thread-safe**: Implements `Send + Sync` +//! +//! # Implementing AuthProvider +//! +//! To create a custom authentication provider: +//! +//! ```ignore +//! use async_trait::async_trait; +//! use bssh::server::auth::{AuthProvider, AuthResult, UserInfo}; +//! use russh::keys::ssh_key::PublicKey; +//! use anyhow::Result; +//! +//! struct MyAuthProvider; +//! +//! #[async_trait] +//! impl AuthProvider for MyAuthProvider { +//! async fn verify_publickey(&self, username: &str, key: &PublicKey) -> Result { +//! // Custom implementation +//! Ok(AuthResult::Reject) +//! } +//! +//! async fn verify_password(&self, username: &str, password: &str) -> Result { +//! // Custom implementation +//! Ok(AuthResult::Reject) +//! } +//! +//! async fn get_user_info(&self, username: &str) -> Result> { +//! Ok(None) +//! } +//! +//! async fn user_exists(&self, username: &str) -> Result { +//! Ok(false) +//! } +//! } +//! ``` + +use anyhow::Result; +use async_trait::async_trait; +use russh::keys::ssh_key::PublicKey; + +use crate::shared::auth_types::{AuthResult, UserInfo}; + +/// Trait for authentication providers. +/// +/// This trait defines the interface that all authentication backends must +/// implement. It supports multiple authentication methods as defined by +/// the SSH protocol (RFC 4252). +/// +/// # Thread Safety +/// +/// All implementations must be `Send + Sync` to allow concurrent use +/// across multiple SSH connections. +/// +/// # Error Handling +/// +/// Methods return `Result` rather than just `AuthResult` to +/// allow for I/O errors and other failures to be distinguished from +/// authentication rejections. +#[async_trait] +pub trait AuthProvider: Send + Sync { + /// Verify public key authentication. + /// + /// This method is called when a client attempts to authenticate using + /// a public key. The server should verify that: + /// + /// 1. The username is valid + /// 2. The public key is authorized for the user + /// + /// # Arguments + /// + /// * `username` - The username attempting to authenticate + /// * `key` - The public key presented by the client + /// + /// # Returns + /// + /// - `Ok(AuthResult::Accept)` if the key is authorized + /// - `Ok(AuthResult::Reject)` if the key is not authorized + /// - `Ok(AuthResult::Partial { .. })` for multi-factor auth scenarios + /// - `Err(...)` if an error occurred during verification + /// + /// # Security + /// + /// Implementations should: + /// - Validate the username to prevent path traversal + /// - Use constant-time comparison where possible + /// - Log authentication attempts + async fn verify_publickey(&self, username: &str, key: &PublicKey) -> Result; + + /// Verify password authentication. + /// + /// This method is called when a client attempts to authenticate using + /// a password. Password authentication is generally less secure than + /// public key authentication and may be disabled in the server config. + /// + /// # Arguments + /// + /// * `username` - The username attempting to authenticate + /// * `password` - The password presented by the client + /// + /// # Returns + /// + /// - `Ok(AuthResult::Accept)` if the password is correct + /// - `Ok(AuthResult::Reject)` if the password is incorrect + /// - `Ok(AuthResult::Partial { .. })` for multi-factor auth scenarios + /// - `Err(...)` if an error occurred during verification + /// + /// # Security + /// + /// Implementations should: + /// - Use constant-time string comparison + /// - Never log the actual password + /// - Consider rate limiting + async fn verify_password(&self, username: &str, password: &str) -> Result; + + /// Get user information after successful authentication. + /// + /// This method retrieves information about an authenticated user, + /// which is used to set up the session environment. + /// + /// # Arguments + /// + /// * `username` - The authenticated username + /// + /// # Returns + /// + /// - `Ok(Some(UserInfo))` if the user exists + /// - `Ok(None)` if the user does not exist + /// - `Err(...)` if an error occurred + async fn get_user_info(&self, username: &str) -> Result>; + + /// Check if a user exists. + /// + /// This method checks whether a user account exists, without performing + /// authentication. It's useful for early rejection of invalid usernames. + /// + /// # Arguments + /// + /// * `username` - The username to check + /// + /// # Returns + /// + /// - `Ok(true)` if the user exists + /// - `Ok(false)` if the user does not exist + /// - `Err(...)` if an error occurred + /// + /// # Security + /// + /// Be careful not to leak user enumeration information. Consider always + /// returning `Ok(true)` and letting authentication fail naturally, or + /// using timing attack mitigation. + async fn user_exists(&self, username: &str) -> Result; +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A test auth provider that always rejects + struct RejectAllProvider; + + #[async_trait] + impl AuthProvider for RejectAllProvider { + async fn verify_publickey(&self, _username: &str, _key: &PublicKey) -> Result { + Ok(AuthResult::Reject) + } + + async fn verify_password(&self, _username: &str, _password: &str) -> Result { + Ok(AuthResult::Reject) + } + + async fn get_user_info(&self, _username: &str) -> Result> { + Ok(None) + } + + async fn user_exists(&self, _username: &str) -> Result { + Ok(false) + } + } + + /// A test auth provider that always accepts + struct AcceptAllProvider; + + #[async_trait] + impl AuthProvider for AcceptAllProvider { + async fn verify_publickey(&self, _username: &str, _key: &PublicKey) -> Result { + Ok(AuthResult::Accept) + } + + async fn verify_password(&self, _username: &str, _password: &str) -> Result { + Ok(AuthResult::Accept) + } + + async fn get_user_info(&self, username: &str) -> Result> { + Ok(Some(UserInfo::new(username))) + } + + async fn user_exists(&self, _username: &str) -> Result { + Ok(true) + } + } + + #[tokio::test] + async fn test_reject_all_provider() { + let provider = RejectAllProvider; + + let result = provider.verify_password("test", "pass").await.unwrap(); + assert!(result.is_rejected()); + + let exists = provider.user_exists("test").await.unwrap(); + assert!(!exists); + + let info = provider.get_user_info("test").await.unwrap(); + assert!(info.is_none()); + } + + #[tokio::test] + async fn test_accept_all_provider() { + let provider = AcceptAllProvider; + + let result = provider.verify_password("test", "pass").await.unwrap(); + assert!(result.is_accepted()); + + let exists = provider.user_exists("test").await.unwrap(); + assert!(exists); + + let info = provider.get_user_info("test").await.unwrap(); + assert!(info.is_some()); + assert_eq!(info.unwrap().username, "test"); + } +} diff --git a/src/server/auth/publickey.rs b/src/server/auth/publickey.rs new file mode 100644 index 00000000..609af69b --- /dev/null +++ b/src/server/auth/publickey.rs @@ -0,0 +1,662 @@ +// 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. + +//! Public key authentication verifier. +//! +//! This module provides the [`PublicKeyVerifier`] which implements public key +//! authentication by loading and parsing authorized_keys files. +//! +//! # OpenSSH authorized_keys Format +//! +//! The verifier supports the standard OpenSSH authorized_keys format: +//! +//! ```text +//! # Comment line +//! ssh-ed25519 AAAAC3NzaC1lZDI1NTE5... user@host +//! ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQ... another@host +//! ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTIt... third@host +//! ``` +//! +//! # Security +//! +//! The verifier includes security features: +//! +//! - Username validation to prevent path traversal +//! - Constant-time key comparison where possible +//! - Comprehensive logging of auth attempts +//! +//! # Configuration +//! +//! Two modes are supported: +//! +//! 1. **Directory mode**: `{dir}/{username}/authorized_keys` +//! 2. **Pattern mode**: `/home/{user}/.ssh/authorized_keys` + +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use async_trait::async_trait; +use russh::keys::ssh_key::PublicKey; + +use super::provider::AuthProvider; +use crate::shared::auth_types::{AuthResult, UserInfo}; +use crate::shared::validation::validate_username; + +/// Configuration for public key authentication. +/// +/// Specifies where to find authorized_keys files for users. +#[derive(Debug, Clone)] +pub struct PublicKeyAuthConfig { + /// Directory containing authorized_keys files. + /// Structure: `{dir}/{username}/authorized_keys` + pub authorized_keys_dir: Option, + + /// Alternative: file path pattern with `{user}` placeholder. + /// Example: `/home/{user}/.ssh/authorized_keys` + pub authorized_keys_pattern: Option, +} + +impl PublicKeyAuthConfig { + /// Create a new configuration with a directory path. + /// + /// The directory should contain subdirectories for each user, + /// with an `authorized_keys` file in each. + /// + /// # Arguments + /// + /// * `dir` - Path to the directory (e.g., `/etc/bssh/authorized_keys/`) + /// + /// # Example + /// + /// ``` + /// use bssh::server::auth::PublicKeyAuthConfig; + /// + /// let config = PublicKeyAuthConfig::with_directory("/etc/bssh/authorized_keys"); + /// ``` + pub fn with_directory(dir: impl Into) -> Self { + Self { + authorized_keys_dir: Some(dir.into()), + authorized_keys_pattern: None, + } + } + + /// Create a new configuration with a file pattern. + /// + /// The pattern should contain `{user}` which will be replaced + /// with the username. + /// + /// # Arguments + /// + /// * `pattern` - Path pattern (e.g., `/home/{user}/.ssh/authorized_keys`) + /// + /// # Example + /// + /// ``` + /// use bssh::server::auth::PublicKeyAuthConfig; + /// + /// let config = PublicKeyAuthConfig::with_pattern("/home/{user}/.ssh/authorized_keys"); + /// ``` + pub fn with_pattern(pattern: impl Into) -> Self { + Self { + authorized_keys_dir: None, + authorized_keys_pattern: Some(pattern.into()), + } + } + + /// Get the authorized_keys file path for a user. + /// + /// # Arguments + /// + /// * `username` - The validated username + /// + /// # Returns + /// + /// The path to the user's authorized_keys file. + fn get_authorized_keys_path(&self, username: &str) -> PathBuf { + if let Some(ref pattern) = self.authorized_keys_pattern { + PathBuf::from(pattern.replace("{user}", username)) + } else if let Some(ref dir) = self.authorized_keys_dir { + dir.join(username).join("authorized_keys") + } else { + // Default to home directory pattern + PathBuf::from(format!("/home/{username}/.ssh/authorized_keys")) + } + } +} + +impl Default for PublicKeyAuthConfig { + fn default() -> Self { + Self { + authorized_keys_dir: None, + authorized_keys_pattern: Some("/home/{user}/.ssh/authorized_keys".to_string()), + } + } +} + +/// Options parsed from authorized_keys file entries. +/// +/// These options follow the OpenSSH authorized_keys format and can +/// restrict what the key can be used for. +#[derive(Debug, Clone, Default)] +pub struct AuthKeyOptions { + /// Force a specific command to be executed + pub command: Option, + + /// Environment variables to set + pub environment: Vec, + + /// Restrict connections to specific source addresses + pub from: Vec, + + /// Disable PTY allocation + pub no_pty: bool, + + /// Disable port forwarding + pub no_port_forwarding: bool, + + /// Disable agent forwarding + pub no_agent_forwarding: bool, + + /// Disable X11 forwarding + pub no_x11_forwarding: bool, +} + +/// A parsed authorized key entry. +#[derive(Debug)] +pub struct AuthorizedKey { + /// The public key + pub key: PublicKey, + + /// Optional comment (usually user@host) + pub comment: Option, + + /// Key options from the authorized_keys file + pub options: AuthKeyOptions, +} + +/// Public key authentication verifier. +/// +/// Verifies public keys against authorized_keys files in the OpenSSH format. +/// +/// # Example +/// +/// ```no_run +/// use bssh::server::auth::{PublicKeyVerifier, PublicKeyAuthConfig, AuthProvider}; +/// +/// # async fn example() -> anyhow::Result<()> { +/// let config = PublicKeyAuthConfig::with_directory("/etc/bssh/authorized_keys"); +/// let verifier = PublicKeyVerifier::new(config); +/// +/// // Check if a user exists +/// let exists = verifier.user_exists("testuser").await?; +/// # Ok(()) +/// # } +/// ``` +pub struct PublicKeyVerifier { + config: PublicKeyAuthConfig, +} + +impl PublicKeyVerifier { + /// Create a new public key verifier. + /// + /// # Arguments + /// + /// * `config` - Configuration specifying authorized_keys file locations + pub fn new(config: PublicKeyAuthConfig) -> Self { + Self { config } + } + + /// Verify if a public key is authorized for a user. + /// + /// # Arguments + /// + /// * `username` - The username to check (will be validated) + /// * `key` - The public key to verify + /// + /// # Returns + /// + /// `Ok(true)` if the key is authorized, `Ok(false)` otherwise. + /// + /// # Errors + /// + /// Returns an error if username validation fails. + pub async fn verify(&self, username: &str, key: &PublicKey) -> Result { + // Validate username to prevent path traversal + let username = validate_username(username).context("Invalid username")?; + + // Load authorized keys for user + let authorized_keys = self.load_authorized_keys(&username).await?; + + // Check if key matches any authorized key + for authorized_key in &authorized_keys { + if self.keys_match(key, authorized_key) { + tracing::info!( + user = %username, + key_type = %key.algorithm(), + "Public key authentication successful" + ); + return Ok(true); + } + } + + tracing::debug!( + user = %username, + key_type = %key.algorithm(), + authorized_keys_count = %authorized_keys.len(), + "No matching authorized key found" + ); + Ok(false) + } + + /// Load authorized keys for a user. + /// + /// # Arguments + /// + /// * `username` - The validated username + /// + /// # Returns + /// + /// A vector of parsed authorized keys. Returns empty vector if the + /// authorized_keys file doesn't exist. + async fn load_authorized_keys(&self, username: &str) -> Result> { + let path = self.config.get_authorized_keys_path(username); + + if !path.exists() { + tracing::debug!( + user = %username, + path = %path.display(), + "No authorized_keys file found" + ); + return Ok(Vec::new()); + } + + // Check file permissions (should be readable but not world-writable) + #[cfg(unix)] + self.check_file_permissions(&path)?; + + let content = tokio::fs::read_to_string(&path) + .await + .with_context(|| format!("Failed to read authorized_keys file: {}", path.display()))?; + + self.parse_authorized_keys(&content) + } + + /// Check that file permissions are secure (Unix only). + #[cfg(unix)] + fn check_file_permissions(&self, path: &Path) -> Result<()> { + use std::os::unix::fs::MetadataExt; + + let metadata = std::fs::metadata(path) + .with_context(|| format!("Failed to get metadata for {}", path.display()))?; + + let mode = metadata.mode(); + + // Check if file is world-writable (security risk) + if mode & 0o002 != 0 { + anyhow::bail!( + "authorized_keys file {} is world-writable (mode {:o})", + path.display(), + mode & 0o777 + ); + } + + Ok(()) + } + + /// Parse authorized_keys file content. + /// + /// # Arguments + /// + /// * `content` - The file content as a string + /// + /// # Returns + /// + /// A vector of successfully parsed authorized keys. Invalid lines + /// are logged and skipped. + fn parse_authorized_keys(&self, content: &str) -> Result> { + let mut keys = Vec::new(); + + for (line_num, line) in content.lines().enumerate() { + let line = line.trim(); + + // Skip empty lines and comments + if line.is_empty() || line.starts_with('#') { + continue; + } + + match self.parse_authorized_key_line(line) { + Ok(key) => keys.push(key), + Err(e) => { + tracing::warn!( + line = %line_num + 1, + error = %e, + "Failed to parse authorized_keys line" + ); + } + } + } + + Ok(keys) + } + + /// Parse a single authorized_keys line. + /// + /// Format: `[options] key-type base64-key [comment]` + /// + /// # Arguments + /// + /// * `line` - A single line from the authorized_keys file + fn parse_authorized_key_line(&self, line: &str) -> Result { + let parts: Vec<&str> = line.split_whitespace().collect(); + + if parts.is_empty() { + anyhow::bail!("Empty line"); + } + + // Try to determine if first part is options or key type + let (options, key_type_idx) = if is_key_type(parts[0]) { + (AuthKeyOptions::default(), 0) + } else { + // First part is options, parse them + let opts = parse_key_options(parts[0])?; + (opts, 1) + }; + + if parts.len() <= key_type_idx + 1 { + anyhow::bail!("Missing key data"); + } + + let key_type = parts[key_type_idx]; + let key_data = parts[key_type_idx + 1]; + + // Get comment if present + let comment = if parts.len() > key_type_idx + 2 { + Some(parts[key_type_idx + 2..].join(" ")) + } else { + None + }; + + // Parse the public key + let key_str = format!("{key_type} {key_data}"); + let key = parse_public_key(&key_str).with_context(|| { + format!("Failed to parse public key of type {key_type}") + })?; + + Ok(AuthorizedKey { + key, + comment, + options, + }) + } + + /// Check if two public keys match. + /// + /// Uses the key's algorithm and encoded data for comparison. + fn keys_match(&self, client_key: &PublicKey, authorized: &AuthorizedKey) -> bool { + // Compare using the public key's encoded form for consistent comparison + client_key == &authorized.key + } +} + +#[async_trait] +impl AuthProvider for PublicKeyVerifier { + async fn verify_publickey(&self, username: &str, key: &PublicKey) -> Result { + match self.verify(username, key).await { + Ok(true) => Ok(AuthResult::Accept), + Ok(false) => Ok(AuthResult::Reject), + Err(e) => { + tracing::error!( + user = %username, + error = %e, + "Error during public key verification" + ); + Ok(AuthResult::Reject) + } + } + } + + async fn verify_password(&self, _username: &str, _password: &str) -> Result { + // Public key verifier doesn't handle password auth + Ok(AuthResult::Reject) + } + + async fn get_user_info(&self, username: &str) -> Result> { + // Validate username first + let username = validate_username(username).context("Invalid username")?; + + // Check if user has an authorized_keys file + let path = self.config.get_authorized_keys_path(&username); + if path.exists() { + Ok(Some(UserInfo::new(username))) + } else { + Ok(None) + } + } + + async fn user_exists(&self, username: &str) -> Result { + // Validate username first + let username = match validate_username(username) { + Ok(u) => u, + Err(_) => return Ok(false), + }; + + // Check if authorized_keys file exists + let path = self.config.get_authorized_keys_path(&username); + Ok(path.exists()) + } +} + +/// Check if a string looks like a key type. +fn is_key_type(s: &str) -> bool { + matches!( + s, + "ssh-rsa" + | "ssh-dss" + | "ssh-ed25519" + | "ssh-ed448" + | "ecdsa-sha2-nistp256" + | "ecdsa-sha2-nistp384" + | "ecdsa-sha2-nistp521" + | "sk-ssh-ed25519@openssh.com" + | "sk-ecdsa-sha2-nistp256@openssh.com" + ) +} + +/// Parse key options from authorized_keys format. +fn parse_key_options(options_str: &str) -> Result { + let mut options = AuthKeyOptions::default(); + + // Options are comma-separated + for option in options_str.split(',') { + let option = option.trim(); + + if option.is_empty() { + continue; + } + + if let Some((key, value)) = option.split_once('=') { + // Option with value + let value = value.trim_matches('"'); + match key { + "command" => options.command = Some(value.to_string()), + "environment" => options.environment.push(value.to_string()), + "from" => { + for addr in value.split(',') { + options.from.push(addr.trim().to_string()); + } + } + _ => { + tracing::debug!(option = %key, "Unknown authorized_keys option"); + } + } + } else { + // Boolean option + match option { + "no-pty" => options.no_pty = true, + "no-port-forwarding" => options.no_port_forwarding = true, + "no-agent-forwarding" => options.no_agent_forwarding = true, + "no-X11-forwarding" => options.no_x11_forwarding = true, + _ => { + tracing::debug!(option = %option, "Unknown authorized_keys option"); + } + } + } + } + + Ok(options) +} + +/// Parse a public key from OpenSSH format string. +fn parse_public_key(key_str: &str) -> Result { + let parts: Vec<&str> = key_str.split_whitespace().collect(); + if parts.len() < 2 { + anyhow::bail!("Invalid key format: expected 'type base64data'"); + } + + let key_data = parts[1]; + + // Decode base64 and parse + russh::keys::parse_public_key_base64(key_data) + .map_err(|e| anyhow::anyhow!("Failed to parse public key: {e}")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_is_key_type() { + assert!(is_key_type("ssh-ed25519")); + assert!(is_key_type("ssh-rsa")); + assert!(is_key_type("ecdsa-sha2-nistp256")); + assert!(!is_key_type("no-pty")); + assert!(!is_key_type("command=\"/bin/date\"")); + } + + #[test] + fn test_parse_key_options_empty() { + let options = parse_key_options("").unwrap(); + assert!(options.command.is_none()); + assert!(!options.no_pty); + } + + #[test] + fn test_parse_key_options_no_pty() { + let options = parse_key_options("no-pty").unwrap(); + assert!(options.no_pty); + } + + #[test] + fn test_parse_key_options_multiple() { + let options = parse_key_options("no-pty,no-port-forwarding").unwrap(); + assert!(options.no_pty); + assert!(options.no_port_forwarding); + } + + #[test] + fn test_parse_key_options_command() { + let options = parse_key_options("command=\"/bin/date\"").unwrap(); + assert_eq!(options.command, Some("/bin/date".to_string())); + } + + #[test] + fn test_config_with_directory() { + let config = PublicKeyAuthConfig::with_directory("/etc/bssh/keys"); + let path = config.get_authorized_keys_path("testuser"); + assert_eq!(path, PathBuf::from("/etc/bssh/keys/testuser/authorized_keys")); + } + + #[test] + fn test_config_with_pattern() { + let config = PublicKeyAuthConfig::with_pattern("/home/{user}/.ssh/authorized_keys"); + let path = config.get_authorized_keys_path("testuser"); + assert_eq!(path, PathBuf::from("/home/testuser/.ssh/authorized_keys")); + } + + #[test] + fn test_config_default() { + let config = PublicKeyAuthConfig::default(); + let path = config.get_authorized_keys_path("testuser"); + assert_eq!(path, PathBuf::from("/home/testuser/.ssh/authorized_keys")); + } + + #[test] + fn test_parse_authorized_keys_comments() { + let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); + let content = "# This is a comment\n\n# Another comment\n"; + let keys = verifier.parse_authorized_keys(content).unwrap(); + assert!(keys.is_empty()); + } + + #[test] + fn test_parse_authorized_key_line_ed25519() { + let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); + + // Valid ed25519 key + let line = + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl test@example"; + let result = verifier.parse_authorized_key_line(line); + assert!(result.is_ok()); + let key = result.unwrap(); + assert_eq!(key.comment, Some("test@example".to_string())); + } + + #[test] + fn test_parse_authorized_key_line_with_options() { + let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); + + // Key with options + let line = "no-pty ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl test@example"; + let result = verifier.parse_authorized_key_line(line); + assert!(result.is_ok()); + let key = result.unwrap(); + assert!(key.options.no_pty); + } + + #[test] + fn test_parse_authorized_key_line_invalid() { + let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); + + // Invalid key data + let line = "ssh-ed25519 notbase64!@#$"; + let result = verifier.parse_authorized_key_line(line); + assert!(result.is_err()); + } + + #[tokio::test] + async fn test_user_exists_invalid_username() { + let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); + + // Path traversal attempt should return false + let exists = verifier.user_exists("../etc/passwd").await.unwrap(); + assert!(!exists); + + // Empty username + let exists = verifier.user_exists("").await.unwrap(); + assert!(!exists); + } + + #[tokio::test] + async fn test_verify_invalid_username() { + let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); + + // Create a dummy key for testing + let key_str = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl"; + let key = parse_public_key(key_str).unwrap(); + + // Path traversal attempt should fail + let result = verifier.verify("../etc/passwd", &key).await; + assert!(result.is_err()); + } +} diff --git a/src/server/config.rs b/src/server/config.rs index 7036347e..62776181 100644 --- a/src/server/config.rs +++ b/src/server/config.rs @@ -17,10 +17,13 @@ //! This module defines configuration options for the SSH server. use std::path::PathBuf; +use std::sync::Arc; use std::time::Duration; use serde::{Deserialize, Serialize}; +use super::auth::{AuthProvider, PublicKeyAuthConfig, PublicKeyVerifier}; + /// Configuration for the SSH server. /// /// Contains all settings needed to initialize and run the SSH server. @@ -65,6 +68,37 @@ pub struct ServerConfig { /// Banner message displayed to clients before authentication. #[serde(default)] pub banner: Option, + + /// Configuration for public key authentication. + #[serde(default)] + pub publickey_auth: PublicKeyAuthConfigSerde, +} + +/// Serializable configuration for public key authentication. +/// +/// This is a separate type from `PublicKeyAuthConfig` to support +/// serde serialization while keeping the actual config flexible. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct PublicKeyAuthConfigSerde { + /// Directory containing authorized_keys files. + /// Structure: `{dir}/{username}/authorized_keys` + pub authorized_keys_dir: Option, + + /// Alternative: file path pattern with `{user}` placeholder. + /// Example: `/home/{user}/.ssh/authorized_keys` + pub authorized_keys_pattern: Option, +} + +impl From for PublicKeyAuthConfig { + fn from(serde_config: PublicKeyAuthConfigSerde) -> Self { + if let Some(dir) = serde_config.authorized_keys_dir { + PublicKeyAuthConfig::with_directory(dir) + } else if let Some(pattern) = serde_config.authorized_keys_pattern { + PublicKeyAuthConfig::with_pattern(pattern) + } else { + PublicKeyAuthConfig::default() + } + } } fn default_listen_address() -> String { @@ -104,6 +138,7 @@ impl Default for ServerConfig { allow_publickey_auth: true, allow_keyboard_interactive: false, banner: None, + publickey_auth: PublicKeyAuthConfigSerde::default(), } } } @@ -144,6 +179,14 @@ impl ServerConfig { pub fn add_host_key(&mut self, path: impl Into) { self.host_keys.push(path.into()); } + + /// Create an auth provider based on the configuration. + /// + /// Returns a `PublicKeyVerifier` configured according to the server settings. + pub fn create_auth_provider(&self) -> Arc { + let config: PublicKeyAuthConfig = self.publickey_auth.clone().into(); + Arc::new(PublicKeyVerifier::new(config)) + } } /// Builder for constructing ServerConfig. @@ -219,6 +262,26 @@ impl ServerConfigBuilder { self } + /// Set the authorized_keys directory. + /// + /// The directory should contain subdirectories for each user, + /// with an `authorized_keys` file in each. + pub fn authorized_keys_dir(mut self, dir: impl Into) -> Self { + self.config.publickey_auth.authorized_keys_dir = Some(dir.into()); + self.config.publickey_auth.authorized_keys_pattern = None; + self + } + + /// Set the authorized_keys file pattern. + /// + /// The pattern should contain `{user}` which will be replaced + /// with the username. + pub fn authorized_keys_pattern(mut self, pattern: impl Into) -> Self { + self.config.publickey_auth.authorized_keys_pattern = Some(pattern.into()); + self.config.publickey_auth.authorized_keys_dir = None; + self + } + /// Build the ServerConfig. pub fn build(self) -> ServerConfig { self.config @@ -314,4 +377,40 @@ mod tests { assert_eq!(config.idle_timeout_secs, 600); assert_eq!(config.idle_timeout(), Some(Duration::from_secs(600))); } + + #[test] + fn test_builder_authorized_keys_dir() { + let config = ServerConfig::builder() + .authorized_keys_dir("/etc/bssh/authorized_keys") + .build(); + + assert_eq!( + config.publickey_auth.authorized_keys_dir, + Some(PathBuf::from("/etc/bssh/authorized_keys")) + ); + assert!(config.publickey_auth.authorized_keys_pattern.is_none()); + } + + #[test] + fn test_builder_authorized_keys_pattern() { + let config = ServerConfig::builder() + .authorized_keys_pattern("/home/{user}/.ssh/authorized_keys") + .build(); + + assert!(config.publickey_auth.authorized_keys_dir.is_none()); + assert_eq!( + config.publickey_auth.authorized_keys_pattern, + Some("/home/{user}/.ssh/authorized_keys".to_string()) + ); + } + + #[test] + fn test_create_auth_provider() { + let config = ServerConfig::builder() + .authorized_keys_dir("/etc/bssh/keys") + .build(); + + // Provider should be created successfully (verifies no panic) + let _provider = config.create_auth_provider(); + } } diff --git a/src/server/handler.rs b/src/server/handler.rs index 0d8a05f2..af1a88c2 100644 --- a/src/server/handler.rs +++ b/src/server/handler.rs @@ -26,8 +26,10 @@ use russh::server::{Auth, Msg, Session}; use russh::{Channel, ChannelId, MethodKind, MethodSet, Pty}; use tokio::sync::RwLock; +use super::auth::AuthProvider; use super::config::ServerConfig; use super::session::{ChannelState, PtyConfig, SessionId, SessionInfo, SessionManager}; +use crate::shared::rate_limit::RateLimiter; /// SSH handler for a single client connection. /// @@ -43,6 +45,12 @@ pub struct SshHandler { /// Shared session manager. sessions: Arc>, + /// Authentication provider for verifying credentials. + auth_provider: Arc, + + /// Rate limiter for authentication attempts. + rate_limiter: RateLimiter, + /// Session information for this connection. session_info: Option, @@ -57,10 +65,38 @@ impl SshHandler { config: Arc, sessions: Arc>, ) -> Self { + let auth_provider = config.create_auth_provider(); + // Rate limiter: allow burst of 5 attempts, refill 1 attempt per second + let rate_limiter = RateLimiter::with_simple_config(5, 1.0); + + Self { + peer_addr, + config, + sessions, + auth_provider, + rate_limiter, + session_info: None, + channels: HashMap::new(), + } + } + + /// Create a new SSH handler with a custom auth provider. + /// + /// This is useful for testing or when you need a different auth provider. + pub fn with_auth_provider( + peer_addr: Option, + config: Arc, + sessions: Arc>, + auth_provider: Arc, + ) -> Self { + let rate_limiter = RateLimiter::with_simple_config(5, 1.0); + Self { peer_addr, config, sessions, + auth_provider, + rate_limiter, session_info: None, channels: HashMap::new(), } @@ -189,7 +225,7 @@ impl russh::server::Handler for SshHandler { /// Handle public key authentication. /// - /// Placeholder implementation - will be implemented in a future issue. + /// Verifies the public key against the user's authorized_keys file. fn auth_publickey( &mut self, user: &str, @@ -212,27 +248,115 @@ impl russh::server::Handler for SshHandler { let mut methods = self.allowed_methods(); methods.remove(MethodKind::PublicKey); + // Clone what we need for the async block + let auth_provider = Arc::clone(&self.auth_provider); + let rate_limiter = self.rate_limiter.clone(); + let peer_addr = self.peer_addr; + let user = user.to_string(); + let public_key = public_key.clone(); + + // Get mutable reference to session_info for authentication update + let session_info = &mut self.session_info; + async move { if exceeded { - tracing::warn!("Max authentication attempts exceeded"); + tracing::warn!( + user = %user, + peer = ?peer_addr, + "Max authentication attempts exceeded" + ); return Ok(Auth::Reject { proceed_with_methods: None, partial_success: false, }); } - // Placeholder - reject but allow other methods - // Will be implemented in #126 - let proceed = if methods.is_empty() { - None - } else { - Some(methods) - }; + // Check rate limiting based on peer address + let rate_key = peer_addr + .map(|addr| addr.ip().to_string()) + .unwrap_or_else(|| "unknown".to_string()); - Ok(Auth::Reject { - proceed_with_methods: proceed, - partial_success: false, - }) + if rate_limiter.is_rate_limited(&rate_key).await { + tracing::warn!( + user = %user, + peer = ?peer_addr, + "Rate limited authentication attempt" + ); + return Ok(Auth::Reject { + proceed_with_methods: None, + partial_success: false, + }); + } + + // Try to acquire a rate limit token + if rate_limiter.try_acquire(&rate_key).await.is_err() { + tracing::warn!( + user = %user, + peer = ?peer_addr, + "Rate limit exceeded" + ); + return Ok(Auth::Reject { + proceed_with_methods: None, + partial_success: false, + }); + } + + // Verify public key using auth provider + match auth_provider.verify_publickey(&user, &public_key).await { + Ok(result) if result.is_accepted() => { + tracing::info!( + user = %user, + peer = ?peer_addr, + key_type = %public_key.algorithm(), + "Public key authentication successful" + ); + + // Mark session as authenticated + if let Some(ref mut info) = session_info { + info.authenticate(&user); + } + + Ok(Auth::Accept) + } + Ok(_) => { + tracing::debug!( + user = %user, + peer = ?peer_addr, + key_type = %public_key.algorithm(), + "Public key authentication rejected" + ); + + let proceed = if methods.is_empty() { + None + } else { + Some(methods) + }; + + Ok(Auth::Reject { + proceed_with_methods: proceed, + partial_success: false, + }) + } + Err(e) => { + tracing::error!( + user = %user, + peer = ?peer_addr, + error = %e, + "Error during public key verification" + ); + + let proceed = if methods.is_empty() { + None + } else { + Some(methods) + }; + + Ok(Auth::Reject { + proceed_with_methods: proceed, + partial_success: false, + }) + } + } } } @@ -476,6 +600,8 @@ impl Drop for SshHandler { #[cfg(test)] mod tests { use super::*; + use crate::shared::auth_types::{AuthResult, UserInfo}; + use async_trait::async_trait; use std::net::{IpAddr, Ipv4Addr}; fn test_addr() -> SocketAddr { @@ -495,6 +621,66 @@ mod tests { Arc::new(RwLock::new(SessionManager::new())) } + /// Test auth provider that always accepts + struct AcceptAllAuthProvider; + + #[async_trait] + impl AuthProvider for AcceptAllAuthProvider { + async fn verify_publickey( + &self, + _username: &str, + _key: &ssh_key::PublicKey, + ) -> anyhow::Result { + Ok(AuthResult::Accept) + } + + async fn verify_password( + &self, + _username: &str, + _password: &str, + ) -> anyhow::Result { + Ok(AuthResult::Accept) + } + + async fn get_user_info(&self, username: &str) -> anyhow::Result> { + Ok(Some(UserInfo::new(username))) + } + + async fn user_exists(&self, _username: &str) -> anyhow::Result { + Ok(true) + } + } + + /// Test auth provider that always rejects + struct RejectAllAuthProvider; + + #[async_trait] + impl AuthProvider for RejectAllAuthProvider { + async fn verify_publickey( + &self, + _username: &str, + _key: &ssh_key::PublicKey, + ) -> anyhow::Result { + Ok(AuthResult::Reject) + } + + async fn verify_password( + &self, + _username: &str, + _password: &str, + ) -> anyhow::Result { + Ok(AuthResult::Reject) + } + + async fn get_user_info(&self, _username: &str) -> anyhow::Result> { + Ok(None) + } + + async fn user_exists(&self, _username: &str) -> anyhow::Result { + Ok(false) + } + } + #[test] fn test_handler_creation() { let handler = SshHandler::new(Some(test_addr()), test_config(), test_sessions()); @@ -505,6 +691,18 @@ mod tests { assert!(handler.username().is_none()); } + #[test] + fn test_handler_with_custom_auth_provider() { + let handler = SshHandler::with_auth_provider( + Some(test_addr()), + test_config(), + test_sessions(), + Arc::new(AcceptAllAuthProvider), + ); + + assert_eq!(handler.peer_addr(), Some(test_addr())); + } + #[test] fn test_allowed_methods_all() { let config = Arc::new( diff --git a/src/server/mod.rs b/src/server/mod.rs index fb35a652..e92dfce6 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -25,6 +25,7 @@ //! - [`SshHandler`]: Handles SSH protocol events for each connection //! - [`SessionManager`]: Tracks active sessions //! - [`ServerConfig`]: Server configuration options +//! - [`auth`]: Authentication providers (public key, password) //! //! # Example //! @@ -43,6 +44,7 @@ //! } //! ``` +pub mod auth; pub mod config; pub mod handler; pub mod session; From b5030a408566851d9d783a934c6fda509270ba9b Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Jan 2026 17:04:06 +0900 Subject: [PATCH 2/3] fix: address critical and high severity security issues in public key authentication CRITICAL Issue Fixed: - Fix TOCTOU race condition in load_authorized_keys by removing path.exists() check and handling NotFound from read operation. Use symlink_metadata to detect symlinks before reading. HIGH Severity Issues Fixed: - Add group-writable permission check (0o020) in check_file_permissions - Fix user enumeration timing attack in user_exists by using constant-time behavior - always perform same operations regardless of username validity - Add directory ownership and permission validation in load_authorized_keys - Fix symlink check in get_user_info to use symlink_metadata MEDIUM Issue Fixed: - Share rate limiter across handlers via Arc to provide server-wide rate limiting instead of per-instance limiting Security Improvements: - Use symlink_metadata consistently to avoid following symlinks - Validate parent directory permissions (not world-writable, warn on group-writable) - Check ownership consistency between file and parent directory - Reject both world-writable and group-writable authorized_keys files - Prevent user enumeration through timing attacks All tests pass with cargo test and cargo clippy. --- src/server/auth/publickey.rs | 145 ++++++++++++++++++++++++++++------- src/server/handler.rs | 25 ++++++ src/server/mod.rs | 12 ++- 3 files changed, 153 insertions(+), 29 deletions(-) diff --git a/src/server/auth/publickey.rs b/src/server/auth/publickey.rs index 609af69b..0fb51a77 100644 --- a/src/server/auth/publickey.rs +++ b/src/server/auth/publickey.rs @@ -272,37 +272,69 @@ impl PublicKeyVerifier { async fn load_authorized_keys(&self, username: &str) -> Result> { let path = self.config.get_authorized_keys_path(username); - if !path.exists() { - tracing::debug!( - user = %username, - path = %path.display(), - "No authorized_keys file found" - ); - return Ok(Vec::new()); - } - - // Check file permissions (should be readable but not world-writable) + // SECURITY: Check file permissions and reject symlinks before reading + // This prevents TOCTOU race conditions by using metadata operations #[cfg(unix)] self.check_file_permissions(&path)?; - let content = tokio::fs::read_to_string(&path) - .await - .with_context(|| format!("Failed to read authorized_keys file: {}", path.display()))?; + // Read the file - handle NotFound case to return empty vector + let content = match tokio::fs::read_to_string(&path).await { + Ok(content) => content, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + tracing::debug!( + user = %username, + path = %path.display(), + "No authorized_keys file found" + ); + return Ok(Vec::new()); + } + Err(e) => { + return Err(e).with_context(|| { + format!("Failed to read authorized_keys file: {}", path.display()) + }); + } + }; self.parse_authorized_keys(&content) } /// Check that file permissions are secure (Unix only). + /// + /// # Security Checks + /// + /// - Rejects symlinks to prevent TOCTOU attacks + /// - Rejects world-writable files (0o002) + /// - Rejects group-writable files (0o020) + /// - Validates parent directory permissions #[cfg(unix)] fn check_file_permissions(&self, path: &Path) -> Result<()> { use std::os::unix::fs::MetadataExt; - let metadata = std::fs::metadata(path) - .with_context(|| format!("Failed to get metadata for {}", path.display()))?; + // Use symlink_metadata to detect symlinks without following them + let metadata = match std::fs::symlink_metadata(path) { + Ok(m) => m, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + // File doesn't exist - this is OK, will be handled by caller + return Ok(()); + } + Err(e) => { + return Err(e).with_context(|| { + format!("Failed to get metadata for {}", path.display()) + }); + } + }; + + // SECURITY: Reject symlinks to prevent TOCTOU attacks + if metadata.is_symlink() { + anyhow::bail!( + "authorized_keys file {} is a symbolic link. Symlinks are not allowed for security reasons.", + path.display() + ); + } let mode = metadata.mode(); - // Check if file is world-writable (security risk) + // SECURITY: Check if file is world-writable (critical security risk) if mode & 0o002 != 0 { anyhow::bail!( "authorized_keys file {} is world-writable (mode {:o})", @@ -311,6 +343,53 @@ impl PublicKeyVerifier { ); } + // SECURITY: Check if file is group-writable (security risk) + if mode & 0o020 != 0 { + anyhow::bail!( + "authorized_keys file {} is group-writable (mode {:o}). This is a security risk.", + path.display(), + mode & 0o777 + ); + } + + // SECURITY: Validate parent directory permissions + if let Some(parent) = path.parent() { + if let Ok(parent_metadata) = std::fs::symlink_metadata(parent) { + let parent_mode = parent_metadata.mode(); + + // Parent directory should not be world-writable or group-writable + if parent_mode & 0o002 != 0 { + anyhow::bail!( + "Parent directory {} of authorized_keys is world-writable (mode {:o})", + parent.display(), + parent_mode & 0o777 + ); + } + + if parent_mode & 0o020 != 0 { + tracing::warn!( + "Parent directory {} of authorized_keys is group-writable (mode {:o}). This is a potential security risk.", + parent.display(), + parent_mode & 0o777 + ); + } + + // Check ownership - parent directory should be owned by same user as file + let file_uid = metadata.uid(); + let parent_uid = parent_metadata.uid(); + + if file_uid != parent_uid { + tracing::warn!( + "authorized_keys file {} (uid: {}) and parent directory {} (uid: {}) have different owners", + path.display(), + file_uid, + parent.display(), + parent_uid + ); + } + } + } + Ok(()) } @@ -435,25 +514,35 @@ impl AuthProvider for PublicKeyVerifier { // Validate username first let username = validate_username(username).context("Invalid username")?; - // Check if user has an authorized_keys file + // Check if user has an authorized_keys file using symlink_metadata + // to avoid following symlinks (security) let path = self.config.get_authorized_keys_path(&username); - if path.exists() { - Ok(Some(UserInfo::new(username))) - } else { - Ok(None) + match std::fs::symlink_metadata(&path) { + Ok(metadata) if metadata.is_file() => Ok(Some(UserInfo::new(username))), + Ok(_) => Ok(None), // Exists but not a regular file (e.g., symlink, directory) + Err(_) => Ok(None), // Doesn't exist or can't access } } async fn user_exists(&self, username: &str) -> Result { + // SECURITY: Use constant-time behavior to prevent user enumeration via timing + // Always perform the same operations regardless of whether user exists + // Validate username first - let username = match validate_username(username) { - Ok(u) => u, - Err(_) => return Ok(false), - }; + let username_result = validate_username(username); - // Check if authorized_keys file exists - let path = self.config.get_authorized_keys_path(&username); - Ok(path.exists()) + // Always compute the path, even if username is invalid + let path = self.config.get_authorized_keys_path( + username_result.as_deref().unwrap_or("_invalid_") + ); + + // Always perform a filesystem check using symlink_metadata to avoid following symlinks + let file_exists = std::fs::symlink_metadata(&path) + .map(|metadata| metadata.is_file()) + .unwrap_or(false); + + // Return false if username was invalid, true if username is valid AND file exists + Ok(username_result.is_ok() && file_exists) } } diff --git a/src/server/handler.rs b/src/server/handler.rs index af1a88c2..ff959bc0 100644 --- a/src/server/handler.rs +++ b/src/server/handler.rs @@ -67,6 +67,8 @@ impl SshHandler { ) -> Self { let auth_provider = config.create_auth_provider(); // Rate limiter: allow burst of 5 attempts, refill 1 attempt per second + // Note: This creates a per-handler rate limiter. For production use, + // prefer with_rate_limiter() to share a rate limiter across handlers. let rate_limiter = RateLimiter::with_simple_config(5, 1.0); Self { @@ -80,6 +82,29 @@ impl SshHandler { } } + /// Create a new SSH handler with a shared rate limiter. + /// + /// This is the preferred constructor for production use as it shares + /// the rate limiter across all handlers, providing server-wide rate limiting. + pub fn with_rate_limiter( + peer_addr: Option, + config: Arc, + sessions: Arc>, + rate_limiter: RateLimiter, + ) -> Self { + let auth_provider = config.create_auth_provider(); + + Self { + peer_addr, + config, + sessions, + auth_provider, + rate_limiter, + session_info: None, + channels: HashMap::new(), + } + } + /// Create a new SSH handler with a custom auth provider. /// /// This is useful for testing or when you need a different auth provider. diff --git a/src/server/mod.rs b/src/server/mod.rs index e92dfce6..125a9bef 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -59,6 +59,8 @@ use russh::server::Server; use tokio::net::{TcpListener, ToSocketAddrs}; use tokio::sync::RwLock; +use crate::shared::rate_limit::RateLimiter; + pub use self::config::{ServerConfig, ServerConfigBuilder}; pub use self::handler::SshHandler; pub use self::session::{ @@ -200,9 +202,14 @@ impl BsshServer { "SSH server listening" ); + // Create shared rate limiter for all handlers + // Allow burst of 5 auth attempts, refill 1 attempt per second + let rate_limiter = RateLimiter::with_simple_config(5, 1.0); + let mut server = BsshServerRunner { config: Arc::clone(&self.config), sessions: Arc::clone(&self.sessions), + rate_limiter, }; // Use run_on_socket which handles the server loop @@ -231,6 +238,8 @@ impl BsshServer { struct BsshServerRunner { config: Arc, sessions: Arc>, + /// Shared rate limiter for authentication attempts across all handlers + rate_limiter: RateLimiter, } impl russh::server::Server for BsshServerRunner { @@ -242,10 +251,11 @@ impl russh::server::Server for BsshServerRunner { "New client connection" ); - SshHandler::new( + SshHandler::with_rate_limiter( peer_addr, Arc::clone(&self.config), Arc::clone(&self.sessions), + self.rate_limiter.clone(), ) } From b87a74496f645b607de5bbd77b829240c610f418 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 22 Jan 2026 17:07:20 +0900 Subject: [PATCH 3/3] chore: finalize PR with documentation and formatting - Update ARCHITECTURE.md with comprehensive auth module documentation - Add authentication module reference to docs/architecture/README.md - Apply cargo fmt formatting to publickey.rs --- ARCHITECTURE.md | 59 ++++++++++++++++++++++++++++++++++-- docs/architecture/README.md | 1 + src/server/auth/publickey.rs | 24 ++++++++------- 3 files changed, 71 insertions(+), 13 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 61f9f6b8..d495cf49 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -198,6 +198,7 @@ SSH server implementation using the russh library for accepting incoming connect - `config.rs` - `ServerConfig` with builder pattern for server settings - `handler.rs` - `SshHandler` implementing `russh::server::Handler` trait - `session.rs` - Session state management (`SessionManager`, `SessionInfo`, `ChannelState`) +- `auth/` - Authentication provider infrastructure **Key Components**: @@ -205,14 +206,17 @@ SSH server implementation using the russh library for accepting incoming connect - Accepts connections on configured address - Loads host keys from OpenSSH format files - Configures russh with authentication settings + - Creates shared rate limiter for authentication attempts - **ServerConfig**: Configuration options with builder pattern - Host key paths and listen address - Connection limits and timeouts - Authentication method toggles (password, publickey, keyboard-interactive) + - Public key authentication configuration (authorized_keys location) - **SshHandler**: Per-connection handler for SSH protocol events - - Authentication handling (placeholder implementations) + - Public key authentication via AuthProvider trait + - Rate limiting for authentication attempts - Channel operations (open, close, EOF, data) - PTY, exec, shell, and subsystem request handling @@ -221,7 +225,58 @@ SSH server implementation using the russh library for accepting incoming connect - Idle session management - Authentication state tracking -**Current Status**: Foundation implementation with placeholder authentication. Actual authentication and command execution will be implemented in follow-up issues (#126-#132). +### Server Authentication Module + +The authentication subsystem (`src/server/auth/`) provides extensible authentication for the SSH server: + +**Structure**: +- `mod.rs` - Module exports and re-exports +- `provider.rs` - `AuthProvider` trait definition +- `publickey.rs` - `PublicKeyVerifier` implementation + +**AuthProvider Trait**: + +The `AuthProvider` trait defines the interface for all authentication backends: + +```rust +#[async_trait] +pub trait AuthProvider: Send + Sync { + async fn verify_publickey(&self, username: &str, key: &PublicKey) -> Result; + async fn verify_password(&self, username: &str, password: &str) -> Result; + async fn get_user_info(&self, username: &str) -> Result>; + async fn user_exists(&self, username: &str) -> Result; +} +``` + +**PublicKeyVerifier**: + +Implements public key authentication by parsing OpenSSH authorized_keys files: + +- **Key file location modes**: + - Directory mode: `{dir}/{username}/authorized_keys` + - Pattern mode: `/home/{user}/.ssh/authorized_keys` + +- **Supported key types**: + - ssh-ed25519, ssh-ed448 + - ssh-rsa, ssh-dss + - ecdsa-sha2-nistp256/384/521 + - Security keys (sk-ssh-ed25519, sk-ecdsa-sha2-nistp256) + +- **Key options parsing**: + - `command="..."` - Force specific command + - `from="..."` - Restrict source addresses + - `no-pty`, `no-port-forwarding`, `no-agent-forwarding`, `no-X11-forwarding` + - `environment="..."` - Set environment variables + +**Security Features**: + +- **Username validation**: Prevents path traversal attacks (e.g., `../etc/passwd`) +- **File permission checks** (Unix): Rejects world/group-writable files and symlinks +- **Symlink protection**: Uses `symlink_metadata()` to detect and reject symlinks +- **Parent directory validation**: Checks parent directory permissions +- **Rate limiting**: Token bucket rate limiter for authentication attempts +- **Timing attack mitigation**: Constant-time behavior in `user_exists()` check +- **Comprehensive logging**: All authentication attempts are logged ## Data Flow diff --git a/docs/architecture/README.md b/docs/architecture/README.md index de814dd3..6a091bbf 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -33,6 +33,7 @@ bssh is a high-performance parallel SSH command execution tool with SSH-compatib ### Server Components - **SSH Server Module** - SSH server implementation using russh (see main ARCHITECTURE.md) +- **Server Authentication** - Authentication providers including public key verification (see main ARCHITECTURE.md) ## Navigation diff --git a/src/server/auth/publickey.rs b/src/server/auth/publickey.rs index 0fb51a77..295b4c87 100644 --- a/src/server/auth/publickey.rs +++ b/src/server/auth/publickey.rs @@ -318,9 +318,8 @@ impl PublicKeyVerifier { return Ok(()); } Err(e) => { - return Err(e).with_context(|| { - format!("Failed to get metadata for {}", path.display()) - }); + return Err(e) + .with_context(|| format!("Failed to get metadata for {}", path.display())); } }; @@ -468,9 +467,8 @@ impl PublicKeyVerifier { // Parse the public key let key_str = format!("{key_type} {key_data}"); - let key = parse_public_key(&key_str).with_context(|| { - format!("Failed to parse public key of type {key_type}") - })?; + let key = parse_public_key(&key_str) + .with_context(|| format!("Failed to parse public key of type {key_type}"))?; Ok(AuthorizedKey { key, @@ -532,9 +530,9 @@ impl AuthProvider for PublicKeyVerifier { let username_result = validate_username(username); // Always compute the path, even if username is invalid - let path = self.config.get_authorized_keys_path( - username_result.as_deref().unwrap_or("_invalid_") - ); + let path = self + .config + .get_authorized_keys_path(username_result.as_deref().unwrap_or("_invalid_")); // Always perform a filesystem check using symlink_metadata to avoid following symlinks let file_exists = std::fs::symlink_metadata(&path) @@ -663,7 +661,10 @@ mod tests { fn test_config_with_directory() { let config = PublicKeyAuthConfig::with_directory("/etc/bssh/keys"); let path = config.get_authorized_keys_path("testuser"); - assert_eq!(path, PathBuf::from("/etc/bssh/keys/testuser/authorized_keys")); + assert_eq!( + path, + PathBuf::from("/etc/bssh/keys/testuser/authorized_keys") + ); } #[test] @@ -741,7 +742,8 @@ mod tests { let verifier = PublicKeyVerifier::new(PublicKeyAuthConfig::default()); // Create a dummy key for testing - let key_str = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl"; + let key_str = + "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl"; let key = parse_public_key(key_str).unwrap(); // Path traversal attempt should fail