From 22842c4b4714fb379d406ba764d2d6021fe6c43c Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 16:59:43 +0800 Subject: [PATCH 01/10] feat(ssh): confirm targeted updates for changed host keys --- ...026-10-03-changed-host-key-confirmation.md | 67 +++++++ nebula_app/i18n/en-US.json | 5 +- nebula_app/i18n/zh-CN.json | 5 +- .../src/gpui_shell/workspace/ssh_dialog.rs | 32 +++ nebula_app/src/ssh_prompt.rs | 8 +- nebula_app/src/ssh_session.rs | 31 +++ nebula_app/src/ssh_session/host_key.rs | 187 ++++++++++++++++++ nebula_app/src/ssh_session/lifecycle/tests.rs | 26 +++ 8 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md create mode 100644 nebula_app/src/ssh_session/host_key.rs diff --git a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md new file mode 100644 index 000000000..4d088e659 --- /dev/null +++ b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md @@ -0,0 +1,67 @@ +# Changed SSH host-key confirmation + +## Status + +Proposed for review. + +## Context + +Issue #252 requests an explicit update when a known SSH server changes identity. +The existing russh check rejects changed keys, without a confirmation path. + +## Evidence + +russh 0.62.2 checks literal/hashed host records and reports `KeyChanged` for an +incoming algorithm with a different recorded key. Its line index excludes +comments; marker records are not interpreted as ordinary host entries. +`ssh-key::known_hosts::Entry` supplies the existing semantic parser. + +## Decision + +Keep the handshake refused until the existing bounded prompt receives explicit +trust and the changed record is saved successfully. Show host/port, old SHA256 +fingerprints, the new SHA256 fingerprint and the identity-change warning. + +Use the library parser, preserve unrelated original lines, and split only the +literal target from multi-host records. Preserve hashed names, comments, line +endings and other algorithms. Marked matching records and changed wildcard or +negated patterns fail closed; Pebrel does not implement certificate trust or +silently convert revocation into ordinary trust. + +Confirmation owns a file snapshot. The short application writer lock and two +byte comparisons reject concurrent edits observed before replacement. Stage a +regular sibling file, preserve file permissions, sync and use existing atomic +replacement. Reject symlinks rather than replacing a user's indirection. + +## Rejected alternatives + +- Automatically trusting a changed server bypasses identity verification. +- Deleting a reported line can remove other hosts and uses an unreliable index. +- Appending a literal record leaves the stale key authoritative. +- Reimplementing the complete OpenSSH parser adds unnecessary protocol policy. + +## Consequences + +Canceled, unattended and unavailable UI paths cannot update trust. Pattern and +marked records require external known_hosts administration. Invalid input fails +closed. Independent OpenSSH writers do not share Pebrel's lock: the comparison +is not a cross-tool atomic compare-and-swap. Native ACLs beyond Rust file +permissions are not claimed to be preserved. + +## Validation + +Active regressions cover isolated files, shared host lines, hashed names, ports, +other algorithms, comments/endings, markers, patterns and changed snapshots. +A real loopback SSH service rejects the old record, remains refused after +cancellation, and connects after explicit confirmation saves the new key. +The GPUI cancellation path and English/Chinese fingerprint formatting are covered. +All execution is delegated to GitHub Actions; local automated execution is prohibited. + +## Supersedes + +None. + +## Revisit when + +Safe wildcard replacement, certificate-authority policy, stronger cross-tool +coordination or platform ACL preservation is requested. diff --git a/nebula_app/i18n/en-US.json b/nebula_app/i18n/en-US.json index 52874d55b..dea5289bf 100644 --- a/nebula_app/i18n/en-US.json +++ b/nebula_app/i18n/en-US.json @@ -1292,5 +1292,8 @@ "failed": "Could not complete the operation: {detail}", "changed": "The package changed after inspection. Choose the file again before installing." } - } + }, + "ssh.changedHost.title": "SSH host identity changed", + "ssh.changedHost.description": "{endpoint}\n\nPreviously saved SHA256 fingerprint(s):\n{previous}\n\nNew SHA256 fingerprint:\n{fingerprint}\n\nThis may mean the server was replaced or an attacker is impersonating it. Verify the new fingerprint through a trusted channel. Updating replaces this host’s saved key; cancel keeps the previous trust record and refuses the connection.", + "ssh.changedHost.confirm": "Update key and connect" } diff --git a/nebula_app/i18n/zh-CN.json b/nebula_app/i18n/zh-CN.json index a61a8c5b9..5b4000df7 100644 --- a/nebula_app/i18n/zh-CN.json +++ b/nebula_app/i18n/zh-CN.json @@ -1292,5 +1292,8 @@ "failed": "操作未完成：{detail}", "changed": "主题包在检查后发生了变化。请重新选择文件，再确认安装。" } - } + }, + "ssh.changedHost.title": "SSH 主机身份已变更", + "ssh.changedHost.description": "{endpoint}\n\n之前保存的 SHA256 指纹：\n{previous}\n\n新的 SHA256 指纹：\n{fingerprint}\n\n这可能表示服务器已更换，也可能有攻击者冒充主机。请通过可信渠道核对新指纹。更新将替换此主机保存的密钥；取消将保留原信任记录并拒绝连接。", + "ssh.changedHost.confirm": "更新密钥并连接" } diff --git a/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs b/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs index fa89fd15c..7c285fcc0 100644 --- a/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs +++ b/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs @@ -22,6 +22,14 @@ pub(super) fn show(request: Arc, window: &mut Window, cx: &mut App) { false, false, ), + PromptKind::ChangedHostKey { host, port, previous, fingerprint } => ( + language.text(crate::i18n::Message::SshChangedHostTitle), + language.format(crate::i18n::Message::SshChangedHostDescription, &[ + ("endpoint", &format!("{host}:{port}")), ("previous", previous), ("fingerprint", fingerprint), + ]), + false, + false, + ), PromptKind::Secret { label, allow_save } => { (language.pick("SSH 身份验证", "SSH authentication"), label.clone(), true, *allow_save) }, @@ -58,6 +66,8 @@ pub(super) fn show(request: Arc, window: &mut Window, cx: &mut App) { description.clone(), if is_secret { language.pick("继续", "Continue") + } else if matches!(&request.kind, PromptKind::ChangedHostKey { .. }) { + language.text(crate::i18n::Message::SshChangedHostConfirm) } else { language.pick("信任并连接", "Trust and connect") }, @@ -115,6 +125,28 @@ mod tests { ); } + #[gpui::test] + fn changed_host_dialog_cancel_preserves_refusal(cx: &mut TestAppContext) { + cx.update(|cx| { gpui_component::init(cx); cx.set_reduce_motion(true); }); + let (_, cx) = cx.add_window_view(|window, cx| { + let view = cx.new(|_| DialogProbe); + Root::new(view, window, cx) + }); + let (request, mut response) = Prompt::for_test(PromptKind::ChangedHostKey { + host: "fixture.example".into(), port: 2200, + previous: "SHA256:previous".into(), fingerprint: "SHA256:new".into(), + }); + cx.update(|window, cx| { show(request, window, cx); let _ = window.draw(cx); }); + click(cx, "confirm-dialog-cancel"); + assert!(matches!(response.try_recv(), Ok(PromptResponse::Cancel))); + for language in [crate::i18n::UiLanguage::EnUs, crate::i18n::UiLanguage::ZhCn] { + let description = language.format(crate::i18n::Message::SshChangedHostDescription, &[ + ("endpoint", "fixture.example:2200"), ("previous", "SHA256:previous"), ("fingerprint", "SHA256:new"), + ]); + for value in ["fixture.example:2200", "SHA256:previous", "SHA256:new"] { assert!(description.contains(value)); } + } + } + #[gpui::test] fn ssh_dialog_cancel_does_not_accept_an_unknown_host(cx: &mut TestAppContext) { cx.update(|cx| { diff --git a/nebula_app/src/ssh_prompt.rs b/nebula_app/src/ssh_prompt.rs index 0ecd4f1dc..d57df39bd 100644 --- a/nebula_app/src/ssh_prompt.rs +++ b/nebula_app/src/ssh_prompt.rs @@ -7,6 +7,7 @@ use zeroize::Zeroizing; pub enum PromptKind { HostKey { host: String, port: u16, fingerprint: String }, + ChangedHostKey { host: String, port: u16, previous: String, fingerprint: String }, Secret { label: String, allow_save: bool }, } @@ -34,7 +35,7 @@ impl Prompt { pub fn respond(&self, response: PromptResponse) -> bool { let response = match (&self.kind, response) { - (PromptKind::HostKey { .. }, PromptResponse::Trust) => PromptResponse::Trust, + (PromptKind::HostKey { .. } | PromptKind::ChangedHostKey { .. }, PromptResponse::Trust) => PromptResponse::Trust, (PromptKind::Secret { allow_save, .. }, PromptResponse::Secret { value, save }) => { PromptResponse::Secret { value, save: save && *allow_save } }, @@ -98,6 +99,11 @@ pub async fn confirm_host(host: &str, port: u16, fingerprint: String) -> io::Res Ok(matches!(response, PromptResponse::Trust)) } +pub async fn confirm_changed_host(host: &str, port: u16, previous: String, fingerprint: String) -> io::Result { + let response = request(PromptKind::ChangedHostKey { host: host.to_owned(), port, previous, fingerprint }).await?; + Ok(matches!(response, PromptResponse::Trust)) +} + pub async fn secret( label: String, allow_save: bool, diff --git a/nebula_app/src/ssh_session.rs b/nebula_app/src/ssh_session.rs index 0fef64849..4a6379f8b 100644 --- a/nebula_app/src/ssh_session.rs +++ b/nebula_app/src/ssh_session.rs @@ -29,6 +29,7 @@ mod agent; mod config; mod exec; mod integration; +mod host_key; mod lifecycle; mod route; mod transcript; @@ -460,6 +461,23 @@ struct ClientHandler { } impl ClientHandler { + fn host_key_path(&self) -> io::Result { + #[cfg(test)] + if let Some(path) = &self.known_hosts_path { return Ok(path.clone()); } + home::home_dir().map(|home| home.join(".ssh/known_hosts")) + .ok_or_else(|| io::Error::other("SSH home directory unavailable")) + } + + async fn confirm_changed_key( + &self, + change: host_key::Change, + confirmation: impl std::future::Future, + ) -> io::Result { + if !self.allow_prompt || !self.handshake.confirm(confirmation).await { return Ok(false); } + change.save(&self.host_key_path()?)?; + Ok(true) + } + fn verify_host_key(&self, key: &ssh_key::PublicKey) -> Result { #[cfg(test)] if let Some(path) = self.known_hosts_path.as_deref() { @@ -503,6 +521,10 @@ impl client::Handler for ClientHandler { &mut self, server_public_key: &ssh_key::PublicKey, ) -> Result { + // Inspect marked records even on a first visit: russh's literal checker + // otherwise treats @revoked / @cert-authority entries as unknown hosts. + let path = self.host_key_path()?; + let inspected = host_key::inspect(&path, &self.host, self.port, server_public_key)?; match self.verify_host_key(server_public_key) { Ok(true) => Ok(true), Ok(false) => Ok(self @@ -511,6 +533,15 @@ impl client::Handler for ClientHandler { confirm_new_host(&self.host, self.port, server_public_key), ) .await), + Err(russh::keys::Error::KeyChanged { .. }) if self.allow_prompt => { + let change = inspected.ok_or_else(|| io::Error::other("No changed SSH host record"))?; + let confirmation = crate::ssh_prompt::confirm_changed_host( + &self.host, self.port, change.fingerprints.join("\n"), + server_public_key.fingerprint(ssh_key::HashAlg::Sha256).to_string(), + ); + let accepted = async { confirmation.await.unwrap_or(false) }; + Ok(self.confirm_changed_key(change, accepted).await?) + }, Err(err) => { warn!("SSH 主机密钥验证失败: {err}"); // Return the verification error to the terminal/test status. A blocking diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs new file mode 100644 index 000000000..549055aef --- /dev/null +++ b/nebula_app/src/ssh_session/host_key.rs @@ -0,0 +1,187 @@ +//! Changed-key updates preserve unrelated known_hosts bytes and require a fresh snapshot. +use std::io::{self, Write as _}; +use std::path::Path; + +use hmac::{Hmac, KeyInit as _, Mac as _}; +use russh::keys::ssh_key::{self, known_hosts::{Entry, HostPatterns}}; + +pub(super) struct Change { + snapshot: Vec, + replacement: Vec, + pub(super) fingerprints: Vec, +} + +fn denied(message: &str) -> io::Error { + io::Error::new(io::ErrorKind::PermissionDenied, message) +} + +fn matches(patterns: &HostPatterns, endpoint: &str) -> bool { + match patterns { + HostPatterns::HashedName { salt, hash } => { + let Ok(mut mac) = Hmac::::new_from_slice(salt) else { return false }; + mac.update(endpoint.as_bytes()); + mac.verify_slice(hash).is_ok() + }, + HostPatterns::Patterns(patterns) => { + let matches = |pattern: &str| { + let expression = regex::escape(pattern).replace("\\*", ".*").replace("\\?", "."); + regex::Regex::new(&format!("^{expression}$")).is_ok_and(|regex| regex.is_match(endpoint)) + }; + !patterns.iter().any(|pattern| pattern.strip_prefix('!').is_some_and(matches)) + && patterns.iter().any(|pattern| !pattern.starts_with('!') && matches(pattern)) + }, + } +} + +/// Marked entries are never downgraded to an ordinary trust prompt. Pattern-based +/// replacement is deliberately refused: a literal update cannot revoke a wildcard. +pub(super) fn inspect(path: &Path, host: &str, port: u16, key: &ssh_key::PublicKey) -> io::Result> { + let snapshot = match std::fs::read(path) { + Ok(bytes) => bytes, + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(error) => return Err(error), + }; + let source = std::str::from_utf8(&snapshot).map_err(|_| denied("known_hosts is not UTF-8"))?; + let endpoint = if port == 22 { host.to_owned() } else { format!("[{host}]:{port}") }; + let mut replacement = String::new(); + let mut fingerprints = Vec::new(); + for line in source.split_inclusive('\n') { + let content = line.trim_end_matches(['\r', '\n']); + let ending = &line[content.len()..]; + let normalized = content.split_whitespace().collect::>().join(" "); + if normalized.is_empty() || normalized.starts_with('#') { + replacement.push_str(line); + continue; + } + let entry: Entry = normalized.parse().map_err(|_| denied("Cannot safely parse known_hosts"))?; + if !matches(entry.host_patterns(), &endpoint) { + replacement.push_str(line); + continue; + } + if entry.marker().is_some() { + return Err(denied("SSH host has a revoked or certificate-authority record; update it outside Pebrel")); + } + if entry.public_key().algorithm() != key.algorithm() { + replacement.push_str(line); + continue; + } + if entry.public_key().key_data() == key.key_data() { + replacement.push_str(line); + continue; + } + let fingerprint = entry.public_key().fingerprint(ssh_key::HashAlg::Sha256).to_string(); + if !fingerprints.contains(&fingerprint) { fingerprints.push(fingerprint); } + let hosts = content.split_whitespace().next().expect("parsed host field"); + let prefix_length = content.find(hosts).expect("original host field"); + let suffix = &content[prefix_length + hosts.len()..]; + let remaining = match entry.host_patterns() { + HostPatterns::HashedName { .. } => Vec::new(), + HostPatterns::Patterns(patterns) => { + if patterns.iter().any(|pattern| pattern.contains(['*', '?']) || pattern.starts_with('!')) { + return Err(denied("Pattern-based SSH host records cannot be safely replaced")); + } + patterns.iter().filter(|pattern| *pattern != &endpoint).cloned().collect::>() + }, + }; + if !remaining.is_empty() { + replacement.push_str(&content[..prefix_length]); + replacement.push_str(&remaining.join(",")); + replacement.push_str(suffix); + replacement.push_str(if ending.is_empty() { "\n" } else { ending }); + } + let target = if matches!(entry.host_patterns(), HostPatterns::HashedName { .. }) { hosts } else { &endpoint }; + replacement.push_str(&content[..prefix_length]); + replacement.push_str(target); + replacement.push(' '); + let mut saved_key = key.clone(); + saved_key.set_comment(entry.public_key().comment().clone()); + replacement.push_str(&saved_key.to_openssh().map_err(io::Error::other)?); + replacement.push_str(ending); + } + if fingerprints.is_empty() { return Ok(None); } + Ok(Some(Change { snapshot, replacement: replacement.into_bytes(), fingerprints })) +} + +impl Change { + pub(super) fn save(self, path: &Path) -> io::Result<()> { + let _lock = crate::atomic_file::try_lock(path)?.ok_or_else(|| denied("known_hosts is busy"))?; + let metadata = std::fs::symlink_metadata(path)?; + if !metadata.is_file() || metadata.file_type().is_symlink() { + return Err(denied("Only a regular known_hosts file can be updated")); + } + if std::fs::read(path)? != self.snapshot { + return Err(denied("known_hosts changed during confirmation; reconnect to verify again")); + } + let mut temporary = tempfile::NamedTempFile::new_in(path.parent().ok_or_else(|| denied("No known_hosts directory"))?)?; + temporary.as_file().set_permissions(metadata.permissions())?; + temporary.write_all(&self.replacement)?; + temporary.as_file().sync_all()?; + // Independent OpenSSH writers do not share Pebrel's lock. Compare again + // immediately before replace; this is not a cross-process compare-and-swap. + if std::fs::read(path)? != self.snapshot { + return Err(denied("known_hosts changed during confirmation; reconnect to verify again")); + } + crate::atomic_file::replace(temporary.path(), path) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use base64::Engine as _; + use ssh_key::{Algorithm, PrivateKey}; + + #[test] + fn update_preserves_other_hosts_ports_algorithms_hashes_comments_and_endings() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("known_hosts"); + let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let other = PrivateKey::random(&mut rand::rng(), Algorithm::Ecdsa { curve: ssh_key::EcdsaCurve::NistP256 }).unwrap(); + let mut commented = old.public_key().clone(); + commented.set_comment("operator comment"); + let old_line = commented.to_openssh().unwrap(); + let endpoint = "[fixture.example]:2200"; + let salt = b"fixed regression salt"; + let mut mac = Hmac::::new_from_slice(salt).unwrap(); + mac.update(endpoint.as_bytes()); + let encoder = base64::engine::general_purpose::STANDARD; + let hash = format!("|1|{}|{}", encoder.encode(salt), encoder.encode(mac.finalize().into_bytes())); + let unrelated = format!("# retained comment\r\nfixture.example {}\r\n{endpoint} {}\r\n", old.public_key().to_openssh().unwrap(), other.public_key().to_openssh().unwrap()); + let source = format!("{unrelated}{endpoint},other.example {old_line}\r\n{hash} {old_line}"); + std::fs::write(&path, &source).unwrap(); + let change = inspect(&path, "fixture.example", 2200, new.public_key()).unwrap().unwrap(); + assert_eq!(change.fingerprints, [old.public_key().fingerprint(ssh_key::HashAlg::Sha256).to_string()]); + change.save(&path).unwrap(); + let actual = std::fs::read_to_string(&path).unwrap(); + assert!(actual.starts_with(&unrelated)); + assert!(actual.contains(&format!("other.example {old_line}\r\n"))); + assert!(actual.contains("operator comment")); + assert!(actual.lines().any(|line| line.starts_with(&hash))); + assert!(!actual.ends_with('\n')); + assert!(russh::keys::known_hosts::check_known_hosts_path("fixture.example", 2200, new.public_key(), &path).unwrap()); + assert!(russh::keys::known_hosts::check_known_hosts_path("other.example", 22, old.public_key(), &path).unwrap()); + assert!(russh::keys::known_hosts::check_known_hosts_path("fixture.example", 22, old.public_key(), &path).unwrap()); + } + + #[test] + fn stale_snapshot_marked_and_wildcard_records_cannot_be_updated() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("known_hosts"); + let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let source = format!("fixture.example {}\n", old.public_key().to_openssh().unwrap()); + std::fs::write(&path, &source).unwrap(); + let change = inspect(&path, "fixture.example", 22, new.public_key()).unwrap().unwrap(); + let concurrent = format!("{source}# another writer\n"); + std::fs::write(&path, &concurrent).unwrap(); + assert!(change.save(&path).is_err()); + assert_eq!(std::fs::read_to_string(&path).unwrap(), concurrent); + for hosts in ["@revoked fixture.example", "@cert-authority fixture.example", "*.example", "*.example,!excluded.example"] { + let source = format!("{hosts} {}\n", old.public_key().to_openssh().unwrap()); + std::fs::write(&path, &source).unwrap(); + assert!(inspect(&path, "fixture.example", 22, new.public_key()).is_err(), "{hosts}"); + assert_eq!(std::fs::read_to_string(&path).unwrap(), source); + } + } +} diff --git a/nebula_app/src/ssh_session/lifecycle/tests.rs b/nebula_app/src/ssh_session/lifecycle/tests.rs index ae77378fa..9bb10e1a9 100644 --- a/nebula_app/src/ssh_session/lifecycle/tests.rs +++ b/nebula_app/src/ssh_session/lifecycle/tests.rs @@ -764,3 +764,29 @@ fn hung_channel_does_not_lock_reuse_and_fresh_connection_can_retry() { fixture.forget(&replacement.session).await; }); } + +#[test] +fn changed_loopback_host_refuses_before_confirmation_and_connects_only_after_saved_update() { + check(async { + let fixture = Fixture::new(Mode::Open).await; + let path = fixture.route.known_hosts_path.as_ref().unwrap(); + let actual = std::fs::read_to_string(path).unwrap(); + let entry: russh::keys::ssh_key::known_hosts::Entry = actual.trim().parse().unwrap(); + let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let source = format!("{} {}\n", entry.host_patterns().to_string(), old.public_key().to_openssh().unwrap()); + std::fs::write(path, &source).unwrap(); + assert!(authenticated_route(&fixture.route, None::<&NoopSshEventHost>, false, true).await.is_err()); + assert_eq!(std::fs::read_to_string(path).unwrap(), source); + let destination = &fixture.route.destination; + let handler = super::super::ClientHandler { + host: "127.0.0.1".into(), port: destination.port, allow_prompt: true, + handshake: Handshake::default(), known_hosts_path: Some(path.clone()), + }; + let change = || super::super::host_key::inspect(path, "127.0.0.1", destination.port, entry.public_key()).unwrap().unwrap(); + assert!(!handler.confirm_changed_key(change(), async { false }).await.unwrap()); + assert_eq!(std::fs::read_to_string(path).unwrap(), source); + assert!(handler.confirm_changed_key(change(), async { true }).await.unwrap()); + let acquired = fixture.connect().await; + fixture.forget(&acquired.session).await; + }); +} From 6c9fc738f234872d29cf5315fbae62a989258e57 Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:02:30 +0800 Subject: [PATCH 02/10] style(ssh): apply Actions rustfmt output --- .../src/gpui_shell/workspace/ssh_dialog.rs | 42 +++++-- nebula_app/src/ssh_prompt.rs | 16 ++- nebula_app/src/ssh_session.rs | 20 +++- nebula_app/src/ssh_session/host_key.rs | 113 ++++++++++++++---- nebula_app/src/ssh_session/lifecycle/tests.rs | 25 +++- 5 files changed, 170 insertions(+), 46 deletions(-) diff --git a/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs b/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs index 7c285fcc0..ba3403843 100644 --- a/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs +++ b/nebula_app/src/gpui_shell/workspace/ssh_dialog.rs @@ -24,9 +24,14 @@ pub(super) fn show(request: Arc, window: &mut Window, cx: &mut App) { ), PromptKind::ChangedHostKey { host, port, previous, fingerprint } => ( language.text(crate::i18n::Message::SshChangedHostTitle), - language.format(crate::i18n::Message::SshChangedHostDescription, &[ - ("endpoint", &format!("{host}:{port}")), ("previous", previous), ("fingerprint", fingerprint), - ]), + language.format( + crate::i18n::Message::SshChangedHostDescription, + &[ + ("endpoint", &format!("{host}:{port}")), + ("previous", previous), + ("fingerprint", fingerprint), + ], + ), false, false, ), @@ -127,23 +132,38 @@ mod tests { #[gpui::test] fn changed_host_dialog_cancel_preserves_refusal(cx: &mut TestAppContext) { - cx.update(|cx| { gpui_component::init(cx); cx.set_reduce_motion(true); }); + cx.update(|cx| { + gpui_component::init(cx); + cx.set_reduce_motion(true); + }); let (_, cx) = cx.add_window_view(|window, cx| { let view = cx.new(|_| DialogProbe); Root::new(view, window, cx) }); let (request, mut response) = Prompt::for_test(PromptKind::ChangedHostKey { - host: "fixture.example".into(), port: 2200, - previous: "SHA256:previous".into(), fingerprint: "SHA256:new".into(), + host: "fixture.example".into(), + port: 2200, + previous: "SHA256:previous".into(), + fingerprint: "SHA256:new".into(), + }); + cx.update(|window, cx| { + show(request, window, cx); + let _ = window.draw(cx); }); - cx.update(|window, cx| { show(request, window, cx); let _ = window.draw(cx); }); click(cx, "confirm-dialog-cancel"); assert!(matches!(response.try_recv(), Ok(PromptResponse::Cancel))); for language in [crate::i18n::UiLanguage::EnUs, crate::i18n::UiLanguage::ZhCn] { - let description = language.format(crate::i18n::Message::SshChangedHostDescription, &[ - ("endpoint", "fixture.example:2200"), ("previous", "SHA256:previous"), ("fingerprint", "SHA256:new"), - ]); - for value in ["fixture.example:2200", "SHA256:previous", "SHA256:new"] { assert!(description.contains(value)); } + let description = language.format( + crate::i18n::Message::SshChangedHostDescription, + &[ + ("endpoint", "fixture.example:2200"), + ("previous", "SHA256:previous"), + ("fingerprint", "SHA256:new"), + ], + ); + for value in ["fixture.example:2200", "SHA256:previous", "SHA256:new"] { + assert!(description.contains(value)); + } } } diff --git a/nebula_app/src/ssh_prompt.rs b/nebula_app/src/ssh_prompt.rs index d57df39bd..d815b3ba7 100644 --- a/nebula_app/src/ssh_prompt.rs +++ b/nebula_app/src/ssh_prompt.rs @@ -35,7 +35,10 @@ impl Prompt { pub fn respond(&self, response: PromptResponse) -> bool { let response = match (&self.kind, response) { - (PromptKind::HostKey { .. } | PromptKind::ChangedHostKey { .. }, PromptResponse::Trust) => PromptResponse::Trust, + ( + PromptKind::HostKey { .. } | PromptKind::ChangedHostKey { .. }, + PromptResponse::Trust, + ) => PromptResponse::Trust, (PromptKind::Secret { allow_save, .. }, PromptResponse::Secret { value, save }) => { PromptResponse::Secret { value, save: save && *allow_save } }, @@ -99,8 +102,15 @@ pub async fn confirm_host(host: &str, port: u16, fingerprint: String) -> io::Res Ok(matches!(response, PromptResponse::Trust)) } -pub async fn confirm_changed_host(host: &str, port: u16, previous: String, fingerprint: String) -> io::Result { - let response = request(PromptKind::ChangedHostKey { host: host.to_owned(), port, previous, fingerprint }).await?; +pub async fn confirm_changed_host( + host: &str, + port: u16, + previous: String, + fingerprint: String, +) -> io::Result { + let response = + request(PromptKind::ChangedHostKey { host: host.to_owned(), port, previous, fingerprint }) + .await?; Ok(matches!(response, PromptResponse::Trust)) } diff --git a/nebula_app/src/ssh_session.rs b/nebula_app/src/ssh_session.rs index 4a6379f8b..0d85b616b 100644 --- a/nebula_app/src/ssh_session.rs +++ b/nebula_app/src/ssh_session.rs @@ -28,8 +28,8 @@ use crate::proxy_test::{ProxyTestFailure, ProxyTestOutcome, ProxyTestResult, Pro mod agent; mod config; mod exec; -mod integration; mod host_key; +mod integration; mod lifecycle; mod route; mod transcript; @@ -463,8 +463,11 @@ struct ClientHandler { impl ClientHandler { fn host_key_path(&self) -> io::Result { #[cfg(test)] - if let Some(path) = &self.known_hosts_path { return Ok(path.clone()); } - home::home_dir().map(|home| home.join(".ssh/known_hosts")) + if let Some(path) = &self.known_hosts_path { + return Ok(path.clone()); + } + home::home_dir() + .map(|home| home.join(".ssh/known_hosts")) .ok_or_else(|| io::Error::other("SSH home directory unavailable")) } @@ -473,7 +476,9 @@ impl ClientHandler { change: host_key::Change, confirmation: impl std::future::Future, ) -> io::Result { - if !self.allow_prompt || !self.handshake.confirm(confirmation).await { return Ok(false); } + if !self.allow_prompt || !self.handshake.confirm(confirmation).await { + return Ok(false); + } change.save(&self.host_key_path()?)?; Ok(true) } @@ -534,9 +539,12 @@ impl client::Handler for ClientHandler { ) .await), Err(russh::keys::Error::KeyChanged { .. }) if self.allow_prompt => { - let change = inspected.ok_or_else(|| io::Error::other("No changed SSH host record"))?; + let change = + inspected.ok_or_else(|| io::Error::other("No changed SSH host record"))?; let confirmation = crate::ssh_prompt::confirm_changed_host( - &self.host, self.port, change.fingerprints.join("\n"), + &self.host, + self.port, + change.fingerprints.join("\n"), server_public_key.fingerprint(ssh_key::HashAlg::Sha256).to_string(), ); let accepted = async { confirmation.await.unwrap_or(false) }; diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs index 549055aef..06f331540 100644 --- a/nebula_app/src/ssh_session/host_key.rs +++ b/nebula_app/src/ssh_session/host_key.rs @@ -3,7 +3,10 @@ use std::io::{self, Write as _}; use std::path::Path; use hmac::{Hmac, KeyInit as _, Mac as _}; -use russh::keys::ssh_key::{self, known_hosts::{Entry, HostPatterns}}; +use russh::keys::ssh_key::{ + self, + known_hosts::{Entry, HostPatterns}, +}; pub(super) struct Change { snapshot: Vec, @@ -25,7 +28,8 @@ fn matches(patterns: &HostPatterns, endpoint: &str) -> bool { HostPatterns::Patterns(patterns) => { let matches = |pattern: &str| { let expression = regex::escape(pattern).replace("\\*", ".*").replace("\\?", "."); - regex::Regex::new(&format!("^{expression}$")).is_ok_and(|regex| regex.is_match(endpoint)) + regex::Regex::new(&format!("^{expression}$")) + .is_ok_and(|regex| regex.is_match(endpoint)) }; !patterns.iter().any(|pattern| pattern.strip_prefix('!').is_some_and(matches)) && patterns.iter().any(|pattern| !pattern.starts_with('!') && matches(pattern)) @@ -35,7 +39,12 @@ fn matches(patterns: &HostPatterns, endpoint: &str) -> bool { /// Marked entries are never downgraded to an ordinary trust prompt. Pattern-based /// replacement is deliberately refused: a literal update cannot revoke a wildcard. -pub(super) fn inspect(path: &Path, host: &str, port: u16, key: &ssh_key::PublicKey) -> io::Result> { +pub(super) fn inspect( + path: &Path, + host: &str, + port: u16, + key: &ssh_key::PublicKey, +) -> io::Result> { let snapshot = match std::fs::read(path) { Ok(bytes) => bytes, Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), @@ -53,13 +62,16 @@ pub(super) fn inspect(path: &Path, host: &str, port: u16, key: &ssh_key::PublicK replacement.push_str(line); continue; } - let entry: Entry = normalized.parse().map_err(|_| denied("Cannot safely parse known_hosts"))?; + let entry: Entry = + normalized.parse().map_err(|_| denied("Cannot safely parse known_hosts"))?; if !matches(entry.host_patterns(), &endpoint) { replacement.push_str(line); continue; } if entry.marker().is_some() { - return Err(denied("SSH host has a revoked or certificate-authority record; update it outside Pebrel")); + return Err(denied( + "SSH host has a revoked or certificate-authority record; update it outside Pebrel", + )); } if entry.public_key().algorithm() != key.algorithm() { replacement.push_str(line); @@ -70,14 +82,19 @@ pub(super) fn inspect(path: &Path, host: &str, port: u16, key: &ssh_key::PublicK continue; } let fingerprint = entry.public_key().fingerprint(ssh_key::HashAlg::Sha256).to_string(); - if !fingerprints.contains(&fingerprint) { fingerprints.push(fingerprint); } + if !fingerprints.contains(&fingerprint) { + fingerprints.push(fingerprint); + } let hosts = content.split_whitespace().next().expect("parsed host field"); let prefix_length = content.find(hosts).expect("original host field"); let suffix = &content[prefix_length + hosts.len()..]; let remaining = match entry.host_patterns() { HostPatterns::HashedName { .. } => Vec::new(), HostPatterns::Patterns(patterns) => { - if patterns.iter().any(|pattern| pattern.contains(['*', '?']) || pattern.starts_with('!')) { + if patterns + .iter() + .any(|pattern| pattern.contains(['*', '?']) || pattern.starts_with('!')) + { return Err(denied("Pattern-based SSH host records cannot be safely replaced")); } patterns.iter().filter(|pattern| *pattern != &endpoint).cloned().collect::>() @@ -89,7 +106,11 @@ pub(super) fn inspect(path: &Path, host: &str, port: u16, key: &ssh_key::PublicK replacement.push_str(suffix); replacement.push_str(if ending.is_empty() { "\n" } else { ending }); } - let target = if matches!(entry.host_patterns(), HostPatterns::HashedName { .. }) { hosts } else { &endpoint }; + let target = if matches!(entry.host_patterns(), HostPatterns::HashedName { .. }) { + hosts + } else { + &endpoint + }; replacement.push_str(&content[..prefix_length]); replacement.push_str(target); replacement.push(' '); @@ -98,28 +119,37 @@ pub(super) fn inspect(path: &Path, host: &str, port: u16, key: &ssh_key::PublicK replacement.push_str(&saved_key.to_openssh().map_err(io::Error::other)?); replacement.push_str(ending); } - if fingerprints.is_empty() { return Ok(None); } + if fingerprints.is_empty() { + return Ok(None); + } Ok(Some(Change { snapshot, replacement: replacement.into_bytes(), fingerprints })) } impl Change { pub(super) fn save(self, path: &Path) -> io::Result<()> { - let _lock = crate::atomic_file::try_lock(path)?.ok_or_else(|| denied("known_hosts is busy"))?; + let _lock = + crate::atomic_file::try_lock(path)?.ok_or_else(|| denied("known_hosts is busy"))?; let metadata = std::fs::symlink_metadata(path)?; if !metadata.is_file() || metadata.file_type().is_symlink() { return Err(denied("Only a regular known_hosts file can be updated")); } if std::fs::read(path)? != self.snapshot { - return Err(denied("known_hosts changed during confirmation; reconnect to verify again")); + return Err(denied( + "known_hosts changed during confirmation; reconnect to verify again", + )); } - let mut temporary = tempfile::NamedTempFile::new_in(path.parent().ok_or_else(|| denied("No known_hosts directory"))?)?; + let mut temporary = tempfile::NamedTempFile::new_in( + path.parent().ok_or_else(|| denied("No known_hosts directory"))?, + )?; temporary.as_file().set_permissions(metadata.permissions())?; temporary.write_all(&self.replacement)?; temporary.as_file().sync_all()?; // Independent OpenSSH writers do not share Pebrel's lock. Compare again // immediately before replace; this is not a cross-process compare-and-swap. if std::fs::read(path)? != self.snapshot { - return Err(denied("known_hosts changed during confirmation; reconnect to verify again")); + return Err(denied( + "known_hosts changed during confirmation; reconnect to verify again", + )); } crate::atomic_file::replace(temporary.path(), path) } @@ -137,7 +167,11 @@ mod tests { let path = directory.path().join("known_hosts"); let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); - let other = PrivateKey::random(&mut rand::rng(), Algorithm::Ecdsa { curve: ssh_key::EcdsaCurve::NistP256 }).unwrap(); + let other = PrivateKey::random( + &mut rand::rng(), + Algorithm::Ecdsa { curve: ssh_key::EcdsaCurve::NistP256 }, + ) + .unwrap(); let mut commented = old.public_key().clone(); commented.set_comment("operator comment"); let old_line = commented.to_openssh().unwrap(); @@ -146,12 +180,20 @@ mod tests { let mut mac = Hmac::::new_from_slice(salt).unwrap(); mac.update(endpoint.as_bytes()); let encoder = base64::engine::general_purpose::STANDARD; - let hash = format!("|1|{}|{}", encoder.encode(salt), encoder.encode(mac.finalize().into_bytes())); - let unrelated = format!("# retained comment\r\nfixture.example {}\r\n{endpoint} {}\r\n", old.public_key().to_openssh().unwrap(), other.public_key().to_openssh().unwrap()); + let hash = + format!("|1|{}|{}", encoder.encode(salt), encoder.encode(mac.finalize().into_bytes())); + let unrelated = format!( + "# retained comment\r\nfixture.example {}\r\n{endpoint} {}\r\n", + old.public_key().to_openssh().unwrap(), + other.public_key().to_openssh().unwrap() + ); let source = format!("{unrelated}{endpoint},other.example {old_line}\r\n{hash} {old_line}"); std::fs::write(&path, &source).unwrap(); let change = inspect(&path, "fixture.example", 2200, new.public_key()).unwrap().unwrap(); - assert_eq!(change.fingerprints, [old.public_key().fingerprint(ssh_key::HashAlg::Sha256).to_string()]); + assert_eq!( + change.fingerprints, + [old.public_key().fingerprint(ssh_key::HashAlg::Sha256).to_string()] + ); change.save(&path).unwrap(); let actual = std::fs::read_to_string(&path).unwrap(); assert!(actual.starts_with(&unrelated)); @@ -159,9 +201,33 @@ mod tests { assert!(actual.contains("operator comment")); assert!(actual.lines().any(|line| line.starts_with(&hash))); assert!(!actual.ends_with('\n')); - assert!(russh::keys::known_hosts::check_known_hosts_path("fixture.example", 2200, new.public_key(), &path).unwrap()); - assert!(russh::keys::known_hosts::check_known_hosts_path("other.example", 22, old.public_key(), &path).unwrap()); - assert!(russh::keys::known_hosts::check_known_hosts_path("fixture.example", 22, old.public_key(), &path).unwrap()); + assert!( + russh::keys::known_hosts::check_known_hosts_path( + "fixture.example", + 2200, + new.public_key(), + &path + ) + .unwrap() + ); + assert!( + russh::keys::known_hosts::check_known_hosts_path( + "other.example", + 22, + old.public_key(), + &path + ) + .unwrap() + ); + assert!( + russh::keys::known_hosts::check_known_hosts_path( + "fixture.example", + 22, + old.public_key(), + &path + ) + .unwrap() + ); } #[test] @@ -177,7 +243,12 @@ mod tests { std::fs::write(&path, &concurrent).unwrap(); assert!(change.save(&path).is_err()); assert_eq!(std::fs::read_to_string(&path).unwrap(), concurrent); - for hosts in ["@revoked fixture.example", "@cert-authority fixture.example", "*.example", "*.example,!excluded.example"] { + for hosts in [ + "@revoked fixture.example", + "@cert-authority fixture.example", + "*.example", + "*.example,!excluded.example", + ] { let source = format!("{hosts} {}\n", old.public_key().to_openssh().unwrap()); std::fs::write(&path, &source).unwrap(); assert!(inspect(&path, "fixture.example", 22, new.public_key()).is_err(), "{hosts}"); diff --git a/nebula_app/src/ssh_session/lifecycle/tests.rs b/nebula_app/src/ssh_session/lifecycle/tests.rs index 9bb10e1a9..1668a6a66 100644 --- a/nebula_app/src/ssh_session/lifecycle/tests.rs +++ b/nebula_app/src/ssh_session/lifecycle/tests.rs @@ -773,16 +773,31 @@ fn changed_loopback_host_refuses_before_confirmation_and_connects_only_after_sav let actual = std::fs::read_to_string(path).unwrap(); let entry: russh::keys::ssh_key::known_hosts::Entry = actual.trim().parse().unwrap(); let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); - let source = format!("{} {}\n", entry.host_patterns().to_string(), old.public_key().to_openssh().unwrap()); + let source = format!( + "{} {}\n", + entry.host_patterns().to_string(), + old.public_key().to_openssh().unwrap() + ); std::fs::write(path, &source).unwrap(); - assert!(authenticated_route(&fixture.route, None::<&NoopSshEventHost>, false, true).await.is_err()); + assert!( + authenticated_route(&fixture.route, None::<&NoopSshEventHost>, false, true) + .await + .is_err() + ); assert_eq!(std::fs::read_to_string(path).unwrap(), source); let destination = &fixture.route.destination; let handler = super::super::ClientHandler { - host: "127.0.0.1".into(), port: destination.port, allow_prompt: true, - handshake: Handshake::default(), known_hosts_path: Some(path.clone()), + host: "127.0.0.1".into(), + port: destination.port, + allow_prompt: true, + handshake: Handshake::default(), + known_hosts_path: Some(path.clone()), + }; + let change = || { + super::super::host_key::inspect(path, "127.0.0.1", destination.port, entry.public_key()) + .unwrap() + .unwrap() }; - let change = || super::super::host_key::inspect(path, "127.0.0.1", destination.port, entry.public_key()).unwrap().unwrap(); assert!(!handler.confirm_changed_key(change(), async { false }).await.unwrap()); assert_eq!(std::fs::read_to_string(path).unwrap(), source); assert!(handler.confirm_changed_key(change(), async { true }).await.unwrap()); From 7617ed0774f6558f2284899cc10f85b0cde0cfa3 Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:07:02 +0800 Subject: [PATCH 03/10] fix(ssh): isolate host parsing and preserve Windows trust ACLs --- ...026-10-03-changed-host-key-confirmation.md | 13 ++- nebula_app/src/ssh_session/host_key.rs | 22 ++++- .../src/ssh_session/host_key/windows.rs | 83 +++++++++++++++++++ nebula_app/src/ssh_session/tests.rs | 18 ++++ 4 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 nebula_app/src/ssh_session/host_key/windows.rs diff --git a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md index 4d088e659..72a2f9706 100644 --- a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md +++ b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md @@ -31,7 +31,7 @@ silently convert revocation into ordinary trust. Confirmation owns a file snapshot. The short application writer lock and two byte comparisons reject concurrent edits observed before replacement. Stage a regular sibling file, preserve file permissions, sync and use existing atomic -replacement. Reject symlinks rather than replacing a user's indirection. +replacement on Unix and DACL-preserving replacement on Windows. Reject symlinks rather than replacing a user's indirection. ## Rejected alternatives @@ -45,13 +45,18 @@ replacement. Reject symlinks rather than replacing a user's indirection. Canceled, unattended and unavailable UI paths cannot update trust. Pattern and marked records require external known_hosts administration. Invalid input fails closed. Independent OpenSSH writers do not share Pebrel's lock: the comparison -is not a cross-tool atomic compare-and-swap. Native ACLs beyond Rust file -permissions are not claimed to be preserved. +is not a cross-tool atomic compare-and-swap. Windows uses `ReplaceFileW` without ACL/merge-error bypass flags, preserving +the target DACL. An explicit backup protects failure cases that move the original +file; recovery never overwrites a concurrent destination and retains the backup +if automatic recovery cannot complete. ## Validation Active regressions cover isolated files, shared host lines, hashed names, ports, other algorithms, comments/endings, markers, patterns and changed snapshots. +Unrelated malformed keys and unknown algorithms cannot block a trusted host. +Windows regressions compare a protected target DACL against a broader parent +and verify replacement failure retains the original file. A real loopback SSH service rejects the old record, remains refused after cancellation, and connects after explicit confirmation saves the new key. The GPUI cancellation path and English/Chinese fingerprint formatting are covered. @@ -64,4 +69,4 @@ None. ## Revisit when Safe wildcard replacement, certificate-authority policy, stronger cross-tool -coordination or platform ACL preservation is requested. +coordination is requested. diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs index 06f331540..d605938c6 100644 --- a/nebula_app/src/ssh_session/host_key.rs +++ b/nebula_app/src/ssh_session/host_key.rs @@ -8,6 +8,9 @@ use russh::keys::ssh_key::{ known_hosts::{Entry, HostPatterns}, }; +#[cfg(windows)] +mod windows; + pub(super) struct Change { snapshot: Vec, replacement: Vec, @@ -62,12 +65,19 @@ pub(super) fn inspect( replacement.push_str(line); continue; } - let entry: Entry = - normalized.parse().map_err(|_| denied("Cannot safely parse known_hosts"))?; - if !matches(entry.host_patterns(), &endpoint) { + let mut fields = content.split_whitespace(); + let first = fields.next().expect("nonempty record"); + let hosts = if first.starts_with('@') { + fields.next().ok_or_else(|| denied("Missing marked host patterns"))? + } else { + first + }; + let patterns: HostPatterns = hosts.parse().map_err(|_| denied("Cannot safely parse host patterns"))?; + if !matches(&patterns, &endpoint) { replacement.push_str(line); continue; } + let entry: Entry = normalized.parse().map_err(|_| denied("Cannot safely parse matching known_hosts record"))?; if entry.marker().is_some() { return Err(denied( "SSH host has a revoked or certificate-authority record; update it outside Pebrel", @@ -151,7 +161,11 @@ impl Change { "known_hosts changed during confirmation; reconnect to verify again", )); } - crate::atomic_file::replace(temporary.path(), path) + let temporary = temporary.into_temp_path(); + #[cfg(windows)] + return windows::replace(&temporary, path); + #[cfg(not(windows))] + crate::atomic_file::replace(&temporary, path) } } diff --git a/nebula_app/src/ssh_session/host_key/windows.rs b/nebula_app/src/ssh_session/host_key/windows.rs new file mode 100644 index 000000000..fbdec6247 --- /dev/null +++ b/nebula_app/src/ssh_session/host_key/windows.rs @@ -0,0 +1,83 @@ +//! Trust-file replacement preserves the target DACL and never ignores ACL merge errors. +use std::io; +use std::path::Path; +use std::os::windows::ffi::OsStrExt as _; +use windows_sys::Win32::Storage::FileSystem::ReplaceFileW; + +pub(super) fn replace(source: &Path, destination: &Path) -> io::Result<()> { + let backup = tempfile::Builder::new().prefix(".known-hosts-backup-") + .tempfile_in(destination.parent().ok_or_else(|| io::Error::other("No trust directory"))?)? + .into_temp_path(); + std::fs::remove_file(&backup)?; + let wide = |path: &Path| path.as_os_str().encode_wide().chain(Some(0)).collect::>(); + let original = wide(destination); + let replacement = wide(source); + let saved = wide(&backup); + // No IGNORE_ACL_ERRORS / IGNORE_MERGE_ERRORS: permission preservation is + // mandatory. A backup is required because error 1176 without one can remove + // the destination, and 1177 can move it to the backup before returning failure. + let success = unsafe { + ReplaceFileW(original.as_ptr(), replacement.as_ptr(), saved.as_ptr(), 0, + std::ptr::null(), std::ptr::null()) + }; + if success != 0 { return Ok(()); } + let error = io::Error::last_os_error(); + if backup.exists() { + // An exclusive hard-link creation restores the original file identity + // and DACL if the destination was moved away, without overwriting any + // concurrent writer. If recovery cannot complete, retain the backup. + if !destination.exists() && std::fs::hard_link(&backup, destination).is_ok() { + return Err(error); + } + let preserved = backup.keep().map_err(io::Error::other)?; + return Err(io::Error::new(error.kind(), format!("{error}; original trust file retained at {}", preserved.display()))); + } + Err(error) +} + +#[cfg(test)] +mod tests { + use super::*; + use russh::keys::ssh_key::{Algorithm, PrivateKey}; + + fn acl(path: &Path, setup: bool) -> String { + let script = if setup { + r#"$ErrorActionPreference='Stop'; $p=$env:PEBREL_KNOWN_HOSTS; $parent=Split-Path $p; $a=Get-Acl $parent; $rule=[System.Security.AccessControl.FileSystemAccessRule]::new([System.Security.Principal.SecurityIdentifier]::new('S-1-1-0'),'Modify','ContainerInherit,ObjectInherit','None','Allow'); $a.AddAccessRule($rule); Set-Acl $parent $a; $a=Get-Acl $p; $a.SetAccessRuleProtection($true,$false); foreach($rule in @($a.Access)) { $a.RemoveAccessRuleSpecific($rule) }; $user=[System.Security.Principal.WindowsIdentity]::GetCurrent().User; $a.AddAccessRule([System.Security.AccessControl.FileSystemAccessRule]::new($user,'FullControl','Allow')); Set-Acl $p $a; $a=Get-Acl $p; $parentAcl=Get-Acl $parent; if($a.GetSecurityDescriptorSddlForm('Access') -eq $parentAcl.GetSecurityDescriptorSddlForm('Access')) { throw 'Target must have a stricter DACL than its parent' }; $a.GetSecurityDescriptorSddlForm('Access')"# + } else { + r#"$ErrorActionPreference='Stop'; (Get-Acl $env:PEBREL_KNOWN_HOSTS).GetSecurityDescriptorSddlForm('Access')"# + }; + let output = std::process::Command::new("powershell.exe") + .args(["-NoProfile", "-NonInteractive", "-Command", script]) + .env("PEBREL_KNOWN_HOSTS", path).output().unwrap(); + assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); + String::from_utf8(output.stdout).unwrap().trim().to_owned() + } + + #[test] + fn changed_trust_file_keeps_its_stricter_windows_dacl() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("known_hosts"); + let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + std::fs::write(&path, format!("fixture.example {}\n", old.public_key().to_openssh().unwrap())).unwrap(); + let before = acl(&path, true); + super::super::inspect(&path, "fixture.example", 22, new.public_key()).unwrap().unwrap().save(&path).unwrap(); + assert_eq!(acl(&path, false), before, "Parent's broader inherited DACL must not replace target DACL"); + assert!(russh::keys::known_hosts::check_known_hosts_path("fixture.example", 22, new.public_key(), &path).unwrap()); + } + + #[test] + fn locked_destination_failure_retains_original_trust_file() { + use std::os::windows::fs::OpenOptionsExt as _; + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("known_hosts"); + let replacement = directory.path().join("replacement"); + std::fs::write(&path, "original trust").unwrap(); + std::fs::write(&replacement, "changed trust").unwrap(); + let locked = std::fs::OpenOptions::new().read(true).share_mode(0).open(&path).unwrap(); + assert!(replace(&replacement, &path).is_err()); + drop(locked); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "original trust"); + assert_eq!(std::fs::read_to_string(&replacement).unwrap(), "changed trust"); + } +} diff --git a/nebula_app/src/ssh_session/tests.rs b/nebula_app/src/ssh_session/tests.rs index 21a74f54a..a63406b82 100644 --- a/nebula_app/src/ssh_session/tests.rs +++ b/nebula_app/src/ssh_session/tests.rs @@ -363,3 +363,21 @@ fn all_keys_failing_locally_is_not_reported_as_server_rejection() { assert!(partial.contains("服务器拒绝"), "实际文案: {partial}"); assert!(partial.contains("本地密钥问题"), "实际文案: {partial}"); } + +#[test] +fn trusted_host_is_not_blocked_by_unrelated_invalid_keys_or_unknown_algorithms() { + use russh::client::Handler as _; + use russh::keys::ssh_key::{Algorithm, PrivateKey}; + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("known_hosts"); + let key = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let source = format!("fixture.example {}\nother.example ssh-ed25519 NOT_BASE64\nunknown.example ssh-future-format opaque\n", key.public_key().to_openssh().unwrap()); + std::fs::write(&path, &source).unwrap(); + let mut handler = super::ClientHandler { + host: "fixture.example".into(), port: 22, allow_prompt: true, + handshake: super::lifecycle::Handshake::default(), known_hosts_path: Some(path.clone()), + }; + let runtime = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap(); + assert!(runtime.block_on(handler.check_server_key(key.public_key())).unwrap()); + assert_eq!(std::fs::read_to_string(&path).unwrap(), source); +} From f3f25ad9aa66b240a80ddb0b827363f57618b903 Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:09:09 +0800 Subject: [PATCH 04/10] style(ssh): align safety fix with remote rustfmt --- nebula_app/src/ssh_session/host_key.rs | 7 ++- .../src/ssh_session/host_key/windows.rs | 56 +++++++++++++++---- nebula_app/src/ssh_session/tests.rs | 12 +++- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs index d605938c6..df0c40b75 100644 --- a/nebula_app/src/ssh_session/host_key.rs +++ b/nebula_app/src/ssh_session/host_key.rs @@ -72,12 +72,15 @@ pub(super) fn inspect( } else { first }; - let patterns: HostPatterns = hosts.parse().map_err(|_| denied("Cannot safely parse host patterns"))?; + let patterns: HostPatterns = + hosts.parse().map_err(|_| denied("Cannot safely parse host patterns"))?; if !matches(&patterns, &endpoint) { replacement.push_str(line); continue; } - let entry: Entry = normalized.parse().map_err(|_| denied("Cannot safely parse matching known_hosts record"))?; + let entry: Entry = normalized + .parse() + .map_err(|_| denied("Cannot safely parse matching known_hosts record"))?; if entry.marker().is_some() { return Err(denied( "SSH host has a revoked or certificate-authority record; update it outside Pebrel", diff --git a/nebula_app/src/ssh_session/host_key/windows.rs b/nebula_app/src/ssh_session/host_key/windows.rs index fbdec6247..beaf7af26 100644 --- a/nebula_app/src/ssh_session/host_key/windows.rs +++ b/nebula_app/src/ssh_session/host_key/windows.rs @@ -1,11 +1,12 @@ //! Trust-file replacement preserves the target DACL and never ignores ACL merge errors. use std::io; -use std::path::Path; use std::os::windows::ffi::OsStrExt as _; +use std::path::Path; use windows_sys::Win32::Storage::FileSystem::ReplaceFileW; pub(super) fn replace(source: &Path, destination: &Path) -> io::Result<()> { - let backup = tempfile::Builder::new().prefix(".known-hosts-backup-") + let backup = tempfile::Builder::new() + .prefix(".known-hosts-backup-") .tempfile_in(destination.parent().ok_or_else(|| io::Error::other("No trust directory"))?)? .into_temp_path(); std::fs::remove_file(&backup)?; @@ -17,10 +18,18 @@ pub(super) fn replace(source: &Path, destination: &Path) -> io::Result<()> { // mandatory. A backup is required because error 1176 without one can remove // the destination, and 1177 can move it to the backup before returning failure. let success = unsafe { - ReplaceFileW(original.as_ptr(), replacement.as_ptr(), saved.as_ptr(), 0, - std::ptr::null(), std::ptr::null()) + ReplaceFileW( + original.as_ptr(), + replacement.as_ptr(), + saved.as_ptr(), + 0, + std::ptr::null(), + std::ptr::null(), + ) }; - if success != 0 { return Ok(()); } + if success != 0 { + return Ok(()); + } let error = io::Error::last_os_error(); if backup.exists() { // An exclusive hard-link creation restores the original file identity @@ -30,7 +39,10 @@ pub(super) fn replace(source: &Path, destination: &Path) -> io::Result<()> { return Err(error); } let preserved = backup.keep().map_err(io::Error::other)?; - return Err(io::Error::new(error.kind(), format!("{error}; original trust file retained at {}", preserved.display()))); + return Err(io::Error::new( + error.kind(), + format!("{error}; original trust file retained at {}", preserved.display()), + )); } Err(error) } @@ -48,7 +60,9 @@ mod tests { }; let output = std::process::Command::new("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", script]) - .env("PEBREL_KNOWN_HOSTS", path).output().unwrap(); + .env("PEBREL_KNOWN_HOSTS", path) + .output() + .unwrap(); assert!(output.status.success(), "{}", String::from_utf8_lossy(&output.stderr)); String::from_utf8(output.stdout).unwrap().trim().to_owned() } @@ -59,11 +73,31 @@ mod tests { let path = directory.path().join("known_hosts"); let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); - std::fs::write(&path, format!("fixture.example {}\n", old.public_key().to_openssh().unwrap())).unwrap(); + std::fs::write( + &path, + format!("fixture.example {}\n", old.public_key().to_openssh().unwrap()), + ) + .unwrap(); let before = acl(&path, true); - super::super::inspect(&path, "fixture.example", 22, new.public_key()).unwrap().unwrap().save(&path).unwrap(); - assert_eq!(acl(&path, false), before, "Parent's broader inherited DACL must not replace target DACL"); - assert!(russh::keys::known_hosts::check_known_hosts_path("fixture.example", 22, new.public_key(), &path).unwrap()); + super::super::inspect(&path, "fixture.example", 22, new.public_key()) + .unwrap() + .unwrap() + .save(&path) + .unwrap(); + assert_eq!( + acl(&path, false), + before, + "Parent's broader inherited DACL must not replace target DACL" + ); + assert!( + russh::keys::known_hosts::check_known_hosts_path( + "fixture.example", + 22, + new.public_key(), + &path + ) + .unwrap() + ); } #[test] diff --git a/nebula_app/src/ssh_session/tests.rs b/nebula_app/src/ssh_session/tests.rs index a63406b82..e52fcb658 100644 --- a/nebula_app/src/ssh_session/tests.rs +++ b/nebula_app/src/ssh_session/tests.rs @@ -371,11 +371,17 @@ fn trusted_host_is_not_blocked_by_unrelated_invalid_keys_or_unknown_algorithms() let directory = tempfile::tempdir().unwrap(); let path = directory.path().join("known_hosts"); let key = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); - let source = format!("fixture.example {}\nother.example ssh-ed25519 NOT_BASE64\nunknown.example ssh-future-format opaque\n", key.public_key().to_openssh().unwrap()); + let source = format!( + "fixture.example {}\nother.example ssh-ed25519 NOT_BASE64\nunknown.example ssh-future-format opaque\n", + key.public_key().to_openssh().unwrap() + ); std::fs::write(&path, &source).unwrap(); let mut handler = super::ClientHandler { - host: "fixture.example".into(), port: 22, allow_prompt: true, - handshake: super::lifecycle::Handshake::default(), known_hosts_path: Some(path.clone()), + host: "fixture.example".into(), + port: 22, + allow_prompt: true, + handshake: super::lifecycle::Handshake::default(), + known_hosts_path: Some(path.clone()), }; let runtime = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap(); assert!(runtime.block_on(handler.check_server_key(key.public_key())).unwrap()); From a35b9ba1fd1e62f244da81774d2dce2da4141cc1 Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:10:15 +0800 Subject: [PATCH 05/10] fix(ssh): leave nonmatching malformed patterns uninterpreted --- .../2026-10-03-changed-host-key-confirmation.md | 4 ++-- nebula_app/src/ssh_session/host_key.rs | 14 +++++++++++--- nebula_app/src/ssh_session/tests.rs | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md index 72a2f9706..787c43734 100644 --- a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md +++ b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md @@ -43,8 +43,8 @@ replacement on Unix and DACL-preserving replacement on Windows. Reject symlinks ## Consequences Canceled, unattended and unavailable UI paths cannot update trust. Pattern and -marked records require external known_hosts administration. Invalid input fails -closed. Independent OpenSSH writers do not share Pebrel's lock: the comparison +marked records require external known_hosts administration. Invalid matching keys +fail closed; records without a valid matching pattern remain uninterpreted. Independent OpenSSH writers do not share Pebrel's lock: the comparison is not a cross-tool atomic compare-and-swap. Windows uses `ReplaceFileW` without ACL/merge-error bypass flags, preserving the target DACL. An explicit backup protects failure cases that move the original file; recovery never overwrites a concurrent destination and retains the backup diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs index df0c40b75..6ac0a7c41 100644 --- a/nebula_app/src/ssh_session/host_key.rs +++ b/nebula_app/src/ssh_session/host_key.rs @@ -68,12 +68,20 @@ pub(super) fn inspect( let mut fields = content.split_whitespace(); let first = fields.next().expect("nonempty record"); let hosts = if first.starts_with('@') { - fields.next().ok_or_else(|| denied("Missing marked host patterns"))? + let Some(hosts) = fields.next() else { + replacement.push_str(line); + continue; + }; + hosts } else { first }; - let patterns: HostPatterns = - hosts.parse().map_err(|_| denied("Cannot safely parse host patterns"))?; + let Ok(patterns) = hosts.parse::() else { + // An invalid pattern cannot establish trust for this endpoint. + // Preserve it without interpreting its unrelated key material. + replacement.push_str(line); + continue; + }; if !matches(&patterns, &endpoint) { replacement.push_str(line); continue; diff --git a/nebula_app/src/ssh_session/tests.rs b/nebula_app/src/ssh_session/tests.rs index e52fcb658..8cef41fb6 100644 --- a/nebula_app/src/ssh_session/tests.rs +++ b/nebula_app/src/ssh_session/tests.rs @@ -372,7 +372,7 @@ fn trusted_host_is_not_blocked_by_unrelated_invalid_keys_or_unknown_algorithms() let path = directory.path().join("known_hosts"); let key = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); let source = format!( - "fixture.example {}\nother.example ssh-ed25519 NOT_BASE64\nunknown.example ssh-future-format opaque\n", + "fixture.example {}\nother.example ssh-ed25519 NOT_BASE64\nunknown.example ssh-future-format opaque\n|1|BAD|BAD ssh-ed25519 NOT_BASE64\n@revoked\n", key.public_key().to_openssh().unwrap() ); std::fs::write(&path, &source).unwrap(); From 847dee8648d1898affce249c654b5ba158b3634c Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:12:42 +0800 Subject: [PATCH 06/10] fix(ssh): retain changed-key confirmation across file races --- nebula_app/src/ssh_session.rs | 30 +++++++++++-------- nebula_app/src/ssh_session/lifecycle/tests.rs | 12 ++++++++ 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/nebula_app/src/ssh_session.rs b/nebula_app/src/ssh_session.rs index 0d85b616b..634529e7a 100644 --- a/nebula_app/src/ssh_session.rs +++ b/nebula_app/src/ssh_session.rs @@ -529,7 +529,23 @@ impl client::Handler for ClientHandler { // Inspect marked records even on a first visit: russh's literal checker // otherwise treats @revoked / @cert-authority entries as unknown hosts. let path = self.host_key_path()?; - let inspected = host_key::inspect(&path, &self.host, self.port, server_public_key)?; + if let Some(change) = host_key::inspect(&path, &self.host, self.port, server_public_key)? { + if !self.allow_prompt { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "SSH host identity changed; explicit confirmation required", + ) + .into()); + } + let confirmation = crate::ssh_prompt::confirm_changed_host( + &self.host, + self.port, + change.fingerprints.join("\n"), + server_public_key.fingerprint(ssh_key::HashAlg::Sha256).to_string(), + ); + let accepted = async { confirmation.await.unwrap_or(false) }; + return Ok(self.confirm_changed_key(change, accepted).await?); + } match self.verify_host_key(server_public_key) { Ok(true) => Ok(true), Ok(false) => Ok(self @@ -538,18 +554,6 @@ impl client::Handler for ClientHandler { confirm_new_host(&self.host, self.port, server_public_key), ) .await), - Err(russh::keys::Error::KeyChanged { .. }) if self.allow_prompt => { - let change = - inspected.ok_or_else(|| io::Error::other("No changed SSH host record"))?; - let confirmation = crate::ssh_prompt::confirm_changed_host( - &self.host, - self.port, - change.fingerprints.join("\n"), - server_public_key.fingerprint(ssh_key::HashAlg::Sha256).to_string(), - ); - let accepted = async { confirmation.await.unwrap_or(false) }; - Ok(self.confirm_changed_key(change, accepted).await?) - }, Err(err) => { warn!("SSH 主机密钥验证失败: {err}"); // Return the verification error to the terminal/test status. A blocking diff --git a/nebula_app/src/ssh_session/lifecycle/tests.rs b/nebula_app/src/ssh_session/lifecycle/tests.rs index 1668a6a66..2e733f4e6 100644 --- a/nebula_app/src/ssh_session/lifecycle/tests.rs +++ b/nebula_app/src/ssh_session/lifecycle/tests.rs @@ -800,6 +800,18 @@ fn changed_loopback_host_refuses_before_confirmation_and_connects_only_after_sav }; assert!(!handler.confirm_changed_key(change(), async { false }).await.unwrap()); assert_eq!(std::fs::read_to_string(path).unwrap(), source); + let concurrent = format!("{source}# independent writer\n"); + assert!( + handler + .confirm_changed_key(change(), async { + std::fs::write(path, &concurrent).unwrap(); + true + }) + .await + .is_err() + ); + assert_eq!(std::fs::read_to_string(path).unwrap(), concurrent); + std::fs::write(path, &source).unwrap(); assert!(handler.confirm_changed_key(change(), async { true }).await.unwrap()); let acquired = fixture.connect().await; fixture.forget(&acquired.session).await; From c2567600e7aace4030f6f3d7608e60e9abec349f Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:15:41 +0800 Subject: [PATCH 07/10] refactor(platform): own trust-file replacement by platform capability --- nebula_app/src/platform/mod.rs | 1 + nebula_app/src/platform/trust_file.rs | 17 +++++++++++ .../trust_file}/windows.rs | 29 ++++--------------- nebula_app/src/ssh_session/host_key.rs | 8 +---- 4 files changed, 25 insertions(+), 30 deletions(-) create mode 100644 nebula_app/src/platform/trust_file.rs rename nebula_app/src/{ssh_session/host_key => platform/trust_file}/windows.rs (84%) diff --git a/nebula_app/src/platform/mod.rs b/nebula_app/src/platform/mod.rs index 30361287a..3709848ba 100644 --- a/nebula_app/src/platform/mod.rs +++ b/nebula_app/src/platform/mod.rs @@ -45,6 +45,7 @@ pub(crate) mod ssh_agent; pub mod startup; #[cfg(unix)] pub(crate) mod tray_native; +pub(crate) mod trust_file; pub(crate) mod update_installation; #[cfg(feature = "gpui-shell")] pub(crate) mod window_chrome; diff --git a/nebula_app/src/platform/trust_file.rs b/nebula_app/src/platform/trust_file.rs new file mode 100644 index 000000000..13f19db09 --- /dev/null +++ b/nebula_app/src/platform/trust_file.rs @@ -0,0 +1,17 @@ +//! Replacement of an existing trust file must preserve its access controls. +use std::io; +use std::path::Path; + +#[cfg(windows)] +mod windows; + +pub(crate) fn replace(source: &Path, destination: &Path) -> io::Result<()> { + #[cfg(windows)] + { + windows::replace(source, destination) + } + #[cfg(not(windows))] + { + crate::atomic_file::replace(source, destination) + } +} diff --git a/nebula_app/src/ssh_session/host_key/windows.rs b/nebula_app/src/platform/trust_file/windows.rs similarity index 84% rename from nebula_app/src/ssh_session/host_key/windows.rs rename to nebula_app/src/platform/trust_file/windows.rs index beaf7af26..645a2ce0b 100644 --- a/nebula_app/src/ssh_session/host_key/windows.rs +++ b/nebula_app/src/platform/trust_file/windows.rs @@ -4,7 +4,7 @@ use std::os::windows::ffi::OsStrExt as _; use std::path::Path; use windows_sys::Win32::Storage::FileSystem::ReplaceFileW; -pub(super) fn replace(source: &Path, destination: &Path) -> io::Result<()> { +pub(crate) fn replace(source: &Path, destination: &Path) -> io::Result<()> { let backup = tempfile::Builder::new() .prefix(".known-hosts-backup-") .tempfile_in(destination.parent().ok_or_else(|| io::Error::other("No trust directory"))?)? @@ -50,7 +50,6 @@ pub(super) fn replace(source: &Path, destination: &Path) -> io::Result<()> { #[cfg(test)] mod tests { use super::*; - use russh::keys::ssh_key::{Algorithm, PrivateKey}; fn acl(path: &Path, setup: bool) -> String { let script = if setup { @@ -71,33 +70,17 @@ mod tests { fn changed_trust_file_keeps_its_stricter_windows_dacl() { let directory = tempfile::tempdir().unwrap(); let path = directory.path().join("known_hosts"); - let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); - let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); - std::fs::write( - &path, - format!("fixture.example {}\n", old.public_key().to_openssh().unwrap()), - ) - .unwrap(); + let replacement = directory.path().join("replacement"); + std::fs::write(&path, "original trusted key\n").unwrap(); + std::fs::write(&replacement, "new trusted key\n").unwrap(); let before = acl(&path, true); - super::super::inspect(&path, "fixture.example", 22, new.public_key()) - .unwrap() - .unwrap() - .save(&path) - .unwrap(); + replace(&replacement, &path).unwrap(); assert_eq!( acl(&path, false), before, "Parent's broader inherited DACL must not replace target DACL" ); - assert!( - russh::keys::known_hosts::check_known_hosts_path( - "fixture.example", - 22, - new.public_key(), - &path - ) - .unwrap() - ); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "new trusted key\n"); } #[test] diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs index 6ac0a7c41..e757688f3 100644 --- a/nebula_app/src/ssh_session/host_key.rs +++ b/nebula_app/src/ssh_session/host_key.rs @@ -8,9 +8,6 @@ use russh::keys::ssh_key::{ known_hosts::{Entry, HostPatterns}, }; -#[cfg(windows)] -mod windows; - pub(super) struct Change { snapshot: Vec, replacement: Vec, @@ -173,10 +170,7 @@ impl Change { )); } let temporary = temporary.into_temp_path(); - #[cfg(windows)] - return windows::replace(&temporary, path); - #[cfg(not(windows))] - crate::atomic_file::replace(&temporary, path) + crate::platform::trust_file::replace(&temporary, path) } } From 7ad2f2203ad3114833c48ac8fb7be6aede733a45 Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:22:22 +0800 Subject: [PATCH 08/10] fix(ssh): keep explicitly recorded server keys trusted --- ...026-10-03-changed-host-key-confirmation.md | 2 + nebula_app/src/ssh_session.rs | 36 ++++++------ nebula_app/src/ssh_session/host_key.rs | 57 ++++++++++++++----- nebula_app/src/ssh_session/lifecycle/tests.rs | 10 +++- nebula_app/src/ssh_session/tests.rs | 26 +++++++++ 5 files changed, 97 insertions(+), 34 deletions(-) diff --git a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md index 787c43734..bb7f93baa 100644 --- a/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md +++ b/architecture/notes/nebula_app/ssh_session/2026-10-03-changed-host-key-confirmation.md @@ -22,6 +22,8 @@ Keep the handshake refused until the existing bounded prompt receives explicit trust and the changed record is saved successfully. Show host/port, old SHA256 fingerprints, the new SHA256 fingerprint and the identity-change warning. +A presented key already trusted in the same snapshot remains trusted even when +another ordinary record retains an older key. This is not a reason to update. Use the library parser, preserve unrelated original lines, and split only the literal target from multi-host records. Preserve hashed names, comments, line endings and other algorithms. Marked matching records and changed wildcard or diff --git a/nebula_app/src/ssh_session.rs b/nebula_app/src/ssh_session.rs index 634529e7a..b7b37b2b8 100644 --- a/nebula_app/src/ssh_session.rs +++ b/nebula_app/src/ssh_session.rs @@ -529,22 +529,26 @@ impl client::Handler for ClientHandler { // Inspect marked records even on a first visit: russh's literal checker // otherwise treats @revoked / @cert-authority entries as unknown hosts. let path = self.host_key_path()?; - if let Some(change) = host_key::inspect(&path, &self.host, self.port, server_public_key)? { - if !self.allow_prompt { - return Err(io::Error::new( - io::ErrorKind::PermissionDenied, - "SSH host identity changed; explicit confirmation required", - ) - .into()); - } - let confirmation = crate::ssh_prompt::confirm_changed_host( - &self.host, - self.port, - change.fingerprints.join("\n"), - server_public_key.fingerprint(ssh_key::HashAlg::Sha256).to_string(), - ); - let accepted = async { confirmation.await.unwrap_or(false) }; - return Ok(self.confirm_changed_key(change, accepted).await?); + match host_key::inspect(&path, &self.host, self.port, server_public_key)? { + host_key::Verification::Trusted => return Ok(true), + host_key::Verification::Unknown => {}, + host_key::Verification::Changed(change) => { + if !self.allow_prompt { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "SSH host identity changed; explicit confirmation required", + ) + .into()); + } + let confirmation = crate::ssh_prompt::confirm_changed_host( + &self.host, + self.port, + change.fingerprints.join("\n"), + server_public_key.fingerprint(ssh_key::HashAlg::Sha256).to_string(), + ); + let accepted = async { confirmation.await.unwrap_or(false) }; + return Ok(self.confirm_changed_key(change, accepted).await?); + }, } match self.verify_host_key(server_public_key) { Ok(true) => Ok(true), diff --git a/nebula_app/src/ssh_session/host_key.rs b/nebula_app/src/ssh_session/host_key.rs index e757688f3..bd5f63083 100644 --- a/nebula_app/src/ssh_session/host_key.rs +++ b/nebula_app/src/ssh_session/host_key.rs @@ -8,6 +8,12 @@ use russh::keys::ssh_key::{ known_hosts::{Entry, HostPatterns}, }; +pub(super) enum Verification { + Trusted, + Unknown, + Changed(Change), +} + pub(super) struct Change { snapshot: Vec, replacement: Vec, @@ -44,16 +50,18 @@ pub(super) fn inspect( host: &str, port: u16, key: &ssh_key::PublicKey, -) -> io::Result> { +) -> io::Result { let snapshot = match std::fs::read(path) { Ok(bytes) => bytes, - Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(None), + Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(Verification::Unknown), Err(error) => return Err(error), }; let source = std::str::from_utf8(&snapshot).map_err(|_| denied("known_hosts is not UTF-8"))?; let endpoint = if port == 22 { host.to_owned() } else { format!("[{host}]:{port}") }; let mut replacement = String::new(); let mut fingerprints = Vec::new(); + let mut trusted = false; + let mut unsupported_change = false; for line in source.split_inclusive('\n') { let content = line.trim_end_matches(['\r', '\n']); let ending = &line[content.len()..]; @@ -83,19 +91,18 @@ pub(super) fn inspect( replacement.push_str(line); continue; } - let entry: Entry = normalized - .parse() - .map_err(|_| denied("Cannot safely parse matching known_hosts record"))?; - if entry.marker().is_some() { - return Err(denied( - "SSH host has a revoked or certificate-authority record; update it outside Pebrel", - )); + if first.starts_with('@') { + return Err(denied("SSH host has a marked trust record; update it outside Pebrel")); } - if entry.public_key().algorithm() != key.algorithm() { + if fields.next() != Some(key.algorithm().as_str()) { replacement.push_str(line); continue; } + let entry: Entry = normalized + .parse() + .map_err(|_| denied("Cannot safely parse matching known_hosts record"))?; if entry.public_key().key_data() == key.key_data() { + trusted = true; replacement.push_str(line); continue; } @@ -113,7 +120,9 @@ pub(super) fn inspect( .iter() .any(|pattern| pattern.contains(['*', '?']) || pattern.starts_with('!')) { - return Err(denied("Pattern-based SSH host records cannot be safely replaced")); + unsupported_change = true; + replacement.push_str(line); + continue; } patterns.iter().filter(|pattern| *pattern != &endpoint).cloned().collect::>() }, @@ -137,10 +146,20 @@ pub(super) fn inspect( replacement.push_str(&saved_key.to_openssh().map_err(io::Error::other)?); replacement.push_str(ending); } + if trusted { + return Ok(Verification::Trusted); + } + if unsupported_change { + return Err(denied("Pattern-based SSH host records cannot be safely replaced")); + } if fingerprints.is_empty() { - return Ok(None); + return Ok(Verification::Unknown); } - Ok(Some(Change { snapshot, replacement: replacement.into_bytes(), fingerprints })) + Ok(Verification::Changed(Change { + snapshot, + replacement: replacement.into_bytes(), + fingerprints, + })) } impl Change { @@ -208,7 +227,11 @@ mod tests { ); let source = format!("{unrelated}{endpoint},other.example {old_line}\r\n{hash} {old_line}"); std::fs::write(&path, &source).unwrap(); - let change = inspect(&path, "fixture.example", 2200, new.public_key()).unwrap().unwrap(); + let Verification::Changed(change) = + inspect(&path, "fixture.example", 2200, new.public_key()).unwrap() + else { + panic!("A different recorded key needs confirmation"); + }; assert_eq!( change.fingerprints, [old.public_key().fingerprint(ssh_key::HashAlg::Sha256).to_string()] @@ -257,7 +280,11 @@ mod tests { let new = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); let source = format!("fixture.example {}\n", old.public_key().to_openssh().unwrap()); std::fs::write(&path, &source).unwrap(); - let change = inspect(&path, "fixture.example", 22, new.public_key()).unwrap().unwrap(); + let Verification::Changed(change) = + inspect(&path, "fixture.example", 22, new.public_key()).unwrap() + else { + panic!("A different recorded key needs confirmation"); + }; let concurrent = format!("{source}# another writer\n"); std::fs::write(&path, &concurrent).unwrap(); assert!(change.save(&path).is_err()); diff --git a/nebula_app/src/ssh_session/lifecycle/tests.rs b/nebula_app/src/ssh_session/lifecycle/tests.rs index 2e733f4e6..eee00ffad 100644 --- a/nebula_app/src/ssh_session/lifecycle/tests.rs +++ b/nebula_app/src/ssh_session/lifecycle/tests.rs @@ -794,9 +794,13 @@ fn changed_loopback_host_refuses_before_confirmation_and_connects_only_after_sav known_hosts_path: Some(path.clone()), }; let change = || { - super::super::host_key::inspect(path, "127.0.0.1", destination.port, entry.public_key()) - .unwrap() - .unwrap() + let super::super::host_key::Verification::Changed(change) = + super::super::host_key::inspect(path, "127.0.0.1", destination.port, entry.public_key()) + .unwrap() + else { + panic!("Loopback fixture has a different saved identity"); + }; + change }; assert!(!handler.confirm_changed_key(change(), async { false }).await.unwrap()); assert_eq!(std::fs::read_to_string(path).unwrap(), source); diff --git a/nebula_app/src/ssh_session/tests.rs b/nebula_app/src/ssh_session/tests.rs index 8cef41fb6..c73cc476d 100644 --- a/nebula_app/src/ssh_session/tests.rs +++ b/nebula_app/src/ssh_session/tests.rs @@ -387,3 +387,29 @@ fn trusted_host_is_not_blocked_by_unrelated_invalid_keys_or_unknown_algorithms() assert!(runtime.block_on(handler.check_server_key(key.public_key())).unwrap()); assert_eq!(std::fs::read_to_string(&path).unwrap(), source); } + +#[test] +fn a_recorded_server_key_is_trusted_without_prompt_despite_another_old_key() { + use russh::client::Handler as _; + use russh::keys::ssh_key::{Algorithm, PrivateKey}; + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("known_hosts"); + let current = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let old = PrivateKey::random(&mut rand::rng(), Algorithm::Ed25519).unwrap(); + let source = format!( + "fixture.example {}\nfixture.example {}\nfixture.example ssh-future-format opaque\n", + current.public_key().to_openssh().unwrap(), + old.public_key().to_openssh().unwrap() + ); + std::fs::write(&path, &source).unwrap(); + let mut handler = super::ClientHandler { + host: "fixture.example".into(), + port: 22, + allow_prompt: false, + handshake: super::lifecycle::Handshake::default(), + known_hosts_path: Some(path.clone()), + }; + let runtime = tokio::runtime::Builder::new_current_thread().enable_all().build().unwrap(); + assert!(runtime.block_on(handler.check_server_key(current.public_key())).unwrap()); + assert_eq!(std::fs::read_to_string(path).unwrap(), source); +} From 75b2fee0c57f333e5826ae270ac492d2c088b316 Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 17:25:19 +0800 Subject: [PATCH 09/10] style(ssh): format loopback snapshot classification fixture --- nebula_app/src/ssh_session/lifecycle/tests.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/nebula_app/src/ssh_session/lifecycle/tests.rs b/nebula_app/src/ssh_session/lifecycle/tests.rs index eee00ffad..dd564fa17 100644 --- a/nebula_app/src/ssh_session/lifecycle/tests.rs +++ b/nebula_app/src/ssh_session/lifecycle/tests.rs @@ -795,8 +795,13 @@ fn changed_loopback_host_refuses_before_confirmation_and_connects_only_after_sav }; let change = || { let super::super::host_key::Verification::Changed(change) = - super::super::host_key::inspect(path, "127.0.0.1", destination.port, entry.public_key()) - .unwrap() + super::super::host_key::inspect( + path, + "127.0.0.1", + destination.port, + entry.public_key(), + ) + .unwrap() else { panic!("Loopback fixture has a different saved identity"); }; From 70539af9da7d8790c51cb7fb628e55a8cd9070db Mon Sep 17 00:00:00 2001 From: GeekMr <140129782+WilliamWang1721@users.noreply.github.com> Date: Sat, 3 Oct 2026 18:09:26 +0800 Subject: [PATCH 10/10] test(windows): isolate ACL fixture module paths --- nebula_app/src/platform/trust_file/windows.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nebula_app/src/platform/trust_file/windows.rs b/nebula_app/src/platform/trust_file/windows.rs index 645a2ce0b..b8701eb15 100644 --- a/nebula_app/src/platform/trust_file/windows.rs +++ b/nebula_app/src/platform/trust_file/windows.rs @@ -57,8 +57,11 @@ mod tests { } else { r#"$ErrorActionPreference='Stop'; (Get-Acl $env:PEBREL_KNOWN_HOSTS).GetSecurityDescriptorSddlForm('Access')"# }; + // Indirect launches from pwsh inherit incompatible PowerShell 7 modules. + // Let Windows PowerShell rebuild its own module paths. let output = std::process::Command::new("powershell.exe") .args(["-NoProfile", "-NonInteractive", "-Command", script]) + .env_remove("PSModulePath") .env("PEBREL_KNOWN_HOSTS", path) .output() .unwrap();