From a4bb4ce954ed210f80c5dbc643fc25a46da38442 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 14 Nov 2024 14:32:21 +0100 Subject: [PATCH 1/8] Force unlock when keys are cleared / on first unlock and fix account switching behavior --- apps/desktop/desktop_native/Cargo.lock | 2 +- apps/desktop/desktop_native/core/Cargo.toml | 2 +- .../desktop_native/core/src/ssh_agent/mod.rs | 38 ++++++++++++++++++ .../desktop_native/core/src/ssh_agent/unix.rs | 1 + .../core/src/ssh_agent/windows.rs | 1 + apps/desktop/desktop_native/napi/index.d.ts | 1 + apps/desktop/desktop_native/napi/src/lib.rs | 6 +++ .../platform/main/main-ssh-agent.service.ts | 6 +++ apps/desktop/src/platform/preload.ts | 3 ++ .../platform/services/ssh-agent.service.ts | 40 +++++++++++++++++-- 10 files changed, 94 insertions(+), 6 deletions(-) diff --git a/apps/desktop/desktop_native/Cargo.lock b/apps/desktop/desktop_native/Cargo.lock index 53d151397f6..85b80e920e6 100644 --- a/apps/desktop/desktop_native/Cargo.lock +++ b/apps/desktop/desktop_native/Cargo.lock @@ -313,7 +313,7 @@ checksum = "b048fb63fd8b5923fc5aa7b340d8e156aec7ec02f0c78fa8a6ddc2613f6f71de" [[package]] name = "bitwarden-russh" version = "0.1.0" -source = "git+https://github.com/bitwarden/bitwarden-russh.git?branch=km/pm-10098/clean-russh-implementation#86ff1bf2f4620a3ae5684adee31abdbee33c6f07" +source = "git+https://github.com/bitwarden/bitwarden-russh.git?branch=km/PM-14863/allow-unlock-on-list#275771a3a764dcf3e7c9ec75c396499af5925190" dependencies = [ "anyhow", "byteorder", diff --git a/apps/desktop/desktop_native/core/Cargo.toml b/apps/desktop/desktop_native/core/Cargo.toml index 4f6fdb47fdf..b285621c50b 100644 --- a/apps/desktop/desktop_native/core/Cargo.toml +++ b/apps/desktop/desktop_native/core/Cargo.toml @@ -50,7 +50,7 @@ ssh-key = { version = "0.6.6", default-features = false, features = [ "rsa", "getrandom", ] } -bitwarden-russh = { git = "https://github.com/bitwarden/bitwarden-russh.git", branch = "km/pm-10098/clean-russh-implementation" } +bitwarden-russh = { git = "https://github.com/bitwarden/bitwarden-russh.git", branch = "km/PM-14863/allow-unlock-on-list" } tokio = { version = "=1.40.0", features = ["io-util", "sync", "macros", "net"] } tokio-stream = { version = "=0.1.15", features = ["net"] } tokio-util = "=0.7.12" diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs index ad0ac837afc..7f29ea3eaae 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs @@ -20,6 +20,8 @@ pub struct BitwardenDesktopAgent { show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, get_ui_response_rx: Arc>>, request_id: Arc>, + // before first unlock, or after account switching, listing keys should require an unlock to get a list of public keys + needs_unlock: Arc>, } impl BitwardenDesktopAgent { @@ -46,6 +48,28 @@ impl ssh_agent::Agent for BitwardenDesktopAgent { } false } + + async fn can_list(&self) -> bool { + let needs_unlock = self.needs_unlock.lock().await; + if !*needs_unlock { + return true; + } + drop(needs_unlock); + + let request_id = self.get_request_id().await; + + let mut rx_channel = self.get_ui_response_rx.lock().await.resubscribe(); + self.show_ui_request_tx + .send((request_id, "".to_string())) + .await + .expect("Should send request to ui"); + while let Ok((id, response)) = rx_channel.recv().await { + if id == request_id { + return response; + } + } + false + } } impl BitwardenDesktopAgent { @@ -65,6 +89,10 @@ impl BitwardenDesktopAgent { let keystore = &mut self.keystore; keystore.0.write().expect("RwLock is not poisoned").clear(); + let mut needs_unlock = self.needs_unlock.blocking_lock(); + *needs_unlock = false; + drop(needs_unlock); + for (key, name, cipher_id) in new_keys.iter() { match parse_key_safe(&key) { Ok(private_key) => { @@ -102,6 +130,16 @@ impl BitwardenDesktopAgent { }); Ok(()) } + + pub fn clear_keys(&mut self) -> Result<(), anyhow::Error> { + let keystore = &mut self.keystore; + keystore.0.write().expect("RwLock is not poisoned").clear(); + let mut needs_unlock = self.needs_unlock.blocking_lock(); + *needs_unlock = true; + drop(needs_unlock); + + Ok(()) + } } fn parse_key_safe(pem: &str) -> Result { diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs b/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs index c1a39506660..61bda7ee25c 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs @@ -23,6 +23,7 @@ impl BitwardenDesktopAgent { show_ui_request_tx: auth_request_tx, get_ui_response_rx: auth_response_rx, request_id: Arc::new(tokio::sync::Mutex::new(0)), + needs_unlock: Arc::new(tokio::sync::Mutex::new(true)), }; let cloned_agent_state = agent.clone(); tokio::spawn(async move { diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs b/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs index fd6d9dacb9f..611b0025e96 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs @@ -21,6 +21,7 @@ impl BitwardenDesktopAgent { get_ui_response_rx: auth_response_rx, cancellation_token: CancellationToken::new(), request_id: Arc::new(tokio::sync::Mutex::new(0)), + needs_unlock: Arc::new(tokio::sync::Mutex::new(true)), }; let stream = named_pipe_listener_stream::NamedPipeServerStream::new( agent_state.cancellation_token.clone(), diff --git a/apps/desktop/desktop_native/napi/index.d.ts b/apps/desktop/desktop_native/napi/index.d.ts index 6d1a7b8abbc..d4543c0d957 100644 --- a/apps/desktop/desktop_native/napi/index.d.ts +++ b/apps/desktop/desktop_native/napi/index.d.ts @@ -74,6 +74,7 @@ export declare namespace sshagent { export function setKeys(agentState: SshAgentState, newKeys: Array): void export function lock(agentState: SshAgentState): void export function importKey(encodedKey: string, password: string): SshKeyImportResult + export function clearKeys(agentState: SshAgentState): void export function generateKeypair(keyAlgorithm: string): Promise export class SshAgentState { } } diff --git a/apps/desktop/desktop_native/napi/src/lib.rs b/apps/desktop/desktop_native/napi/src/lib.rs index 60a8326a8e5..5dd570e2372 100644 --- a/apps/desktop/desktop_native/napi/src/lib.rs +++ b/apps/desktop/desktop_native/napi/src/lib.rs @@ -339,6 +339,12 @@ pub mod sshagent { Ok(result.into()) } + #[napi] + pub fn clear_keys(agent_state: &mut SshAgentState) -> napi::Result<()> { + let bitwarden_agent_state = &mut agent_state.state; + bitwarden_agent_state.clear_keys().map_err(|e| napi::Error::from_reason(e.to_string())) + } + #[napi] pub async fn generate_keypair(key_algorithm: String) -> napi::Result { desktop_core::ssh_agent::generator::generate_keypair(key_algorithm) diff --git a/apps/desktop/src/platform/main/main-ssh-agent.service.ts b/apps/desktop/src/platform/main/main-ssh-agent.service.ts index c8227e5b6a5..3da9d291007 100644 --- a/apps/desktop/src/platform/main/main-ssh-agent.service.ts +++ b/apps/desktop/src/platform/main/main-ssh-agent.service.ts @@ -111,5 +111,11 @@ export class MainSshAgentService { sshagent.lock(this.agentState); } }); + + ipcMain.handle("sshagent.clearkeys", async (event: any) => { + if (this.agentState != null) { + sshagent.clearKeys(this.agentState); + } + }); } } diff --git a/apps/desktop/src/platform/preload.ts b/apps/desktop/src/platform/preload.ts index 35caeff27c8..763e46bd751 100644 --- a/apps/desktop/src/platform/preload.ts +++ b/apps/desktop/src/platform/preload.ts @@ -56,6 +56,9 @@ const sshAgent = { lock: async () => { return await ipcRenderer.invoke("sshagent.lock"); }, + clearKeys: async () => { + return await ipcRenderer.invoke("sshagent.clearkeys"); + }, importKey: async (key: string, password: string): Promise => { const res = await ipcRenderer.invoke("sshagent.importkey", { privateKey: key, diff --git a/apps/desktop/src/platform/services/ssh-agent.service.ts b/apps/desktop/src/platform/services/ssh-agent.service.ts index dd518a943b8..845544c64ed 100644 --- a/apps/desktop/src/platform/services/ssh-agent.service.ts +++ b/apps/desktop/src/platform/services/ssh-agent.service.ts @@ -8,6 +8,7 @@ import { from, map, of, + skip, Subject, switchMap, takeUntil, @@ -17,6 +18,7 @@ import { withLatestFrom, } from "rxjs"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; @@ -52,6 +54,7 @@ export class SshAgentService implements OnDestroy { private i18nService: I18nService, private desktopSettingsService: DesktopSettingsService, private configService: ConfigService, + private accountService: AccountService, ) {} async init() { @@ -110,11 +113,31 @@ export class SshAgentService implements OnDestroy { ), ), // This concatMap handles showing the dialog to approve the request. - concatMap(([message, decryptedCiphers]) => { + concatMap(([message, ciphers]) => { const cipherId = message.cipherId as string; const requestId = message.requestId as number; - if (decryptedCiphers === undefined) { + // cipherid is empty has the special meaning of "unlock in order to list public keys" + if (cipherId == "") { + return of(true).pipe( + switchMap(async (result) => { + const sshCiphers = ciphers.filter( + (cipher) => cipher.type === CipherType.SshKey && !cipher.isDeleted, + ); + const keys = sshCiphers.map((cipher) => { + return { + name: cipher.name, + privateKey: cipher.sshKey.privateKey, + cipherId: cipher.id, + }; + }); + await ipc.platform.sshAgent.setKeys(keys); + await ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result)); + }), + ); + } + + if (ciphers === undefined) { return of(false).pipe( switchMap((result) => ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result)), @@ -122,7 +145,7 @@ export class SshAgentService implements OnDestroy { ); } - const cipher = decryptedCiphers.find((cipher) => cipher.id == cipherId); + const cipher = ciphers.find((cipher) => cipher.id == cipherId); ipc.platform.focusWindow(); const dialogRef = ApproveSshRequestComponent.open( @@ -141,6 +164,15 @@ export class SshAgentService implements OnDestroy { ) .subscribe(); + this.accountService.activeAccount$ + .pipe(skip(1), takeUntil(this.destroy$)) + .subscribe((account) => { + this.logService.info("Active account changed, clearing SSH keys"); + ipc.platform.sshAgent + .clearKeys() + .catch((e) => this.logService.error("Failed to clear SSH keys", e)); + }); + combineLatest([ timer(0, this.SSH_REFRESH_INTERVAL), this.desktopSettingsService.sshAgentEnabled$, @@ -148,7 +180,7 @@ export class SshAgentService implements OnDestroy { .pipe( concatMap(async ([, enabled]) => { if (!enabled) { - await ipc.platform.sshAgent.setKeys([]); + await ipc.platform.sshAgent.clearKeys(); return; } From bedb4ffc0ade5f132cbd59ec9cf44748065d6896 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 21 Nov 2024 14:24:10 +0100 Subject: [PATCH 2/8] Make comment a doc comment --- apps/desktop/desktop_native/core/src/ssh_agent/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs index 7f29ea3eaae..8899178e076 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs @@ -20,7 +20,7 @@ pub struct BitwardenDesktopAgent { show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, get_ui_response_rx: Arc>>, request_id: Arc>, - // before first unlock, or after account switching, listing keys should require an unlock to get a list of public keys + /// before first unlock, or after account switching, listing keys should require an unlock to get a list of public keys needs_unlock: Arc>, } From 81cb91ff1da70cb44d8361968407fbc5dff17992 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 21 Nov 2024 14:28:17 +0100 Subject: [PATCH 3/8] Pin russh commit --- apps/desktop/desktop_native/Cargo.lock | 2 +- apps/desktop/desktop_native/core/Cargo.toml | 40 ++++++++++----------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/apps/desktop/desktop_native/Cargo.lock b/apps/desktop/desktop_native/Cargo.lock index 85b80e920e6..680395f77f9 100644 --- a/apps/desktop/desktop_native/Cargo.lock +++ b/apps/desktop/desktop_native/Cargo.lock @@ -313,7 +313,7 @@ checksum = "b048fb63fd8b5923fc5aa7b340d8e156aec7ec02f0c78fa8a6ddc2613f6f71de" [[package]] name = "bitwarden-russh" version = "0.1.0" -source = "git+https://github.com/bitwarden/bitwarden-russh.git?branch=km/PM-14863/allow-unlock-on-list#275771a3a764dcf3e7c9ec75c396499af5925190" +source = "git+https://github.com/bitwarden/bitwarden-russh.git?rev=b4e7f2f#b4e7f2fedbe3df8c35545feb000176d3e7b2bc32" dependencies = [ "anyhow", "byteorder", diff --git a/apps/desktop/desktop_native/core/Cargo.toml b/apps/desktop/desktop_native/core/Cargo.toml index b285621c50b..cfcf402b73f 100644 --- a/apps/desktop/desktop_native/core/Cargo.toml +++ b/apps/desktop/desktop_native/core/Cargo.toml @@ -10,22 +10,22 @@ default = ["sys"] manual_test = [] sys = [ - "dep:widestring", - "dep:windows", - "dep:core-foundation", - "dep:security-framework", - "dep:security-framework-sys", - "dep:gio", - "dep:libsecret", - "dep:zbus", - "dep:zbus_polkit", + "dep:widestring", + "dep:windows", + "dep:core-foundation", + "dep:security-framework", + "dep:security-framework-sys", + "dep:gio", + "dep:libsecret", + "dep:zbus", + "dep:zbus_polkit", ] [dependencies] aes = "=0.8.4" anyhow = "=1.0.93" arboard = { version = "=3.4.1", default-features = false, features = [ - "wayland-data-control", + "wayland-data-control", ] } async-stream = "0.3.5" base64 = "=0.22.1" @@ -50,7 +50,7 @@ ssh-key = { version = "0.6.6", default-features = false, features = [ "rsa", "getrandom", ] } -bitwarden-russh = { git = "https://github.com/bitwarden/bitwarden-russh.git", branch = "km/PM-14863/allow-unlock-on-list" } +bitwarden-russh = { git = "https://github.com/bitwarden/bitwarden-russh.git", rev = "b4e7f2f" } tokio = { version = "=1.40.0", features = ["io-util", "sync", "macros", "net"] } tokio-stream = { version = "=0.1.15", features = ["net"] } tokio-util = "=0.7.12" @@ -64,15 +64,15 @@ ed25519 = { version = "=2.2.3", features = ["pkcs8"] } [target.'cfg(windows)'.dependencies] widestring = { version = "=1.1.0", optional = true } windows = { version = "=0.57.0", features = [ - "Foundation", - "Security_Credentials_UI", - "Security_Cryptography", - "Storage_Streams", - "Win32_Foundation", - "Win32_Security_Credentials", - "Win32_System_WinRT", - "Win32_UI_Input_KeyboardAndMouse", - "Win32_UI_WindowsAndMessaging", + "Foundation", + "Security_Credentials_UI", + "Security_Cryptography", + "Storage_Streams", + "Win32_Foundation", + "Win32_Security_Credentials", + "Win32_System_WinRT", + "Win32_UI_Input_KeyboardAndMouse", + "Win32_UI_WindowsAndMessaging", ], optional = true } [target.'cfg(windows)'.dev-dependencies] From 3b178c58b626d9729af2b7fd3fc350b157ff72d4 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 21 Nov 2024 14:30:52 +0100 Subject: [PATCH 4/8] Cleanup --- .../desktop/desktop_native/core/src/ssh_agent/mod.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs index 8899178e076..13e78b4d134 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs @@ -50,11 +50,9 @@ impl ssh_agent::Agent for BitwardenDesktopAgent { } async fn can_list(&self) -> bool { - let needs_unlock = self.needs_unlock.lock().await; - if !*needs_unlock { + if !*self.needs_unlock.lock().await{ return true; } - drop(needs_unlock); let request_id = self.get_request_id().await; @@ -89,9 +87,7 @@ impl BitwardenDesktopAgent { let keystore = &mut self.keystore; keystore.0.write().expect("RwLock is not poisoned").clear(); - let mut needs_unlock = self.needs_unlock.blocking_lock(); - *needs_unlock = false; - drop(needs_unlock); + *self.needs_unlock.blocking_lock() = false; for (key, name, cipher_id) in new_keys.iter() { match parse_key_safe(&key) { @@ -134,9 +130,7 @@ impl BitwardenDesktopAgent { pub fn clear_keys(&mut self) -> Result<(), anyhow::Error> { let keystore = &mut self.keystore; keystore.0.write().expect("RwLock is not poisoned").clear(); - let mut needs_unlock = self.needs_unlock.blocking_lock(); - *needs_unlock = true; - drop(needs_unlock); + *self.needs_unlock.blocking_lock() = true; Ok(()) } From 643584543887ed8a25d167f56195053178880b70 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 21 Nov 2024 15:16:22 +0100 Subject: [PATCH 5/8] Make list messaging explicit --- .../desktop_native/core/src/ssh_agent/mod.rs | 8 ++-- .../desktop_native/core/src/ssh_agent/unix.rs | 2 +- .../core/src/ssh_agent/windows.rs | 2 +- apps/desktop/desktop_native/napi/index.d.ts | 2 +- apps/desktop/desktop_native/napi/src/lib.rs | 8 ++-- .../platform/main/main-ssh-agent.service.ts | 3 +- .../platform/services/ssh-agent.service.ts | 47 +++++++++---------- 7 files changed, 36 insertions(+), 36 deletions(-) diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs index 37118515660..9d04ea87ccb 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/mod.rs @@ -17,7 +17,7 @@ pub mod importer; pub struct BitwardenDesktopAgent { keystore: ssh_agent::KeyStore, cancellation_token: CancellationToken, - show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, + show_ui_request_tx: tokio::sync::mpsc::Sender<(u32, (String, bool))>, get_ui_response_rx: Arc>>, request_id: Arc>, /// before first unlock, or after account switching, listing keys should require an unlock to get a list of public keys @@ -35,8 +35,9 @@ impl ssh_agent::Agent for BitwardenDesktopAgent { let request_id = self.get_request_id().await; let mut rx_channel = self.get_ui_response_rx.lock().await.resubscribe(); + let message = (request_id, (ssh_key.cipher_uuid.clone(), false)); self.show_ui_request_tx - .send((request_id, ssh_key.cipher_uuid.clone())) + .send(message) .await .expect("Should send request to ui"); while let Ok((id, response)) = rx_channel.recv().await { @@ -55,8 +56,9 @@ impl ssh_agent::Agent for BitwardenDesktopAgent { let request_id = self.get_request_id().await; let mut rx_channel = self.get_ui_response_rx.lock().await.resubscribe(); + let message = (request_id, ("".to_string(), true)); self.show_ui_request_tx - .send((request_id, "".to_string())) + .send(message) .await .expect("Should send request to ui"); while let Ok((id, response)) = rx_channel.recv().await { diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs b/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs index 153fd9ff868..ed2fe9ffab1 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/unix.rs @@ -14,7 +14,7 @@ use super::BitwardenDesktopAgent; impl BitwardenDesktopAgent { pub async fn start_server( - auth_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, + auth_request_tx: tokio::sync::mpsc::Sender<(u32, (String, bool))>, auth_response_rx: Arc>>, ) -> Result { let agent = BitwardenDesktopAgent { diff --git a/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs b/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs index ed7026f9931..6a99b7cfb00 100644 --- a/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs +++ b/apps/desktop/desktop_native/core/src/ssh_agent/windows.rs @@ -12,7 +12,7 @@ use super::BitwardenDesktopAgent; impl BitwardenDesktopAgent { pub async fn start_server( - auth_request_tx: tokio::sync::mpsc::Sender<(u32, String)>, + auth_request_tx: tokio::sync::mpsc::Sender<(u32, (String, bool))>, auth_response_rx: Arc>>, ) -> Result { let agent_state = BitwardenDesktopAgent { diff --git a/apps/desktop/desktop_native/napi/index.d.ts b/apps/desktop/desktop_native/napi/index.d.ts index ed531948044..956b2d726da 100644 --- a/apps/desktop/desktop_native/napi/index.d.ts +++ b/apps/desktop/desktop_native/napi/index.d.ts @@ -69,7 +69,7 @@ export declare namespace sshagent { status: SshKeyImportStatus sshKey?: SshKey } - export function serve(callback: (err: Error | null, arg: string) => any): Promise + export function serve(callback: (err: Error | null, arg0: string, arg1: boolean) => any): Promise export function stop(agentState: SshAgentState): void export function isRunning(agentState: SshAgentState): boolean export function setKeys(agentState: SshAgentState, newKeys: Array): void diff --git a/apps/desktop/desktop_native/napi/src/lib.rs b/apps/desktop/desktop_native/napi/src/lib.rs index 610140b586d..4c7bc8eaa93 100644 --- a/apps/desktop/desktop_native/napi/src/lib.rs +++ b/apps/desktop/desktop_native/napi/src/lib.rs @@ -247,15 +247,15 @@ pub mod sshagent { #[napi] pub async fn serve( - callback: ThreadsafeFunction, + callback: ThreadsafeFunction<(String, bool), CalleeHandled>, ) -> napi::Result { - let (auth_request_tx, mut auth_request_rx) = tokio::sync::mpsc::channel::<(u32, String)>(32); + let (auth_request_tx, mut auth_request_rx) = tokio::sync::mpsc::channel::<(u32, (String, bool))>(32); let (auth_response_tx, auth_response_rx) = tokio::sync::broadcast::channel::<(u32, bool)>(32); let auth_response_tx_arc = Arc::new(Mutex::new(auth_response_tx)); tokio::spawn(async move { let _ = auth_response_rx; - while let Some((request_id, cipher_uuid)) = auth_request_rx.recv().await { + while let Some((request_id, (cipher_uuid, is_list_request))) = auth_request_rx.recv().await { let cloned_request_id = request_id.clone(); let cloned_cipher_uuid = cipher_uuid.clone(); let cloned_response_tx_arc = auth_response_tx_arc.clone(); @@ -266,7 +266,7 @@ pub mod sshagent { let auth_response_tx_arc = cloned_response_tx_arc; let callback = cloned_callback; let promise_result: Result, napi::Error> = - callback.call_async(Ok(cipher_uuid)).await; + callback.call_async(Ok((cipher_uuid, is_list_request))).await; match promise_result { Ok(promise_result) => match promise_result.await { Ok(result) => { diff --git a/apps/desktop/src/platform/main/main-ssh-agent.service.ts b/apps/desktop/src/platform/main/main-ssh-agent.service.ts index 65124f1313c..5284029c698 100644 --- a/apps/desktop/src/platform/main/main-ssh-agent.service.ts +++ b/apps/desktop/src/platform/main/main-ssh-agent.service.ts @@ -27,7 +27,7 @@ export class MainSshAgentService { init() { // handle sign request passing to UI sshagent - .serve(async (err: Error, cipherId: string) => { + .serve(async (err: Error, cipherId: string, isListRequest: boolean) => { // clear all old (> SIGN_TIMEOUT) requests this.requestResponses = this.requestResponses.filter( (response) => response.timestamp > new Date(Date.now() - this.SIGN_TIMEOUT), @@ -37,6 +37,7 @@ export class MainSshAgentService { const id_for_this_request = this.request_id; this.messagingService.send("sshagent.signrequest", { cipherId, + isListRequest, requestId: id_for_this_request, }); diff --git a/apps/desktop/src/platform/services/ssh-agent.service.ts b/apps/desktop/src/platform/services/ssh-agent.service.ts index dfcec846d34..b1d30b931b0 100644 --- a/apps/desktop/src/platform/services/ssh-agent.service.ts +++ b/apps/desktop/src/platform/services/ssh-agent.service.ts @@ -115,36 +115,33 @@ export class SshAgentService implements OnDestroy { ), ), // This concatMap handles showing the dialog to approve the request. - concatMap(([message, ciphers]) => { + switchMap(([message, ciphers]) => { const cipherId = message.cipherId as string; + const isListRequest = message.isListRequest as boolean; const requestId = message.requestId as number; - // cipherid is empty has the special meaning of "unlock in order to list public keys" - if (cipherId == "") { - return of(true).pipe( - switchMap(async (result) => { - const sshCiphers = ciphers.filter( - (cipher) => cipher.type === CipherType.SshKey && !cipher.isDeleted, - ); - const keys = sshCiphers.map((cipher) => { - return { - name: cipher.name, - privateKey: cipher.sshKey.privateKey, - cipherId: cipher.id, - }; - }); - await ipc.platform.sshAgent.setKeys(keys); - await ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result)); - }), - ); + if (isListRequest) { + (async () => { + const sshCiphers = ciphers.filter( + (cipher) => cipher.type === CipherType.SshKey && !cipher.isDeleted, + ); + const keys = sshCiphers.map((cipher) => { + return { + name: cipher.name, + privateKey: cipher.sshKey.privateKey, + cipherId: cipher.id, + }; + }); + await ipc.platform.sshAgent.setKeys(keys); + await ipc.platform.sshAgent.signRequestResponse(requestId, true); + })().catch((e) => this.logService.error("Failed to respond to SSH request", e)); + return; } if (ciphers === undefined) { - return of(false).pipe( - switchMap((result) => - ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result)), - ), - ); + ipc.platform.sshAgent + .signRequestResponse(requestId, false) + .catch((e) => this.logService.error("Failed to respond to SSH request", e)); } const cipher = ciphers.find((cipher) => cipher.id == cipherId); @@ -158,7 +155,7 @@ export class SshAgentService implements OnDestroy { return dialogRef.closed.pipe( switchMap((result) => { - return ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result)); + return ipc.platform.sshAgent.signRequestResponse(requestId, result); }), ); }), From 41e25be5b94a651ea30aa050371fa90cb62ae740 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 21 Nov 2024 15:27:30 +0100 Subject: [PATCH 6/8] Add account switching error handling for ssh agent --- apps/desktop/src/platform/services/ssh-agent.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/desktop/src/platform/services/ssh-agent.service.ts b/apps/desktop/src/platform/services/ssh-agent.service.ts index b1d30b931b0..a8a093c8231 100644 --- a/apps/desktop/src/platform/services/ssh-agent.service.ts +++ b/apps/desktop/src/platform/services/ssh-agent.service.ts @@ -163,14 +163,14 @@ export class SshAgentService implements OnDestroy { ) .subscribe(); - this.accountService.activeAccount$ - .pipe(skip(1), takeUntil(this.destroy$)) - .subscribe((account) => { + this.accountService.activeAccount$.pipe(skip(1), takeUntil(this.destroy$)).subscribe({ + next: (account) => { this.logService.info("Active account changed, clearing SSH keys"); ipc.platform.sshAgent .clearKeys() .catch((e) => this.logService.error("Failed to clear SSH keys", e)); - }); + }, + }); combineLatest([ timer(0, this.SSH_REFRESH_INTERVAL), From b9c5721b18a73547d49a438b78115abe4be60444 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 21 Nov 2024 18:44:12 +0100 Subject: [PATCH 7/8] Add account switching error handling for ssh agent --- .../src/platform/services/ssh-agent.service.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/apps/desktop/src/platform/services/ssh-agent.service.ts b/apps/desktop/src/platform/services/ssh-agent.service.ts index a8a093c8231..853b0418885 100644 --- a/apps/desktop/src/platform/services/ssh-agent.service.ts +++ b/apps/desktop/src/platform/services/ssh-agent.service.ts @@ -170,6 +170,18 @@ export class SshAgentService implements OnDestroy { .clearKeys() .catch((e) => this.logService.error("Failed to clear SSH keys", e)); }, + error: (e: unknown) => { + this.logService.error("Error in active account observable", e); + ipc.platform.sshAgent + .clearKeys() + .catch((e) => this.logService.error("Failed to clear SSH keys", e)); + }, + complete: () => { + this.logService.info("Active account observable completed, clearing SSH keys"); + ipc.platform.sshAgent + .clearKeys() + .catch((e) => this.logService.error("Failed to clear SSH keys", e)); + }, }); combineLatest([ From a7c9dfaa6cc225a7591becd243c7349773fa3e97 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 28 Nov 2024 22:23:36 +0100 Subject: [PATCH 8/8] Cleanup --- .../platform/services/ssh-agent.service.ts | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/apps/desktop/src/platform/services/ssh-agent.service.ts b/apps/desktop/src/platform/services/ssh-agent.service.ts index 853b0418885..969300fcc5f 100644 --- a/apps/desktop/src/platform/services/ssh-agent.service.ts +++ b/apps/desktop/src/platform/services/ssh-agent.service.ts @@ -5,6 +5,7 @@ import { concatMap, EMPTY, filter, + firstValueFrom, from, map, of, @@ -115,26 +116,24 @@ export class SshAgentService implements OnDestroy { ), ), // This concatMap handles showing the dialog to approve the request. - switchMap(([message, ciphers]) => { + concatMap(async ([message, ciphers]) => { const cipherId = message.cipherId as string; const isListRequest = message.isListRequest as boolean; const requestId = message.requestId as number; if (isListRequest) { - (async () => { - const sshCiphers = ciphers.filter( - (cipher) => cipher.type === CipherType.SshKey && !cipher.isDeleted, - ); - const keys = sshCiphers.map((cipher) => { - return { - name: cipher.name, - privateKey: cipher.sshKey.privateKey, - cipherId: cipher.id, - }; - }); - await ipc.platform.sshAgent.setKeys(keys); - await ipc.platform.sshAgent.signRequestResponse(requestId, true); - })().catch((e) => this.logService.error("Failed to respond to SSH request", e)); + const sshCiphers = ciphers.filter( + (cipher) => cipher.type === CipherType.SshKey && !cipher.isDeleted, + ); + const keys = sshCiphers.map((cipher) => { + return { + name: cipher.name, + privateKey: cipher.sshKey.privateKey, + cipherId: cipher.id, + }; + }); + await ipc.platform.sshAgent.setKeys(keys); + await ipc.platform.sshAgent.signRequestResponse(requestId, true); return; } @@ -153,11 +152,8 @@ export class SshAgentService implements OnDestroy { this.i18nService.t("unknownApplication"), ); - return dialogRef.closed.pipe( - switchMap((result) => { - return ipc.platform.sshAgent.signRequestResponse(requestId, result); - }), - ); + const result = await firstValueFrom(dialogRef.closed); + return ipc.platform.sshAgent.signRequestResponse(requestId, result); }), takeUntil(this.destroy$), )