From 672db3fda07023fdf6eff57dba82a7b434fce522 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Thu, 3 Sep 2026 16:09:12 +0900 Subject: [PATCH 1/3] fix(ssh): enable macOS UseKeychain runtime support Classify Apple's UseKeychain directive as a runtime-backed authentication option instead of emitting a false unsupported warning, and expose its resolved value in -G output. Match macOS behavior by falling back when a stored passphrase is stale and storing prompted passphrases only after successful key decryption. Document bssh's Keychain namespace and add parser, resolver, dump, and encrypted-key regression coverage. Tests: cargo fmt --all --check; cargo clippy -- -D warnings; cargo test --lib use_keychain; cargo test --test ssh_config_dump_test use_keychain_is_reported_as_runtime_supported --- ARCHITECTURE.md | 8 +++ README.md | 3 +- docs/man/bssh.1 | 9 ++- src/ssh/ssh_config/dump.rs | 2 + .../parser/options/authentication.rs | 6 +- src/ssh/ssh_config/parser/options/support.rs | 8 +-- src/ssh/ssh_config/parser/tests.rs | 1 + src/ssh/ssh_config/resolver_tests.rs | 18 +++--- src/ssh/ssh_config/types.rs | 5 +- src/ssh/tokio_client/authentication.rs | 56 +++++++++++++++++-- tests/ssh_config_dump_test.rs | 21 +++++++ 11 files changed, 110 insertions(+), 27 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 79a222de..99c98886 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -855,6 +855,14 @@ but not with themselves; omit the key (plain `#[serial]`) when in doubt. - Password authentication (discouraged in production) - Public key authentication preferred +#### macOS `UseKeychain` + +On macOS, the resolved `UseKeychain yes` SSH configuration option enables passphrase lookup and storage for encrypted private-key authentication. Keychain access is deferred until an encrypted key actually needs a passphrase, is skipped entirely by `BatchMode yes`, and is serialized with terminal passphrase prompts so parallel connections cannot display competing authentication UI. + +bssh owns generic-password records under the `bssh-ssh-key-passphrase` service and uses the canonical private-key path as the account. This namespace is intentionally separate from the Data Protection Keychain access group used by Apple's `/usr/bin/ssh`; that access group requires an Apple-only code-signing entitlement, so third-party binaries cannot reuse entries created by `ssh-add --apple-use-keychain`. A retrieved passphrase is accepted only if it decrypts the key. Missing or stale records fall back to the terminal prompt, and a newly entered passphrase is stored only after successful decryption. + +Other platforms accept the Apple-specific keyword for portable configuration parsing but ignore its runtime behavior and recommend `IgnoreUnknown UseKeychain` when the same file must also work with upstream OpenSSH clients. + ### Host Verification - known_hosts file verification diff --git a/README.md b/README.md index 30a58fae..a88122ed 100644 --- a/README.md +++ b/README.md @@ -975,7 +975,8 @@ These options provide essential authentication management, security enforcement, **Platform Notes:** - **UseKeychain** is an Apple-specific patch to OpenSSH and only available on macOS - Fully integrated with macOS Keychain via Security Framework for secure passphrase storage and retrieval -- Passphrases are automatically stored after successful authentication and retrieved from Keychain on subsequent connections +- Passphrases are stored only after they successfully decrypt the private key and are retrieved from Keychain on subsequent connections +- bssh stores passphrases in its own `bssh-ssh-key-passphrase` Keychain service. Apple's `/usr/bin/ssh` uses an entitlement-protected access group, so entries created by `ssh-add --apple-use-keychain` cannot be read directly by third-party binaries and the first bssh connection may still prompt once. - For cross-platform configurations, use `IgnoreUnknown UseKeychain` to prevent errors on non-macOS systems ### SSH Config Examples diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 8c823f90..0a5b9f7f 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -1013,7 +1013,14 @@ When enabled, passphrases are automatically retrieved from and stored in the mac .RS .IP \[bu] 2 .B Implementation: -Fully integrated with macOS Keychain via Security Framework. Passphrases are securely stored after successful authentication and retrieved on subsequent connections. +Fully integrated with macOS Keychain via Security Framework. Passphrases are securely stored after successful private-key decryption and retrieved on subsequent connections. +.IP \[bu] 2 +.B Interoperability: +bssh uses its own +.I bssh-ssh-key-passphrase +Keychain service. Entries created by +.I ssh-add --apple-use-keychain +belong to an Apple entitlement-protected access group and cannot be read directly by third-party binaries, so bssh may prompt once before storing its own entry. .IP \[bu] 2 .B Cross-platform compatibility: Use diff --git a/src/ssh/ssh_config/dump.rs b/src/ssh/ssh_config/dump.rs index df20ea68..21113d67 100644 --- a/src/ssh/ssh_config/dump.rs +++ b/src/ssh/ssh_config/dump.rs @@ -78,6 +78,8 @@ pub fn render_resolved_config(original_host: &str, config: &SshHostConfig) -> Re config.hostbased_authentication.unwrap_or(false), )?; output.bool("identitiesonly", config.identities_only.unwrap_or(false))?; + #[cfg(target_os = "macos")] + output.bool("usekeychain", config.use_keychain.unwrap_or(false))?; output.bool( "kbdinteractiveauthentication", config.keyboard_interactive_authentication.unwrap_or(true), diff --git a/src/ssh/ssh_config/parser/options/authentication.rs b/src/ssh/ssh_config/parser/options/authentication.rs index 54692e7c..956be6ef 100644 --- a/src/ssh/ssh_config/parser/options/authentication.rs +++ b/src/ssh/ssh_config/parser/options/authentication.rs @@ -406,11 +406,7 @@ pub(super) fn parse_authentication_option( #[cfg(target_os = "macos")] { if value { - tracing::debug!( - "UseKeychain enabled at line {} (Note: Currently supports parsing only. \ - Keychain integration will be implemented in a future release)", - line_number - ); + tracing::debug!("UseKeychain enabled at line {line_number}"); } host.use_keychain = Some(value); } diff --git a/src/ssh/ssh_config/parser/options/support.rs b/src/ssh/ssh_config/parser/options/support.rs index 0a7f7153..d494b2f8 100644 --- a/src/ssh/ssh_config/parser/options/support.rs +++ b/src/ssh/ssh_config/parser/options/support.rs @@ -109,7 +109,7 @@ pub(super) const ACCEPTED_KEYWORDS: &[(&str, &str, KeywordSupport)] = &[ Runtime(Authentication), ), ("enablesshkeysign", "enablesshkeysign", Unimplemented), - ("usekeychain", "usekeychain", Unimplemented), + ("usekeychain", "usekeychain", Runtime(Authentication)), ( "stricthostkeychecking", "stricthostkeychecking", @@ -306,8 +306,8 @@ mod tests { use std::collections::HashSet; const ACCEPTED_SPELLING_COUNT: usize = 108; - const RUNTIME_SPELLING_COUNT: usize = 58; - const UNIMPLEMENTED_SPELLING_COUNT: usize = 50; + const RUNTIME_SPELLING_COUNT: usize = 59; + const UNIMPLEMENTED_SPELLING_COUNT: usize = 49; #[test] fn accepted_keywords_and_aliases_have_one_consistent_classification() { @@ -360,6 +360,7 @@ mod tests { ("passwordauthentication", Authentication), ("preferredauthentications", Authentication), ("numberofpasswordprompts", Authentication), + ("usekeychain", Authentication), ("stricthostkeychecking", HostVerification), ("userknownhostsfile", HostVerification), ("globalknownhostsfile", HostVerification), @@ -434,7 +435,6 @@ mod tests { "hostbasedauthentication", "hostbasedacceptedalgorithms", "enablesshkeysign", - "usekeychain", "casignaturealgorithms", "nohostauthenticationforlocalhost", "visualhostkey", diff --git a/src/ssh/ssh_config/parser/tests.rs b/src/ssh/ssh_config/parser/tests.rs index c018542c..381cd5b6 100644 --- a/src/ssh/ssh_config/parser/tests.rs +++ b/src/ssh/ssh_config/parser/tests.rs @@ -1383,6 +1383,7 @@ Host example.com let hosts = parse(content).unwrap(); assert_eq!(hosts.len(), 1); assert_eq!(hosts[0].use_keychain, Some(true)); + assert!(!hosts[0].unimplemented_options.contains_key("usekeychain")); } #[test] diff --git a/src/ssh/ssh_config/resolver_tests.rs b/src/ssh/ssh_config/resolver_tests.rs index 72a06a76..267638c3 100644 --- a/src/ssh/ssh_config/resolver_tests.rs +++ b/src/ssh/ssh_config/resolver_tests.rs @@ -407,18 +407,18 @@ Host example.com #[test] #[cfg(target_os = "macos")] - fn test_use_keychain_override() { + fn test_use_keychain_host_specific_value_precedes_global_default() { let content = r#" -Host * - UseKeychain no - Host example.com UseKeychain yes + +Host * + UseKeychain no "#; let hosts = parse(content).unwrap(); let config = find_host_config(&hosts, "example.com"); - // The first matching block wins (yes) + // OpenSSH uses the first value obtained, so specific blocks precede defaults. assert_eq!(config.use_keychain, Some(true)); } @@ -443,8 +443,8 @@ Host example.com #[test] #[cfg(target_os = "macos")] - fn test_use_keychain_last_match_wins() { - // SSH config merges all matching blocks, with later values overriding earlier ones + fn test_use_keychain_first_match_wins() { + // SSH config merges all matching blocks while preserving the first value obtained. let content = r#" Host example.com UseKeychain yes @@ -455,8 +455,8 @@ Host example.com let hosts = parse(content).unwrap(); let config = find_host_config(&hosts, "example.com"); - // Should use the last matching value (no) due to merge logic - assert_eq!(config.use_keychain, Some(false)); + // OpenSSH keeps the first value obtained across matching blocks. + assert_eq!(config.use_keychain, Some(true)); } #[test] diff --git a/src/ssh/ssh_config/types.rs b/src/ssh/ssh_config/types.rs index 50f3c741..1db46073 100644 --- a/src/ssh/ssh_config/types.rs +++ b/src/ssh/ssh_config/types.rs @@ -179,9 +179,8 @@ pub struct SshHostConfig { pub identities_only: Option, pub add_keys_to_agent: Option, // yes/no/ask/confirm pub identity_agent: Option, // socket path or "none" - /// UseKeychain option (macOS only) - specifies whether to integrate with macOS Keychain - /// Note: This is an Apple-specific patch to OpenSSH. Currently supports parsing only. - /// Keychain integration will be implemented in a future release. + /// UseKeychain option (macOS only) - integrates private-key passphrases with macOS Keychain. + /// This is an Apple-specific OpenSSH extension. #[cfg(target_os = "macos")] pub use_keychain: Option, // Security & algorithm management diff --git a/src/ssh/tokio_client/authentication.rs b/src/ssh/tokio_client/authentication.rs index 4a3e04da..eb7e17e4 100644 --- a/src/ssh/tokio_client/authentication.rs +++ b/src/ssh/tokio_client/authentication.rs @@ -667,7 +667,6 @@ async fn load_policy_private_key( // authentication, and use of pre-collected credentials remain concurrent. let _prompt_guard = AUTH_PROMPT_MUTEX.lock().await; - #[cfg(target_os = "macos")] #[cfg(target_os = "macos")] if use_keychain { #[cfg(test)] @@ -680,8 +679,13 @@ async fn load_policy_private_key( .await; match retrieved { Ok(Some(passphrase)) => { - return russh::keys::load_secret_key(key_file_path, Some(&passphrase)) - .map_err(super::Error::KeyInvalid); + match russh::keys::load_secret_key(key_file_path, Some(&passphrase)) { + Ok(key) => return Ok(key), + Err(error) => tracing::warn!( + "Stored Keychain passphrase could not decrypt '{}': {error}; prompting for a replacement", + key_file_path.display() + ), + } } Ok(None) => {} Err(error @ super::Error::AuthenticationPromptTimeout { .. }) => return Err(error), @@ -709,6 +713,9 @@ async fn load_policy_private_key( .await?; let passphrase = Zeroizing::new(passphrase); + let key = russh::keys::load_secret_key(key_file_path, Some(&passphrase)) + .map_err(super::Error::KeyInvalid)?; + #[cfg(target_os = "macos")] if use_keychain { let stored = bounded_auth_prompt("macOS Keychain storage", async { @@ -725,7 +732,7 @@ async fn load_policy_private_key( } } - russh::keys::load_secret_key(key_file_path, Some(&passphrase)).map_err(super::Error::KeyInvalid) + Ok(key) } async fn default_hashes( handle: &mut Handle, @@ -1806,6 +1813,47 @@ mod policy_execution_tests { ); } + #[cfg(target_os = "macos")] + #[tokio::test] + #[serial_test::serial] + async fn use_keychain_decrypts_an_encrypted_policy_key() { + let directory = tempfile::TempDir::new().unwrap(); + let key_path = directory.path().join("encrypted-key"); + let expected = + russh::keys::PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let encrypted = expected + .clone() + .encrypt(&mut rand::rng(), "keychain-test-passphrase") + .unwrap(); + std::fs::write( + &key_path, + encrypted.to_openssh(LineEnding::LF).unwrap().as_bytes(), + ) + .unwrap(); + + if let Err(error) = + crate::ssh::keychain_macos::store_passphrase(&key_path, "keychain-test-passphrase") + .await + { + let message = format!("{error:#}"); + if message.contains("authorization was canceled") + || message.contains("Keychain access is denied") + || message.contains("Keychain is locked") + { + eprintln!("skipping Keychain-backed test: {message}"); + return; + } + panic!("failed to prepare Keychain-backed test: {message}"); + } + + let loaded = load_policy_private_key(&key_path, None, true, true).await; + let cleanup = crate::ssh::keychain_macos::delete_passphrase(&key_path).await; + let loaded = loaded.expect("UseKeychain should decrypt the configured key"); + cleanup.expect("test Keychain entry should be deleted"); + + assert_eq!(loaded.public_key(), expected.public_key()); + } + #[tokio::test(start_paused = true)] async fn password_prompts_have_a_typed_timeout_bound() { let error = serialized_bounded_auth_prompt("password", || async { diff --git a/tests/ssh_config_dump_test.rs b/tests/ssh_config_dump_test.rs index 3eaa144e..30057acb 100644 --- a/tests/ssh_config_dump_test.rs +++ b/tests/ssh_config_dump_test.rs @@ -64,6 +64,27 @@ fn dump_exits_without_proxy_agent_prompt_or_connection_side_effects() { assert!(stdout.contains("identityagent /missing/agent.sock\n")); } +#[cfg(target_os = "macos")] +#[test] +fn use_keychain_is_reported_as_runtime_supported() { + let directory = tempdir().expect("temporary directory should be created"); + let config = directory.path().join("config"); + fs::write(&config, "Host target\n UseKeychain yes\n").expect("config should be written"); + + let output = run(&["-G", "-F", path(&config), "target"]); + assert!( + output.status.success(), + "{}", + String::from_utf8_lossy(&output.stderr) + ); + assert!( + output.stderr.is_empty(), + "runtime-supported UseKeychain emitted a diagnostic: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(String::from_utf8_lossy(&output.stdout).contains("usekeychain yes\n")); +} + #[test] fn match_and_include_restore_parent_scope_for_destination() { let directory = tempdir().expect("temporary directory should be created"); From 57d4c055fbd72a9020641c146caf7cd61fadd517 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Tue, 20 Oct 2026 22:02:10 +0900 Subject: [PATCH 2/3] fix(ssh): harden Keychain passphrase fallback coverage Wrap terminal passphrases in Zeroizing inside the blocking worker so a result discarded after async timeout or cancellation is still cleared on drop. Extract the shared decrypt-before-store sequence and cover valid, missing, and stale cached credentials, incorrect or timed-out prompt input, and nonfatal storage failure without OS interaction. Document the existing blocking-UI cancellation limitation. Refs #316 --- ARCHITECTURE.md | 4 +- src/ssh/tokio_client/authentication.rs | 208 +++++++++++++++++++------ 2 files changed, 163 insertions(+), 49 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index aaaa3a61..3e0b46f9 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -858,10 +858,12 @@ but not with themselves; omit the key (plain `#[serial]`) when in doubt. #### macOS `UseKeychain` -On macOS, the resolved `UseKeychain yes` SSH configuration option enables passphrase lookup and storage for encrypted private-key authentication. Keychain access is deferred until an encrypted key actually needs a passphrase, is skipped entirely by `BatchMode yes`, and is serialized with terminal passphrase prompts so parallel connections cannot display competing authentication UI. +On macOS, the resolved `UseKeychain yes` SSH configuration option enables passphrase lookup and storage for encrypted private-key authentication. Keychain access is deferred until an encrypted key actually needs a passphrase, is skipped entirely by `BatchMode yes`, and is serialized with terminal passphrase prompts during normal completion. The existing authentication timeout bounds the async wait, not the lifetime of a blocking OS Keychain dialog or terminal read; cancellation does not forcibly dismiss those operations. bssh owns generic-password records under the `bssh-ssh-key-passphrase` service and uses the canonical private-key path as the account. This namespace is intentionally separate from the Data Protection Keychain access group used by Apple's `/usr/bin/ssh`; that access group requires an Apple-only code-signing entitlement, so third-party binaries cannot reuse entries created by `ssh-add --apple-use-keychain`. A retrieved passphrase is accepted only if it decrypts the key. Missing or stale records fall back to the terminal prompt, and a newly entered passphrase is stored only after successful decryption. +The cached-passphrase fallback and decrypt-before-store sequence share a platform-independent helper with injected prompt and storage operations, so stale credentials, rejected input, and storage failures are regression-tested without terminal or Keychain side effects. Terminal input is wrapped in `Zeroizing` inside the blocking worker before it is returned, including when the async caller has already timed out or been cancelled. + Other platforms accept the Apple-specific keyword for portable configuration parsing but ignore its runtime behavior and recommend `IgnoreUnknown UseKeychain` when the same file must also work with upstream OpenSSH clients. ### Host Verification diff --git a/src/ssh/tokio_client/authentication.rs b/src/ssh/tokio_client/authentication.rs index eb7e17e4..b0133b26 100644 --- a/src/ssh/tokio_client/authentication.rs +++ b/src/ssh/tokio_client/authentication.rs @@ -668,72 +668,105 @@ async fn load_policy_private_key( let _prompt_guard = AUTH_PROMPT_MUTEX.lock().await; #[cfg(target_os = "macos")] - if use_keychain { + let cached_passphrase = if use_keychain { #[cfg(test)] record_key_user_interaction(); - let retrieved = bounded_auth_prompt("macOS Keychain retrieval", async { + match bounded_auth_prompt("macOS Keychain retrieval", async { crate::ssh::keychain_macos::retrieve_passphrase(key_file_path) .await .map_err(|error| super::Error::AuthenticationPolicy(error.to_string())) }) - .await; - match retrieved { - Ok(Some(passphrase)) => { - match russh::keys::load_secret_key(key_file_path, Some(&passphrase)) { - Ok(key) => return Ok(key), - Err(error) => tracing::warn!( - "Stored Keychain passphrase could not decrypt '{}': {error}; prompting for a replacement", - key_file_path.display() - ), - } - } - Ok(None) => {} + .await + { + Ok(passphrase) => passphrase, Err(error @ super::Error::AuthenticationPromptTimeout { .. }) => return Err(error), - Err(error) => tracing::warn!( - "Failed to retrieve passphrase for '{}' from Keychain: {error}", - key_file_path.display() - ), + Err(error) => { + tracing::warn!( + "Failed to retrieve passphrase for '{}' from Keychain: {error}", + key_file_path.display() + ); + None + } } - } + } else { + None + }; #[cfg(not(target_os = "macos"))] - let _ = use_keychain; + let cached_passphrase = { + let _ = use_keychain; + None + }; - #[cfg(test)] - record_key_user_interaction(); - let display_path = key_file_path.display().to_string(); - let prompt_task = tokio::task::spawn_blocking(move || { - rpassword::prompt_password(format!("Enter passphrase for key {display_path}: ")) - }); - let passphrase = bounded_auth_prompt("private-key passphrase", async { - prompt_task + decrypt_with_passphrase_fallback( + key_file_path, + cached_passphrase, + || async { + #[cfg(test)] + record_key_user_interaction(); + let display_path = key_file_path.display().to_string(); + let prompt_task = tokio::task::spawn_blocking(move || { + rpassword::prompt_password(format!("Enter passphrase for key {display_path}: ")) + .map(Zeroizing::new) + }); + bounded_auth_prompt("private-key passphrase", async { + prompt_task + .await + .map_err(super::Error::JoinError)? + .map_err(super::Error::IoError) + }) .await - .map_err(super::Error::JoinError)? - .map_err(super::Error::IoError) - }) - .await?; - let passphrase = Zeroizing::new(passphrase); - - let key = russh::keys::load_secret_key(key_file_path, Some(&passphrase)) - .map_err(super::Error::KeyInvalid)?; + }, + |passphrase| async move { + #[cfg(target_os = "macos")] + if use_keychain { + return bounded_auth_prompt("macOS Keychain storage", async { + crate::ssh::keychain_macos::store_passphrase(key_file_path, &passphrase) + .await + .map_err(|error| super::Error::AuthenticationPolicy(error.to_string())) + }) + .await; + } + let _ = passphrase; + Ok(()) + }, + ) + .await +} - #[cfg(target_os = "macos")] - if use_keychain { - let stored = bounded_auth_prompt("macOS Keychain storage", async { - crate::ssh::keychain_macos::store_passphrase(key_file_path, &passphrase) - .await - .map_err(|error| super::Error::AuthenticationPolicy(error.to_string())) - }) - .await; - if let Err(error) = stored { - tracing::warn!( - "Failed to store passphrase for '{}' in Keychain: {error}", +async fn decrypt_with_passphrase_fallback( + key_file_path: &std::path::Path, + cached_passphrase: Option>, + prompt: P, + store: S, +) -> Result +where + P: FnOnce() -> PF, + PF: std::future::Future, super::Error>>, + S: FnOnce(Zeroizing) -> SF, + SF: std::future::Future>, +{ + if let Some(passphrase) = cached_passphrase { + match russh::keys::load_secret_key(key_file_path, Some(&passphrase)) { + Ok(key) => return Ok(key), + Err(error) => tracing::warn!( + "Stored Keychain passphrase could not decrypt '{}': {error}; prompting for a replacement", key_file_path.display() - ); + ), } } + let passphrase = prompt().await?; + let key = russh::keys::load_secret_key(key_file_path, Some(&passphrase)) + .map_err(super::Error::KeyInvalid)?; + if let Err(error) = store(passphrase).await { + tracing::warn!( + "Failed to store passphrase for '{}' in Keychain: {error}", + key_file_path.display() + ); + } Ok(key) } + async fn default_hashes( handle: &mut Handle, algorithm: &russh::keys::ssh_key::Algorithm, @@ -1813,6 +1846,85 @@ mod policy_execution_tests { ); } + #[tokio::test] + async fn use_keychain_fallback_decrypts_before_storing() { + let directory = tempfile::TempDir::new().unwrap(); + let key_path = directory.path().join("encrypted-key"); + let expected = + russh::keys::PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let encrypted = expected + .clone() + .encrypt(&mut rand::rng(), "correct") + .unwrap(); + std::fs::write( + &key_path, + encrypted.to_openssh(LineEnding::LF).unwrap().as_bytes(), + ) + .unwrap(); + + // cached, entered, storage failure, success, prompts, stores + for (cached, entered, storage_fails, succeeds, prompts, stores) in [ + (Some("correct"), Some("unused"), false, true, 0, 0), + (Some("stale"), Some("correct"), false, true, 1, 1), + (None, Some("correct"), false, true, 1, 1), + (Some("stale"), Some("wrong"), false, false, 1, 0), + (None, Some("wrong"), false, false, 1, 0), + (Some("stale"), None, false, false, 1, 0), + (Some("stale"), Some("correct"), true, true, 1, 1), + ] { + let prompt_count = std::sync::atomic::AtomicUsize::new(0); + let store_count = std::sync::atomic::AtomicUsize::new(0); + let result = decrypt_with_passphrase_fallback( + &key_path, + cached.map(|value| Zeroizing::new(value.to_owned())), + || async { + prompt_count.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + entered.map(|value| Zeroizing::new(value.to_owned())).ok_or( + super::super::Error::AuthenticationPromptTimeout { + prompt: "test prompt", + seconds: 30, + }, + ) + }, + |value| { + store_count.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + assert_eq!(value.as_str(), "correct", "never persist invalid input"); + async move { + if storage_fails { + Err(super::super::Error::AuthenticationPolicy( + "storage denied".into(), + )) + } else { + Ok(()) + } + } + }, + ) + .await; + assert_eq!( + result.is_ok(), + succeeds, + "cached={cached:?}, entered={entered:?}" + ); + match result { + Ok(key) => assert_eq!(key.public_key(), expected.public_key()), + Err(super::super::Error::AuthenticationPromptTimeout { .. }) => { + assert!(entered.is_none()); + } + Err(super::super::Error::KeyInvalid(_)) => assert_eq!(entered, Some("wrong")), + Err(error) => panic!("unexpected failure: {error}"), + } + assert_eq!( + prompt_count.load(std::sync::atomic::Ordering::SeqCst), + prompts + ); + assert_eq!( + store_count.load(std::sync::atomic::Ordering::SeqCst), + stores + ); + } + } + #[cfg(target_os = "macos")] #[tokio::test] #[serial_test::serial] From 2922e2bd0ebad9fafe4f6ba9763a0720cce6a8b6 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Tue, 20 Oct 2026 22:18:28 +0900 Subject: [PATCH 3/3] test(ssh): correct platform-sensitive authentication regressions Isolate jump authentication tests from the user's HOME and assert the exact ordered explicit/default identity candidates, comparing canonical paths rather than assuming a single candidate. Keep both control-socket endpoints alive while checking peer credentials, and use the sleep executable shared by macOS and Linux so KnownHostsCommand exercises timeout handling instead of failing to spawn. Runtime authentication and peer-credential checks remain unchanged. Refs #316 --- src/jump/chain/auth.rs | 73 ++++++++++++--------- src/ssh/control/socket.rs | 8 ++- src/ssh/tokio_client/known_hosts_command.rs | 8 ++- 3 files changed, 55 insertions(+), 34 deletions(-) diff --git a/src/jump/chain/auth.rs b/src/jump/chain/auth.rs index 11cbf5a8..cc92f2a8 100644 --- a/src/jump/chain/auth.rs +++ b/src/jump/chain/auth.rs @@ -254,6 +254,10 @@ mod tests { let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let _home = EnvGuard::set("HOME", temp_dir.path()); + std::fs::create_dir_all(temp_dir.path().join(".ssh")).unwrap(); + let default_key_path = create_test_ssh_key(&temp_dir, ".ssh/id_ed25519"); + // Create jump host's own key let jump_key_path = create_test_ssh_key(&temp_dir, "jump_host_key"); let jump_key_str = jump_key_path.to_string_lossy().to_string(); @@ -283,23 +287,25 @@ mod tests { assert!(result.is_ok(), "Should succeed with jump host's key"); let auth_method = result.unwrap(); - // Verify it used the jump host's key, not the cluster key - match auth_method { - AuthMethod::PrivateKeyFileWithPolicy { key_file_path, .. } => { - let path_str = key_file_path.to_string_lossy(); - assert!( - path_str.contains("jump_host_key"), - "Should use jump host's key (jump_host_key), got: {path_str}" - ); - assert!( - !path_str.contains("cluster_key"), - "Should NOT use cluster key, got: {path_str}" - ); - } - other => { - panic!("Expected PrivateKeyFile, got {:?}", other); - } - } + // Default keys remain fallback candidates; the jump key must be first + // and the overridden cluster key must not be included at all. + let AuthMethod::Methods(methods) = auth_method else { + panic!("Expected explicit and default identity plan, got {auth_method:?}"); + }; + let paths: Vec<_> = methods + .iter() + .map(|method| match method { + AuthMethod::PrivateKeyFileWithPolicy { key_file_path, .. } => key_file_path, + other => panic!("Unexpected authentication candidate: {other:?}"), + }) + .collect(); + assert_eq!( + paths, + vec![ + &jump_key_path.canonicalize().unwrap(), + &default_key_path.canonicalize().unwrap() + ] + ); } /// Test: Falls back to cluster key when jump host has no ssh_key @@ -310,6 +316,9 @@ mod tests { let _sock = EnvGuard::remove("SSH_AUTH_SOCK"); let temp_dir = TempDir::new().expect("Failed to create temp dir"); + let _home = EnvGuard::set("HOME", temp_dir.path()); + std::fs::create_dir_all(temp_dir.path().join(".ssh")).unwrap(); + let default_key_path = create_test_ssh_key(&temp_dir, ".ssh/id_ed25519"); let cluster_key_path = create_test_ssh_key(&temp_dir, "cluster_key"); // Create jump host WITHOUT its own ssh_key @@ -332,18 +341,22 @@ mod tests { assert!(result.is_ok(), "Should succeed with cluster key"); let auth_method = result.unwrap(); - // Verify it used the cluster key - match auth_method { - AuthMethod::PrivateKeyFileWithPolicy { key_file_path, .. } => { - let path_str = key_file_path.to_string_lossy(); - assert!( - path_str.contains("cluster_key"), - "Should use cluster key, got: {path_str}" - ); - } - other => { - panic!("Expected PrivateKeyFile, got {:?}", other); - } - } + let AuthMethod::Methods(methods) = auth_method else { + panic!("Expected cluster and default identity plan, got {auth_method:?}"); + }; + let paths: Vec<_> = methods + .iter() + .map(|method| match method { + AuthMethod::PrivateKeyFileWithPolicy { key_file_path, .. } => key_file_path, + other => panic!("Unexpected authentication candidate: {other:?}"), + }) + .collect(); + assert_eq!( + paths, + vec![ + &cluster_key_path.canonicalize().unwrap(), + &default_key_path.canonicalize().unwrap() + ] + ); } } diff --git a/src/ssh/control/socket.rs b/src/ssh/control/socket.rs index 5677eabc..46306981 100644 --- a/src/ssh/control/socket.rs +++ b/src/ssh/control/socket.rs @@ -216,8 +216,12 @@ mod tests { let connect = connect_control_socket(&path); let accept = listener.accept(); let (client, accepted) = tokio::join!(connect, accept); - client.unwrap(); - verify_same_user(&accepted.unwrap().0).unwrap(); + // Keep both endpoints connected while querying peer credentials. + // macOS getpeereid can return ENOTCONN once the peer has closed. + let client = client.unwrap(); + let (server, _) = accepted.unwrap(); + verify_same_user(&server).unwrap(); + drop((client, server)); drop(guard); assert!(!path.exists()); diff --git a/src/ssh/tokio_client/known_hosts_command.rs b/src/ssh/tokio_client/known_hosts_command.rs index b585d641..9ce94bf0 100644 --- a/src/ssh/tokio_client/known_hosts_command.rs +++ b/src/ssh/tokio_client/known_hosts_command.rs @@ -413,9 +413,13 @@ mod tests { #[tokio::test] #[cfg(unix)] async fn timeout_is_fail_closed() { - let error = run_known_hosts_command("/usr/bin/sleep 10", &context()) + let error = run_known_hosts_command("/bin/sleep 10", &context()) .await .unwrap_err(); - assert!(error.to_string().contains("timed out")); + assert!( + matches!(error, Error::KnownHostsCommandFailed { .. }) + && error.to_string().contains("timed out"), + "expected timeout rather than spawn or execution failure: {error}" + ); } }