diff --git a/src/cli/bssh.rs b/src/cli/bssh.rs index 04be73bb..f3fbd272 100644 --- a/src/cli/bssh.rs +++ b/src/cli/bssh.rs @@ -299,13 +299,18 @@ pub struct Cli { )] pub quiet: bool, - #[arg(short = 't', long = "tty", help = "Force pseudo-terminal allocation")] + #[arg( + short = 't', + long = "tty", + overrides_with_all = ["force_tty", "no_tty"], + help = "Force pseudo-terminal allocation" + )] pub force_tty: bool, #[arg( short = 'T', long = "no-tty", - conflicts_with = "force_tty", + overrides_with_all = ["force_tty", "no_tty"], help = "Disable pseudo-terminal allocation" )] pub no_tty: bool, @@ -842,6 +847,27 @@ mod tests { ); } + #[test] + fn openssh_tty_flags_accept_repetition_and_last_flag_wins() { + for (args, expected) in [ + (vec!["bssh", "-t", "target"], (true, false, false)), + (vec!["bssh", "-tt", "target"], (true, false, false)), + (vec!["bssh", "-ttq", "target"], (true, false, true)), + (vec!["bssh", "-T", "target"], (false, true, false)), + (vec!["bssh", "-tT", "target"], (false, true, false)), + (vec!["bssh", "-Tt", "target"], (true, false, false)), + ] { + let cli = Cli::try_parse_from(&args) + .unwrap_or_else(|error| panic!("failed to parse {args:?}: {error}")); + + assert_eq!( + (cli.force_tty, cli.no_tty, cli.quiet), + expected, + "unexpected tty mode for {args:?}" + ); + } + } + #[test] fn ssh_option_lookup_is_case_insensitive_and_first_value_wins() { let cli = Cli::try_parse_from([ diff --git a/src/ssh/config_cache/manager.rs b/src/ssh/config_cache/manager.rs index 9b3f5ff8..20b490b9 100644 --- a/src/ssh/config_cache/manager.rs +++ b/src/ssh/config_cache/manager.rs @@ -16,6 +16,7 @@ use super::config::CacheConfig; use super::entry::CacheEntry; use super::stats::CacheStats; use crate::ssh::SshConfig; +use crate::ssh::ssh_config::diagnostic::escape_path; use anyhow::{Context, Result}; use lru::LruCache; use std::path::{Path, PathBuf}; @@ -67,16 +68,16 @@ impl SshConfigCache { let path_ref = path.as_ref(); let path = tokio::fs::canonicalize(path_ref) .await - .with_context(|| format!("Failed to canonicalize path: {}", path_ref.display()))?; + .with_context(|| format!("Failed to canonicalize path: {}", escape_path(path_ref)))?; // Check if file exists and get its modification time let file_metadata = tokio::fs::metadata(&path) .await - .with_context(|| format!("Failed to read file metadata: {}", path.display()))?; + .with_context(|| format!("Failed to read file metadata: {}", escape_path(&path)))?; let current_mtime = file_metadata .modified() - .with_context(|| format!("Failed to get modification time: {}", path.display()))?; + .with_context(|| format!("Failed to get modification time: {}", escape_path(&path)))?; // Try to get from cache first if let Some(config) = self.try_get_cached(&path, current_mtime)? { @@ -84,10 +85,13 @@ impl SshConfigCache { } // Cache miss - load from file - trace!("Cache miss for SSH config: {}", path.display()); - let config = SshConfig::load_from_file(&path) - .await - .with_context(|| format!("Failed to load SSH config from file: {}", path.display()))?; + trace!("Cache miss for SSH config: {}", escape_path(&path)); + let config = SshConfig::load_from_file(&path).await.with_context(|| { + format!( + "Failed to load SSH config from file: {}", + escape_path(&path) + ) + })?; // Store in cache if let Err(e) = self.put(path, config.clone(), current_mtime) { @@ -120,7 +124,7 @@ impl SshConfigCache { if let Some(entry) = cache.get_mut(path) { // Check if entry is expired if entry.is_expired(self.config.ttl) { - debug!("SSH config cache entry expired: {}", path.display()); + debug!("SSH config cache entry expired: {}", escape_path(path)); cache.pop(path); let mut stats = self.stats.write().map_err(|e| { @@ -132,7 +136,7 @@ impl SshConfigCache { // Check if entry is stale (file was modified) if entry.is_stale(current_mtime) { - debug!("SSH config cache entry stale: {}", path.display()); + debug!("SSH config cache entry stale: {}", escape_path(path)); cache.pop(path); let mut stats = self.stats.write().map_err(|e| { @@ -153,7 +157,7 @@ impl SshConfigCache { stats.hits += 1; } - trace!("SSH config cache hit: {}", path.display()); + trace!("SSH config cache hit: {}", escape_path(path)); return Ok(Some(config)); } @@ -185,7 +189,7 @@ impl SshConfigCache { stats.current_entries = cache.len(); } - trace!("SSH config cached: {}", path.display()); + trace!("SSH config cached: {}", escape_path(&path)); Ok(()) } diff --git a/src/ssh/ssh_config/diagnostic.rs b/src/ssh/ssh_config/diagnostic.rs new file mode 100644 index 00000000..4da0aab8 --- /dev/null +++ b/src/ssh/ssh_config/diagnostic.rs @@ -0,0 +1,61 @@ +// 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. + +use std::{borrow::Cow, path::Path}; + +pub(crate) fn escape_field(value: &str) -> Cow<'_, str> { + if !value.chars().any(char::is_control) { + return Cow::Borrowed(value); + } + + let mut escaped = String::with_capacity(value.len()); + for character in value.chars() { + if character.is_control() { + escaped.extend(character.escape_default()); + } else { + escaped.push(character); + } + } + Cow::Owned(escaped) +} + +pub(crate) fn escape_path(path: &Path) -> Cow<'_, str> { + match path.to_string_lossy() { + Cow::Borrowed(value) => escape_field(value), + Cow::Owned(value) => Cow::Owned(escape_field(&value).into_owned()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn fields_escape_controls_but_preserve_printable_unicode() { + assert_eq!( + escape_field("경로/é/λ\r\n\t\u{1b}\u{7f}\u{85}"), + "경로/é/λ\\r\\n\\t\\u{1b}\\u{7f}\\u{85}" + ); + assert!(matches!(escape_field("경로/é/λ"), Cow::Borrowed(_))); + } + + #[test] + fn paths_use_the_same_control_escaping_contract() { + assert_eq!(escape_path(Path::new("safe\nFORGED")), "safe\\nFORGED"); + assert!(matches!( + escape_path(Path::new("printable/경로")), + Cow::Borrowed(_) + )); + } +} diff --git a/src/ssh/ssh_config/include/mod.rs b/src/ssh/ssh_config/include/mod.rs index 8755b946..e62b9610 100644 --- a/src/ssh/ssh_config/include/mod.rs +++ b/src/ssh/ssh_config/include/mod.rs @@ -21,6 +21,8 @@ use anyhow::{Context, Result}; use std::collections::HashSet; use std::path::{Path, PathBuf}; +use super::diagnostic::{escape_field, escape_path}; + mod resolver; mod validation; @@ -95,7 +97,7 @@ impl IncludeContext { // Canonicalize and cache the result let canonical = path .canonicalize() - .with_context(|| format!("Failed to canonicalize path: {}", path.display()))?; + .with_context(|| format!("Failed to canonicalize path: {}", escape_path(path)))?; self.canonical_cache .insert(path.to_path_buf(), canonical.clone()); canonical @@ -115,7 +117,7 @@ impl IncludeContext { if self.visited.contains(&canonical_str) { anyhow::bail!( "Include cycle detected: {} has already been processed", - path.display() + escape_path(path) ); } @@ -166,7 +168,7 @@ pub async fn resolve_includes(config_path: &Path, content: &str) -> Result= MAX_GLOB_RESULTS { anyhow::bail!( - "Glob pattern '{pattern}' matched too many files (>{MAX_GLOB_RESULTS}). \ - Please use a more specific pattern." + "Glob pattern '{}' matched too many files (>{MAX_GLOB_RESULTS}). Please use a more specific pattern.", + escape_field(pattern) ); } @@ -118,7 +119,11 @@ pub async fn resolve_include_pattern( Ok(c) => c, Err(_) if !path.exists() => continue, // Skip non-existent files Err(e) => { - tracing::debug!("Failed to canonicalize {}: {}", path.display(), e); + tracing::debug!( + "Failed to canonicalize {}: {}", + escape_path(&path), + escape_field(&e.to_string()) + ); continue; } }; @@ -127,7 +132,7 @@ pub async fn resolve_include_pattern( if !is_path_allowed(&canonical) { tracing::warn!( "Glob result {} escapes allowed directories, skipping", - path.display() + escape_path(&path) ); continue; } @@ -144,13 +149,21 @@ pub async fn resolve_include_pattern( } } Err(e) => { - tracing::debug!("Failed to get metadata for {}: {}", path.display(), e); + tracing::debug!( + "Failed to get metadata for {}: {}", + escape_path(&path), + escape_field(&e.to_string()) + ); } } } Err(e) => { // Log glob errors but continue - tracing::warn!("Error processing glob pattern '{}': {}", pattern_str, e); + tracing::warn!( + "Error processing glob pattern '{}': {}", + escape_field(pattern_str), + escape_field(&e.to_string()) + ); } } } @@ -162,7 +175,7 @@ pub async fn resolve_include_pattern( if files.is_empty() && !pattern.contains('*') && !pattern.contains('?') { tracing::debug!( "Include pattern '{}' matched no files (this may be intentional)", - pattern + escape_field(pattern) ); } diff --git a/src/ssh/ssh_config/include/validation.rs b/src/ssh/ssh_config/include/validation.rs index ccd9f0f1..acb1837f 100644 --- a/src/ssh/ssh_config/include/validation.rs +++ b/src/ssh/ssh_config/include/validation.rs @@ -17,6 +17,8 @@ use anyhow::{Context, Result}; use std::path::{Path, PathBuf}; +use super::super::diagnostic::{escape_field, escape_path}; + /// Validate a glob pattern for security pub fn validate_glob_pattern(pattern: &str) -> Result<()> { // Check for dangerous glob patterns @@ -27,13 +29,19 @@ pub fn validate_glob_pattern(pattern: &str) -> Result<()> { // Check for excessive wildcards that could cause exponential expansion let wildcard_count = pattern.chars().filter(|&c| c == '*').count(); if wildcard_count > 5 { - anyhow::bail!("Too many wildcards in pattern '{pattern}'. Maximum 5 wildcards allowed."); + anyhow::bail!( + "Too many wildcards in pattern '{}'. Maximum 5 wildcards allowed.", + escape_field(pattern) + ); } // Check for overly broad patterns that could match system files // But allow common SSH config patterns like ~/.ssh/config.d/* if (pattern == "*" || pattern == "/*") && !pattern.contains("ssh") { - anyhow::bail!("Pattern '{pattern}' is too broad and could match system files"); + anyhow::bail!( + "Pattern '{}' is too broad and could match system files", + escape_field(pattern) + ); } // Check pattern length @@ -69,32 +77,32 @@ pub fn validate_include_path(path: &Path) -> Result<()> { // Get metadata without following symlinks let metadata = std::fs::symlink_metadata(path) - .with_context(|| format!("Failed to get metadata for {}", path.display()))?; + .with_context(|| format!("Failed to get metadata for {}", escape_path(path)))?; // Reject symbolic links for security if metadata.is_symlink() { anyhow::bail!( "Include path {} is a symbolic link. Symlinks are not allowed for security reasons.", - path.display() + escape_path(path) ); } // Check if it's a regular file if !metadata.is_file() { - anyhow::bail!("Include path is not a regular file: {}", path.display()); + anyhow::bail!("Include path is not a regular file: {}", escape_path(path)); } // Canonicalize and verify the path doesn't escape expected directories let canonical = path .canonicalize() - .with_context(|| format!("Failed to canonicalize {}", path.display()))?; + .with_context(|| format!("Failed to canonicalize {}", escape_path(path)))?; // Check for directory traversal attempts let path_str = canonical.to_string_lossy(); if path_str.contains("../") || path_str.contains("..\\") { anyhow::bail!( "Include path {} contains directory traversal sequences", - path.display() + escape_path(path) ); } @@ -113,7 +121,7 @@ pub fn validate_include_path(path: &Path) -> Result<()> { if !is_safe { tracing::warn!( "Include path {} is outside of standard SSH config directories. This may be a security risk.", - canonical.display() + escape_path(&canonical) ); } @@ -130,7 +138,7 @@ pub fn validate_include_path(path: &Path) -> Result<()> { if mode & 0o002 != 0 { anyhow::bail!( "SSH config file {} is world-writable. This is a security vulnerability.", - path.display() + escape_path(path) ); } @@ -138,7 +146,7 @@ pub fn validate_include_path(path: &Path) -> Result<()> { if mode & 0o020 != 0 { tracing::warn!( "SSH config file {} is group-writable. This is a potential security risk.", - path.display() + escape_path(path) ); } } diff --git a/src/ssh/ssh_config/mod.rs b/src/ssh/ssh_config/mod.rs index 1d619c56..1132d8d9 100644 --- a/src/ssh/ssh_config/mod.rs +++ b/src/ssh/ssh_config/mod.rs @@ -18,9 +18,13 @@ //! configurations, and provide a clean API for SSH connection setup. use anyhow::{Context, Result}; -use std::path::{Path, PathBuf}; +use std::{ + collections::HashSet, + path::{Path, PathBuf}, +}; // Internal modules +pub(crate) mod diagnostic; mod env_cache; mod include; #[cfg(test)] @@ -50,6 +54,7 @@ pub use types::SshHostConfig; #[derive(Debug, Clone, Default)] pub struct SshConfig { pub hosts: Vec, + reported_diagnostics: HashSet, } impl SshConfig { @@ -61,13 +66,21 @@ impl SshConfig { /// Load SSH configuration from a file with Include support pub async fn load_from_file>(path: P) -> Result { let path = path.as_ref(); - let content = tokio::fs::read_to_string(path) - .await - .with_context(|| format!("Failed to read SSH config file: {}", path.display()))?; + let content = tokio::fs::read_to_string(path).await.with_context(|| { + format!( + "Failed to read SSH config file: {}", + diagnostic::escape_path(path) + ) + })?; Self::parse_from_file_with_content(path, &content) .await - .with_context(|| format!("Failed to parse SSH config file: {}", path.display())) + .with_context(|| { + format!( + "Failed to parse SSH config file: {}", + diagnostic::escape_path(path) + ) + }) } /// Load SSH configuration from a file with caching @@ -104,8 +117,12 @@ impl SshConfig { /// Parse SSH configuration from a string (without Include support) pub fn parse(content: &str) -> Result { - let hosts = parser::parse(content)?; - Ok(Self { hosts }) + let mut reported_diagnostics = HashSet::new(); + let hosts = parser::parse_with_diagnostics(content, &mut reported_diagnostics)?; + Ok(Self { + hosts, + reported_diagnostics, + }) } /// Prepend command-line `-o` options as a structured `Host *` block. @@ -114,7 +131,7 @@ impl SshConfig { /// accepted command-line options have CLI precedence without bypassing /// the parser's validation or first-obtained merge rules. pub fn apply_cli_options(&mut self, options: &[String]) -> Result<()> { - if let Some(overlay) = parser::parse_cli_options(options)? { + if let Some(overlay) = parser::parse_cli_options(options, &mut self.reported_diagnostics)? { self.hosts.insert(0, overlay); } Ok(()) @@ -122,8 +139,14 @@ impl SshConfig { /// Parse SSH configuration from a file with Include support pub async fn parse_from_file_with_content(path: &Path, content: &str) -> Result { - let hosts = parser::parse_from_file(path, content).await?; - Ok(Self { hosts }) + let mut reported_diagnostics = HashSet::new(); + let hosts = + parser::parse_from_file_with_diagnostics(path, content, &mut reported_diagnostics) + .await?; + Ok(Self { + hosts, + reported_diagnostics, + }) } /// Find configuration for a specific hostname diff --git a/src/ssh/ssh_config/parser/core.rs b/src/ssh/ssh_config/parser/core.rs index 7246e4d3..6c385701 100644 --- a/src/ssh/ssh_config/parser/core.rs +++ b/src/ssh/ssh_config/parser/core.rs @@ -25,31 +25,50 @@ use anyhow::{Context, Result}; use std::collections::HashSet; use std::path::Path; +use super::diagnostic::DiagnosticSource; use super::options; +use crate::ssh::ssh_config::diagnostic::escape_path; /// Parse SSH configuration content with Include and Match support +#[cfg(test)] pub fn parse(content: &str) -> Result> { + let mut reported_diagnostics = HashSet::new(); + parse_with_diagnostics(content, &mut reported_diagnostics) +} + +pub(crate) fn parse_with_diagnostics( + content: &str, + reported_diagnostics: &mut HashSet, +) -> Result> { // For synchronous parsing without file path, we can't resolve includes // This maintains backward compatibility for tests and simple usage - parse_without_includes(content) + parse_without_includes(content, reported_diagnostics) } -/// Parse SSH configuration from a file with full Include support -pub async fn parse_from_file(path: &Path, content: &str) -> Result> { +/// Parse SSH configuration from a file with full Include support. +pub(crate) async fn parse_from_file_with_diagnostics( + path: &Path, + content: &str, + reported_diagnostics: &mut HashSet, +) -> Result> { // Pass 1: Resolve all Include directives let included_files = resolve_includes(path, content) .await - .with_context(|| format!("Failed to resolve includes for {}", path.display()))?; - parse_included_files(&included_files) + .with_context(|| format!("Failed to resolve includes for {}", escape_path(path)))?; + parse_included_files(&included_files, reported_diagnostics) } /// Parse SSH configuration content without Include resolution -pub(super) fn parse_without_includes(content: &str) -> Result> { +pub(super) fn parse_without_includes( + content: &str, + reported_diagnostics: &mut HashSet, +) -> Result> { parse_lines( content .lines() .enumerate() .map(|(index, line)| (None, index + 1, line)), + reported_diagnostics, ) } @@ -58,7 +77,10 @@ pub(super) fn parse_without_includes(content: &str) -> Result /// Keeping the overlay as a structured block lets the ordinary resolver apply /// OpenSSH's first-obtained rule: CLI options are visited before file blocks, /// while repeated `-o` scalars retain the first CLI value. -pub(crate) fn parse_cli_options(options: &[String]) -> Result> { +pub(crate) fn parse_cli_options( + options: &[String], + reported_diagnostics: &mut HashSet, +) -> Result> { const MAX_LINE_LENGTH: usize = 8192; const MAX_VALUE_LENGTH: usize = 4096; @@ -71,8 +93,6 @@ pub(crate) fn parse_cli_options(options: &[String]) -> Result Result Result Result> { - parse_lines(files.iter().flat_map(|file| { - file.content.lines().enumerate().map(move |(index, line)| { - ( - Some(file.path.as_path()), - file.source_line_start + index, - line, - ) - }) - })) +fn parse_included_files( + files: &[IncludedFile], + reported_diagnostics: &mut HashSet, +) -> Result> { + parse_lines( + files.iter().flat_map(|file| { + file.content.lines().enumerate().map(move |(index, line)| { + ( + Some(file.path.as_path()), + file.source_line_start + index, + line, + ) + }) + }), + reported_diagnostics, + ) } fn parse_lines<'a>( lines: impl IntoIterator, usize, &'a str)>, + reported_diagnostics: &mut HashSet, ) -> Result> { // Security: Set reasonable limits to prevent DoS attacks const MAX_LINE_LENGTH: usize = 8192; // 8KB per line should be more than enough @@ -124,8 +150,6 @@ fn parse_lines<'a>( let mut current_config: Option = None; let mut current_match: Option = None; let mut in_match_block = false; - let mut reported_diagnostics = HashSet::new(); - for (source_path, line_number, line) in lines { // Security: Check line length to prevent DoS if line.len() > MAX_LINE_LENGTH { @@ -230,9 +254,11 @@ fn parse_lines<'a>( &mut match_block.config, &keyword, &args, - source_path, - line_number, - &mut reported_diagnostics, + DiagnosticSource::Config { + path: source_path, + line_number, + }, + reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; } @@ -241,9 +267,11 @@ fn parse_lines<'a>( config, &keyword, &args, - source_path, - line_number, - &mut reported_diagnostics, + DiagnosticSource::Config { + path: source_path, + line_number, + }, + reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; } else { @@ -260,9 +288,11 @@ fn parse_lines<'a>( config, &keyword, &args, - source_path, - line_number, - &mut reported_diagnostics, + DiagnosticSource::Config { + path: source_path, + line_number, + }, + reported_diagnostics, ) .with_context(|| format!("Error at line {line_number}: {line}"))?; } @@ -287,19 +317,11 @@ fn parse_option_first( target: &mut SshHostConfig, keyword: &str, args: &[String], - source_path: Option<&Path>, - line_number: usize, + source: DiagnosticSource<'_>, reported_diagnostics: &mut HashSet, ) -> Result<()> { let mut parsed = SshHostConfig::default(); - options::parse_option( - &mut parsed, - keyword, - args, - source_path, - line_number, - reported_diagnostics, - )?; + options::parse_option(&mut parsed, keyword, args, source, reported_diagnostics)?; merge_host_config(target, &parsed); Ok(()) } diff --git a/src/ssh/ssh_config/parser/diagnostic.rs b/src/ssh/ssh_config/parser/diagnostic.rs new file mode 100644 index 00000000..411d4974 --- /dev/null +++ b/src/ssh/ssh_config/parser/diagnostic.rs @@ -0,0 +1,79 @@ +// 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. + +use crate::ssh::ssh_config::diagnostic::escape_path; +use std::path::Path; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum DiagnosticSource<'a> { + Config { + path: Option<&'a Path>, + line_number: usize, + }, + CliOption { + option_number: usize, + }, +} + +impl DiagnosticSource<'_> { + pub(super) fn number(self) -> usize { + match self { + Self::Config { line_number, .. } => line_number, + Self::CliOption { option_number } => option_number, + } + } + + pub(super) fn location(self) -> String { + match self { + Self::Config { + path: Some(path), + line_number, + } => format!("{}:{line_number}", escape_path(path)), + Self::Config { + path: None, + line_number, + } => format!("line {line_number}"), + Self::CliOption { option_number } => format!("-o option #{option_number}"), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn diagnostic_locations_distinguish_files_lines_and_cli_options() { + assert_eq!( + DiagnosticSource::Config { + path: Some(Path::new("safe\nFORGED")), + line_number: 7, + } + .location(), + "safe\\nFORGED:7" + ); + assert_eq!( + DiagnosticSource::Config { + path: None, + line_number: 3, + } + .location(), + "line 3" + ); + assert_eq!( + DiagnosticSource::CliOption { option_number: 4 }.location(), + "-o option #4" + ); + } +} diff --git a/src/ssh/ssh_config/parser/mod.rs b/src/ssh/ssh_config/parser/mod.rs index 5a446af3..8a5937ab 100644 --- a/src/ssh/ssh_config/parser/mod.rs +++ b/src/ssh/ssh_config/parser/mod.rs @@ -21,6 +21,7 @@ //! - `tests`: Comprehensive test suite mod core; +mod diagnostic; mod helpers; mod options; @@ -28,7 +29,11 @@ mod options; mod tests; // Re-export public items from core module -pub(super) use core::{parse, parse_cli_options, parse_from_file}; +#[cfg(test)] +pub(super) use core::parse; +pub(super) use core::{ + parse_cli_options, parse_from_file_with_diagnostics, parse_with_diagnostics, +}; // Re-export helper functions that might be used elsewhere diff --git a/src/ssh/ssh_config/parser/options/mod.rs b/src/ssh/ssh_config/parser/options/mod.rs index ab60e768..05b6a09f 100644 --- a/src/ssh/ssh_config/parser/options/mod.rs +++ b/src/ssh/ssh_config/parser/options/mod.rs @@ -29,9 +29,11 @@ mod security; mod support; mod ui; +use super::diagnostic::DiagnosticSource; +use crate::ssh::ssh_config::diagnostic::escape_field; use crate::ssh::ssh_config::types::SshHostConfig; use anyhow::Result; -use std::{collections::HashSet, path::Path}; +use std::collections::HashSet; /// Parse a configuration option for a host /// @@ -41,43 +43,27 @@ pub fn parse_option( host: &mut SshHostConfig, accepted_keyword: &str, args: &[String], - source_path: Option<&Path>, - line_number: usize, + source: DiagnosticSource<'_>, reported_diagnostics: &mut HashSet, ) -> Result<()> { + let line_number = source.number(); let Some(spec) = support::keyword_spec(accepted_keyword) else { if reported_diagnostics.insert(format!("unknown:{accepted_keyword}")) { - let location = source_path.map_or_else( - || format!("line {line_number}"), - |path| format!("{}:{line_number}", path.display()), - ); - crate::diagnosticln!("Unknown SSH config option '{accepted_keyword}' at {location}"); + let keyword = escape_field(accepted_keyword); + let location = source.location(); + crate::diagnosticln!("Unknown SSH config option '{keyword}' at {location}"); } return Ok(()); }; let keyword = spec.canonical; - let tracking_issue = match spec.support { - support::KeywordSupport::Runtime(_) => None, - support::KeywordSupport::Delegated(issue) => Some(issue), - support::KeywordSupport::Unimplemented => Some(0), - }; - if let Some(issue) = - tracking_issue.filter(|_| reported_diagnostics.insert(format!("unsupported:{keyword}"))) + if spec.support == support::KeywordSupport::Unimplemented + && reported_diagnostics.insert(format!("unsupported:{keyword}")) { - let location = source_path.map_or_else( - || format!("line {line_number}"), - |path| format!("{}:{line_number}", path.display()), + let location = source.location(); + crate::diagnosticln!( + "Unsupported SSH config option '{keyword}' at {location}; bssh parses this value for inspection but does not implement its runtime behavior" ); - if issue == 0 { - crate::diagnosticln!( - "Unsupported SSH config option '{keyword}' at {location}; bssh parses this value for inspection but does not implement its runtime behavior" - ); - } else { - crate::diagnosticln!( - "SSH config option '{keyword}' at {location} is not implemented yet; tracked in #{issue}" - ); - } } match keyword { diff --git a/src/ssh/ssh_config/parser/options/support.rs b/src/ssh/ssh_config/parser/options/support.rs index 25236732..39eef257 100644 --- a/src/ssh/ssh_config/parser/options/support.rs +++ b/src/ssh/ssh_config/parser/options/support.rs @@ -4,10 +4,6 @@ pub(super) enum KeywordSupport { Runtime(RuntimeConsumer), Unimplemented, - // Keep the classification available for future split issue waves even - // when the current wave has no remaining delegated keywords. - #[allow(dead_code)] - Delegated(u32), } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -260,7 +256,11 @@ pub(super) fn keyword_spec(keyword: &str) -> Option { #[cfg(test)] mod tests { use super::*; - use std::collections::{HashMap, HashSet}; + use std::collections::HashSet; + + const ACCEPTED_SPELLING_COUNT: usize = 91; + const RUNTIME_SPELLING_COUNT: usize = 51; + const UNIMPLEMENTED_SPELLING_COUNT: usize = 40; #[test] fn accepted_keywords_and_aliases_have_one_consistent_classification() { @@ -279,6 +279,20 @@ mod tests { assert_eq!(canonical_spec.canonical, *canonical); assert_eq!(canonical_spec.support, *support); } + + let runtime_count = ACCEPTED_KEYWORDS + .iter() + .filter(|(_, _, support)| matches!(support, KeywordSupport::Runtime(_))) + .count(); + let unimplemented_count = ACCEPTED_KEYWORDS + .iter() + .filter(|(_, _, support)| matches!(support, KeywordSupport::Unimplemented)) + .count(); + + assert_eq!(ACCEPTED_KEYWORDS.len(), ACCEPTED_SPELLING_COUNT); + assert_eq!(runtime_count, RUNTIME_SPELLING_COUNT); + assert_eq!(unimplemented_count, UNIMPLEMENTED_SPELLING_COUNT); + assert_eq!(runtime_count + unimplemented_count, ACCEPTED_KEYWORDS.len()); } #[test] @@ -347,7 +361,7 @@ mod tests { } match support { KeywordSupport::Runtime(consumer) => Some((*keyword, *consumer)), - KeywordSupport::Delegated(_) | KeywordSupport::Unimplemented => None, + KeywordSupport::Unimplemented => None, } }) .collect::>(); @@ -356,18 +370,55 @@ mod tests { } #[test] - fn first_wave_delegations_match_the_split_issue_dag() { - let expected = HashMap::new(); - let delegated = ACCEPTED_KEYWORDS + fn unimplemented_keywords_are_exactly_the_audited_named_set() { + let expected = [ + "addkeystoagent", + "identityagent", + "kbdinteractiveauthentication", + "gssapiauthentication", + "hostbasedauthentication", + "hostbasedacceptedalgorithms", + "enablesshkeysign", + "usekeychain", + "casignaturealgorithms", + "nohostauthenticationforlocalhost", + "visualhostkey", + "requiredrsasize", + "fingerprinthash", + "forwardagent", + "forwardx11", + "gatewayports", + "permitremoteopen", + "forwardx11timeout", + "forwardx11trusted", + "connecttimeout", + "controlmaster", + "controlpath", + "controlpersist", + "escapechar", + "loglevel", + "syslogfacility", + "protocol", + "forkafterauthentication", + "stdinnull", + "cipher", + "fallbacktorsh", + "globalknownhostsfile2", + "rhostsauthentication", + "securitykeyprovider", + "userknownhostsfile2", + "useroaming", + "usersh", + "useprivilegedport", + ]; + let unimplemented = ACCEPTED_KEYWORDS .iter() .filter_map(|(keyword, canonical, support)| match support { - KeywordSupport::Delegated(issue) if keyword == canonical => { - Some((*keyword, *issue)) - } + KeywordSupport::Unimplemented if keyword == canonical => Some(*keyword), _ => None, }) - .collect::>(); + .collect::>(); - assert_eq!(delegated, expected); + assert_eq!(unimplemented, expected); } } diff --git a/src/ssh/ssh_config/path.rs b/src/ssh/ssh_config/path.rs index bc1ff31d..04f3ea07 100644 --- a/src/ssh/ssh_config/path.rs +++ b/src/ssh/ssh_config/path.rs @@ -20,6 +20,7 @@ use anyhow::Result; use std::path::PathBuf; +use super::diagnostic::{escape_field, escape_path}; use super::env_cache::GLOBAL_ENV_CACHE; /// Expand tilde and environment variables in a path (secure implementation) @@ -59,8 +60,8 @@ pub(super) fn expand_path_internal(path: &str) -> Result { } else { tracing::warn!( "Environment variable expansion failed for '{}': {}. Using original path.", - path_str, - e + escape_path(&path), + escape_field(&e.to_string()) ); Ok(path) } diff --git a/src/ssh/ssh_config/security/checks.rs b/src/ssh/ssh_config/security/checks.rs index 98c6b040..e4a79214 100644 --- a/src/ssh/ssh_config/security/checks.rs +++ b/src/ssh/ssh_config/security/checks.rs @@ -19,10 +19,13 @@ use anyhow::Result; use std::os::unix::fs::PermissionsExt; use std::path::Path; +use super::super::diagnostic::escape_path; + /// Validate security properties of identity files pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Result<()> { // Check for sensitive system paths let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to critical system files let sensitive_patterns = [ @@ -44,7 +47,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul for pattern in &sensitive_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: Identity file path '{path_str}' at line {line_number} points to sensitive system location. \ + "Security violation: Identity file path '{escaped_path}' at line {line_number} points to sensitive system location. \ Access to system files is not allowed for security reasons." ); } @@ -64,7 +67,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul tracing::warn!( "Security warning: Identity file '{}' at line {} is world-readable. \ Private SSH keys should not be readable by other users (chmod 600 recommended).", - path_str, + escaped_path, line_number ); } @@ -74,7 +77,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul tracing::warn!( "Security warning: Identity file '{}' at line {} is group-readable. \ Private SSH keys should only be readable by the owner (chmod 600 recommended).", - path_str, + escaped_path, line_number ); } @@ -82,7 +85,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul // Check if file is world-writable (very dangerous) if mode & 0o002 != 0 { anyhow::bail!( - "Security violation: Identity file '{path_str}' at line {line_number} is world-writable. \ + "Security violation: Identity file '{escaped_path}' at line {line_number} is world-writable. \ This is extremely dangerous and must be fixed immediately." ); } @@ -94,6 +97,7 @@ pub fn validate_identity_file_security(path: &Path, line_number: usize) -> Resul /// Validate security properties of known_hosts files pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Result<()> { let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to critical system files let sensitive_patterns = [ @@ -115,7 +119,7 @@ pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Re for pattern in &sensitive_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: Known hosts file path '{path_str}' at line {line_number} points to sensitive system location. \ + "Security violation: Known hosts file path '{escaped_path}' at line {line_number} points to sensitive system location. \ Access to system files is not allowed for security reasons." ); } @@ -134,7 +138,7 @@ pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Re tracing::warn!( "Security warning: Known hosts file '{}' at line {} is in an unusual location. \ Ensure this is intentional and the file is trustworthy.", - path_str, + escaped_path, line_number ); } @@ -145,6 +149,7 @@ pub fn validate_known_hosts_file_security(path: &Path, line_number: usize) -> Re /// Validate security properties of certificate files pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Result<()> { let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to critical system files that should never be certificates let forbidden_patterns = [ @@ -191,7 +196,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re // Check if it's exactly the private key name without certificate suffix if path_str.ends_with(pattern) || path_str.ends_with(&format!("{pattern}.pub")) { anyhow::bail!( - "Security violation: Certificate file path '{path_str}' at line {line_number} appears to be a private key or regular public key. \ + "Security violation: Certificate file path '{escaped_path}' at line {line_number} appears to be a private key or regular public key. \ SSH certificate files should end with '-cert.pub' or similar suffix. Use CertificateFile for certificates, not regular keys." ); } @@ -199,7 +204,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re } anyhow::bail!( - "Security violation: Certificate file path '{path_str}' at line {line_number} points to forbidden system location. \ + "Security violation: Certificate file path '{escaped_path}' at line {line_number} points to forbidden system location. \ System files and sensitive locations cannot be used as SSH certificates." ); } @@ -214,7 +219,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re tracing::warn!( "Security warning: Certificate file '{}' at line {} has an unusual extension. \ SSH certificates typically end with '.pub', '-cert.pub', '.pem', or '.crt'.", - path_str, + escaped_path, line_number ); } @@ -234,7 +239,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re tracing::warn!( "Security warning: Certificate file '{}' at line {} is in an unusual location. \ Ensure this is intentional and the file is a valid SSH certificate.", - path_str, + escaped_path, line_number ); } @@ -245,6 +250,7 @@ pub fn validate_certificate_file_security(path: &Path, line_number: usize) -> Re /// Validate security properties of general files pub fn validate_general_file_security(path: &Path, line_number: usize) -> Result<()> { let path_str = path.to_string_lossy(); + let escaped_path = escape_path(path); // Block access to the most critical system files let forbidden_patterns = [ @@ -267,7 +273,7 @@ pub fn validate_general_file_security(path: &Path, line_number: usize) -> Result for pattern in &forbidden_patterns { if path_str.contains(pattern) { anyhow::bail!( - "Security violation: File path '{path_str}' at line {line_number} points to forbidden system location. \ + "Security violation: File path '{escaped_path}' at line {line_number} points to forbidden system location. \ Access to this location is not allowed for security reasons." ); } diff --git a/src/ssh/ssh_config/security/path_validation.rs b/src/ssh/ssh_config/security/path_validation.rs index 946377c8..73b8b72b 100644 --- a/src/ssh/ssh_config/security/path_validation.rs +++ b/src/ssh/ssh_config/security/path_validation.rs @@ -18,6 +18,7 @@ use anyhow::{Context, Result}; use std::path::PathBuf; use super::checks; +use crate::ssh::ssh_config::diagnostic::{escape_field, escape_path}; use crate::ssh::ssh_config::path::expand_path_internal; /// Securely validate and expand a file path to prevent path traversal attacks @@ -40,8 +41,12 @@ use crate::ssh::ssh_config::path::expand_path_internal; /// * `Err(anyhow::Error)` if the path is unsafe or invalid pub fn secure_validate_path(path: &str, path_type: &str, line_number: usize) -> Result { // First expand the path using the existing logic - let expanded_path = expand_path_internal(path) - .with_context(|| format!("Failed to expand path '{path}' at line {line_number}"))?; + let expanded_path = expand_path_internal(path).with_context(|| { + format!( + "Failed to expand path '{}' at line {line_number}", + escape_field(path) + ) + })?; // Convert to string for analysis let path_str = expanded_path.to_string_lossy(); @@ -70,9 +75,9 @@ pub fn secure_validate_path(path: &str, path_type: &str, line_number: usize) -> tracing::debug!( "Could not canonicalize {} path '{}' at line {}: {}. Using expanded path as-is.", path_type, - path_str, + escape_path(&expanded_path), line_number, - e + escape_field(&e.to_string()) ); expanded_path.clone() } @@ -91,8 +96,8 @@ pub fn secure_validate_path(path: &str, path_type: &str, line_number: usize) -> || canonical_str.split('\\').any(|component| component == "..") { anyhow::bail!( - "Security violation: Canonicalized {path_type} path '{canonical_str}' contains parent directory references at line {line_number}. \ - This could indicate a path traversal attempt." + "Security violation: Canonicalized {path_type} path '{}' contains parent directory references at line {line_number}. This could indicate a path traversal attempt.", + escape_path(&canonical_path) ); } } diff --git a/tests/ssh_compat_output_test.rs b/tests/ssh_compat_output_test.rs index 2c29049f..069fce9c 100644 --- a/tests/ssh_compat_output_test.rs +++ b/tests/ssh_compat_output_test.rs @@ -97,6 +97,14 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( log.to_str().expect("UTF-8 log path"), "-F", config.to_str().expect("UTF-8 config path"), + "-o", + "ChallengeResponseAuthentication=no", + "-o", + "KbdInteractiveAuthentication=no", + "-o", + "DefinitelyUnknownOption=no", + "-o", + "AnotherUnknownOption=yes", "--connect-timeout=1", "--strict-host-key-checking=no", "127.0.0.1:1", @@ -106,6 +114,10 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( .expect("bssh should parse the ssh config"); assert!(!output.status.success()); + assert!( + output.stdout.is_empty(), + "config diagnostics leaked to stdout" + ); assert!(output.stderr.is_empty(), "-E diagnostics leaked to stderr"); let diagnostics = fs::read_to_string(log).expect("diagnostic log should exist"); @@ -117,6 +129,7 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( "Unsupported SSH config option 'securitykeyprovider' at {included}:4; bssh parses this value for inspection but does not implement its runtime behavior" ); let unknown = format!("Unknown SSH config option 'definitelyunknownoption' at {included}:5"); + let distinct_unknown = "Unknown SSH config option 'anotherunknownoption' at -o option #4"; assert_eq!( diagnostics.lines().filter(|line| line == &alias).count(), @@ -136,9 +149,103 @@ fn canonical_unimplemented_and_unknown_diagnostics_use_real_source_and_log_file( 1, "unknown diagnostic must be emitted once: {diagnostics:?}" ); + assert_eq!( + diagnostics + .lines() + .filter(|line| line == &distinct_unknown) + .count(), + 1, + "distinct unknown diagnostic must remain independent: {diagnostics:?}" + ); assert!(!diagnostics.contains("/spoofed/config")); } +#[cfg(unix)] +#[test] +fn config_diagnostics_escape_control_characters_in_paths_and_keywords() { + let directory = tempdir().expect("temporary directory should be created"); + let config = directory.path().join("ssh_config\nFORGED PATH"); + let log = directory.path().join("bssh.log"); + fs::write(&config, "Host *\nBad\u{1b}[31mKeyword yes\n") + .expect("maliciously named SSH config should be written"); + + let output = bssh() + .arg("-E") + .arg(&log) + .arg("-F") + .arg(&config) + .args([ + "--connect-timeout=1", + "--strict-host-key-checking=no", + "127.0.0.1:1", + "true", + ]) + .output() + .expect("bssh should parse the maliciously named SSH config"); + + assert!(!output.status.success()); + assert!(output.stdout.is_empty()); + assert!(output.stderr.is_empty()); + + let diagnostics = fs::read_to_string(log).expect("diagnostic log should exist"); + let escaped_path = config.to_string_lossy().replace('\n', "\\n"); + let expected = + format!("Unknown SSH config option 'bad\\u{{1b}}[31mkeyword' at {escaped_path}:2"); + assert_eq!( + diagnostics + .lines() + .filter(|line| line.starts_with("Unknown SSH config option")) + .collect::>(), + [expected], + "untrusted diagnostic fields must remain on one escaped line: {diagnostics:?}" + ); + assert!(!diagnostics.contains('\u{1b}')); + assert!( + !diagnostics + .lines() + .any(|line| line.starts_with("FORGED PATH")), + "config path forged a separate diagnostic line: {diagnostics:?}" + ); +} + +#[cfg(unix)] +#[test] +fn config_load_errors_escape_control_characters_in_nonexistent_paths() { + let directory = tempdir().expect("temporary directory should be created"); + let missing = directory.path().join("missing\nFORGED-ERROR\u{1b}[31m"); + let log = directory.path().join("bssh.log"); + + let output = bssh() + .arg("-E") + .arg(&log) + .arg("-F") + .arg(&missing) + .args(["127.0.0.1", "true"]) + .output() + .expect("bssh should report the missing malicious SSH config path"); + + assert_eq!(output.status.code(), Some(1)); + assert!(output.stdout.is_empty()); + assert!(output.stderr.is_empty()); + + let diagnostics = fs::read_to_string(log).expect("diagnostic log should exist"); + let escaped_path = missing + .to_string_lossy() + .replace('\n', "\\n") + .replace('\u{1b}', "\\u{1b}"); + assert!( + diagnostics.contains(&format!("Failed to canonicalize path: {escaped_path}")), + "escaped missing path is absent from diagnostic chain: {diagnostics:?}" + ); + assert!(!diagnostics.contains('\u{1b}')); + assert!( + !diagnostics + .lines() + .any(|line| line.starts_with("FORGED-ERROR")), + "missing config path forged a separate error line: {diagnostics:?}" + ); +} + #[test] fn connection_refused_is_actionable_and_exits_255() { let directory = tempdir().expect("temporary directory should be created");