Skip to content

refactor: centralize SSH authentication logic into dedicated auth module #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
inureyes merged 2 commits into from
Oct 16, 2025
Merged

refactor: centralize SSH authentication logic into dedicated auth module #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb0204c
refactor: centralize SSH authentication logic into dedicated auth module
inureyes Oct 16, 2025
2a443be
fix(security): comprehensive security and performance improvements - …
inureyes Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 106 additions & 1 deletion ARCHITECTURE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,117 @@ let tasks: Vec<JoinHandle<Result<ExecutionResult>>> = nodes
**Security Implementation:**
- Host key verification with three modes:
- `StrictHostKeyChecking::Yes` - Strict verification using known_hosts
- `StrictHostKeyChecking::No` - Skip all verification
- `StrictHostKeyChecking::No` - Skip all verification
- `StrictHostKeyChecking::AcceptNew` - TOFU mode
- CLI flag `--strict-host-key-checking` with default "accept-new"
- Uses system known_hosts file (~/.ssh/known_hosts)
- SSH agent authentication with auto-detection

### 4.1 Authentication Module (`ssh/auth.rs`)

**Status:** Implemented (2025-10-17) as part of code deduplication refactoring (Issue #34)

**Design Motivation:**
Authentication logic was previously duplicated across multiple modules (`ssh/client.rs` and `commands/interactive.rs`) with ~90% code duplication. This created maintenance burden and potential for bugs when fixing authentication issues in one location but not the other.

**Refactoring Goals:**
- Eliminate ~15% code duplication across codebase
- Provide single source of truth for authentication
- Maintain consistent authentication behavior across all commands
- Improve testability with centralized tests
- Reduce maintenance cost for authentication logic

**Implementation:**
The `AuthContext` struct encapsulates all authentication parameters and provides a single `determine_method()` function that implements the standard authentication priority:

```rust
pub struct AuthContext {
pub key_path: Option<PathBuf>,
pub use_agent: bool,
pub use_password: bool,
pub username: String,
pub host: String,
}

impl AuthContext {
pub fn determine_method(&self) -> Result<AuthMethod> {
// Priority 1: Password authentication (if explicitly requested)
// Priority 2: SSH agent (if explicitly requested and available)
// Priority 3: Specified key file (if provided)
// Priority 4: SSH agent auto-detection (if use_agent is true)
// Priority 5: Default key locations (~/.ssh/id_ed25519, ~/.ssh/id_rsa, etc.)
}
}
```

**Builder Pattern Integration:**
The context uses a fluent builder pattern for ergonomic configuration:

```rust
let auth_ctx = AuthContext::new(username, host)
.with_key_path(key_path.map(|p| p.to_path_buf()))
.with_agent(use_agent)
.with_password(use_password);

let auth_method = auth_ctx.determine_method()?;
```

**Security Features:**
- Uses `zeroize` crate to clear passwords and passphrases from memory
- Secure passphrase prompts via `rpassword` crate
- No credential caching or storage
- Platform-specific handling (SSH agent not supported on Windows)

**Code Reduction:**
- Eliminated ~130 lines of duplicated authentication logic
- Reduced from 2 implementations to 1 canonical implementation
- Client modules reduced from ~140 lines to ~10 lines for authentication

**Testing:**
Comprehensive test coverage including:
- Key file authentication
- SSH agent authentication (Unix only)
- Password authentication (manual test only)
- Default key location fallback
- Error conditions and edge cases

**Usage in Codebase:**
1. **`ssh/client.rs`**: Uses `AuthContext` for all SSH operations
```rust
fn determine_auth_method(&self, ...) -> Result<AuthMethod> {
let auth_ctx = super::auth::AuthContext::new(...)
.with_key_path(...)
.with_agent(...)
.with_password(...);
auth_ctx.determine_method()
}
```

2. **`commands/interactive.rs`**: Uses `AuthContext` for interactive sessions
```rust
fn determine_auth_method(&self, node: &Node) -> Result<AuthMethod> {
let auth_ctx = crate::ssh::AuthContext::new(...)
.with_key_path(...)
.with_agent(...)
.with_password(...);
auth_ctx.determine_method()
}
```

**Benefits Realized:**
- Single source of truth for authentication logic
- Easier to add new authentication methods
- Consistent behavior across all bssh commands
- Reduced bug surface area
- Improved code maintainability
- Better test coverage

**Future Enhancements:**
- Support for additional authentication methods (hardware tokens, certificates)
- Credential caching with secure storage integration
- Multi-factor authentication support
- Per-host authentication preferences

### 5. Connection Pooling (`ssh/pool.rs`)

**Current Status:** Placeholder implementation (Phase 3, 2025-08-21)
Expand Down
148 changes: 19 additions & 129 deletions src/commands/interactive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,12 @@ use rustyline::config::Configurer;
use rustyline::error::ReadlineError;
use rustyline::DefaultEditor;
use std::io::{self, Write};
use std::path::{Path, PathBuf};
use std::path::PathBuf;
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::Arc;
use tokio::sync::mpsc;
use tokio::sync::Mutex;
use tokio::time::{timeout, Duration};
use zeroize::Zeroizing;

use crate::config::{Config, InteractiveConfig};
use crate::node::Node;
Expand Down Expand Up @@ -325,7 +324,7 @@ impl InteractiveCommand {
use crate::jump::{parse_jump_hosts, JumpHostChain};

// Determine authentication method using the same logic as exec mode
let auth_method = self.determine_auth_method(&node)?;
let auth_method = self.determine_auth_method(&node).await?;

// Set up host key checking using the configured strict mode
let check_method = get_check_method(self.strict_mode);
Expand Down Expand Up @@ -513,7 +512,7 @@ impl InteractiveCommand {
use crate::jump::{parse_jump_hosts, JumpHostChain};

// Determine authentication method using the same logic as exec mode
let auth_method = self.determine_auth_method(&node)?;
let auth_method = self.determine_auth_method(&node).await?;

// Set up host key checking using the configured strict mode
let check_method = get_check_method(self.strict_mode);
Expand Down Expand Up @@ -640,134 +639,25 @@ impl InteractiveCommand {
}

/// Determine authentication method based on node and config (same logic as exec mode)
fn determine_auth_method(&self, node: &Node) -> Result<AuthMethod> {
// If password authentication is explicitly requested
if self.use_password {
tracing::debug!("Using password authentication");
// Use Zeroizing to ensure password is cleared from memory when dropped
let password = Zeroizing::new(
rpassword::prompt_password(format!(
"Enter password for {}@{}: ",
node.username, node.host
))
.with_context(|| "Failed to read password")?,
);
return Ok(AuthMethod::with_password(&password));
}

// If SSH agent is explicitly requested, try that first
if self.use_agent {
#[cfg(not(target_os = "windows"))]
{
// Check if SSH_AUTH_SOCK is available
if std::env::var("SSH_AUTH_SOCK").is_ok() {
tracing::debug!("Using SSH agent for authentication");
return Ok(AuthMethod::Agent);
}
tracing::warn!(
"SSH agent requested but SSH_AUTH_SOCK environment variable not set"
);
// Fall through to key file authentication
}
#[cfg(target_os = "windows")]
{
anyhow::bail!("SSH agent authentication is not supported on Windows");
}
}

// Try key file authentication
if let Some(ref key_path) = self.key_path {
tracing::debug!("Authenticating with key: {:?}", key_path);

// Check if the key is encrypted by attempting to read it
let key_contents = std::fs::read_to_string(key_path)
.with_context(|| format!("Failed to read SSH key file: {key_path:?}"))?;

let passphrase = if key_contents.contains("ENCRYPTED")
|| key_contents.contains("Proc-Type: 4,ENCRYPTED")
{
tracing::debug!("Detected encrypted SSH key, prompting for passphrase");
// Use Zeroizing for passphrase security
let pass = Zeroizing::new(
rpassword::prompt_password(format!("Enter passphrase for key {key_path:?}: "))
.with_context(|| "Failed to read passphrase")?,
);
Some(pass)
} else {
None
};

return Ok(AuthMethod::with_key_file(
key_path,
passphrase.as_ref().map(|p| p.as_str()),
));
}

// If no explicit key path, try SSH agent if available (auto-detect)
// Note: We skip auto-detection to avoid failures with empty SSH agents
// Only use agent if explicitly requested with use_agent flag
#[cfg(not(target_os = "windows"))]
if self.use_agent && std::env::var("SSH_AUTH_SOCK").is_ok() {
tracing::debug!("SSH agent explicitly requested and available");
return Ok(AuthMethod::Agent);
async fn determine_auth_method(&self, node: &Node) -> Result<AuthMethod> {
// Use centralized authentication logic from auth module
let mut auth_ctx = crate::ssh::AuthContext::new(node.username.clone(), node.host.clone())
.with_context(|| {
format!("Invalid credentials for {}@{}", node.username, node.host)
})?;

// Set key path if provided
if let Some(ref path) = self.key_path {
auth_ctx = auth_ctx
.with_key_path(Some(path.clone()))
.with_context(|| format!("Invalid SSH key path: {path:?}"))?;
}

// Fallback to default key locations
let home = std::env::var("HOME").unwrap_or_else(|_| ".".to_string());
let home_path = Path::new(&home).join(".ssh");

// Try common key files in order of preference
let default_keys = [
home_path.join("id_ed25519"),
home_path.join("id_rsa"),
home_path.join("id_ecdsa"),
home_path.join("id_dsa"),
];

for default_key in &default_keys {
if default_key.exists() {
tracing::debug!("Using default key: {:?}", default_key);

// Check if the key is encrypted
let key_contents = std::fs::read_to_string(default_key)
.with_context(|| format!("Failed to read SSH key file: {default_key:?}"))?;

let passphrase = if key_contents.contains("ENCRYPTED")
|| key_contents.contains("Proc-Type: 4,ENCRYPTED")
{
tracing::debug!("Detected encrypted SSH key, prompting for passphrase");
// Use Zeroizing for passphrase security
let pass = Zeroizing::new(
rpassword::prompt_password(format!(
"Enter passphrase for key {default_key:?}: "
))
.with_context(|| "Failed to read passphrase")?,
);
Some(pass)
} else {
None
};
auth_ctx = auth_ctx
.with_agent(self.use_agent)
.with_password(self.use_password);

return Ok(AuthMethod::with_key_file(
default_key,
passphrase.as_ref().map(|p| p.as_str()),
));
}
}

anyhow::bail!(
"SSH authentication failed: No authentication method available.\n\
Tried:\n\
- SSH agent: {}\n\
- SSH keys: {:?}\n\
Please ensure you have a valid SSH key or SSH agent running.",
if std::env::var("SSH_AUTH_SOCK").is_ok() {
"Available but no identities"
} else {
"Not available (SSH_AUTH_SOCK not set)"
},
default_keys
)
auth_ctx.determine_method().await
}

/// Run interactive mode with a single node
Expand Down
Loading