Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[PM-14863] Force unlock when keys are cleared / on first unlock and fix account switching behavior #11994

Merged
merged 13 commits into from
Dec 2, 2024
Merged
8 changes: 5 additions & 3 deletions apps/desktop/desktop_native/core/src/ssh_agent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Mutex<tokio::sync::broadcast::Receiver<(u32, bool)>>>,
request_id: Arc<Mutex<u32>>,
/// before first unlock, or after account switching, listing keys should require an unlock to get a list of public keys
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion apps/desktop/desktop_native/core/src/ssh_agent/unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Mutex<tokio::sync::broadcast::Receiver<(u32, bool)>>>,
) -> Result<Self, anyhow::Error> {
let agent = BitwardenDesktopAgent {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Mutex<tokio::sync::broadcast::Receiver<(u32, bool)>>>,
) -> Result<Self, anyhow::Error> {
let agent_state = BitwardenDesktopAgent {
Expand Down
2 changes: 1 addition & 1 deletion apps/desktop/desktop_native/napi/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export declare namespace sshagent {
status: SshKeyImportStatus
sshKey?: SshKey
}
export function serve(callback: (err: Error | null, arg: string) => any): Promise<SshAgentState>
export function serve(callback: (err: Error | null, arg0: string, arg1: boolean) => any): Promise<SshAgentState>
export function stop(agentState: SshAgentState): void
export function isRunning(agentState: SshAgentState): boolean
export function setKeys(agentState: SshAgentState, newKeys: Array<PrivateKey>): void
Expand Down
8 changes: 4 additions & 4 deletions apps/desktop/desktop_native/napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,15 +247,15 @@ pub mod sshagent {

#[napi]
pub async fn serve(
callback: ThreadsafeFunction<String, CalleeHandled>,
callback: ThreadsafeFunction<(String, bool), CalleeHandled>,
) -> napi::Result<SshAgentState> {
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();
Expand All @@ -266,7 +266,7 @@ pub mod sshagent {
let auth_response_tx_arc = cloned_response_tx_arc;
let callback = cloned_callback;
let promise_result: Result<Promise<bool>, 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) => {
Expand Down
3 changes: 2 additions & 1 deletion apps/desktop/src/platform/main/main-ssh-agent.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
init() {
// handle sign request passing to UI
sshagent
.serve(async (err: Error, cipherId: string) => {
.serve(async (err: Error, cipherId: string, isListRequest: boolean) => {

Check warning on line 30 in apps/desktop/src/platform/main/main-ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/main/main-ssh-agent.service.ts#L30

Added line #L30 was not covered by tests
// clear all old (> SIGN_TIMEOUT) requests
this.requestResponses = this.requestResponses.filter(
(response) => response.timestamp > new Date(Date.now() - this.SIGN_TIMEOUT),
Expand All @@ -37,6 +37,7 @@
const id_for_this_request = this.request_id;
this.messagingService.send("sshagent.signrequest", {
cipherId,
isListRequest,
requestId: id_for_this_request,
});

Expand Down Expand Up @@ -112,9 +113,9 @@
}
});

ipcMain.handle("sshagent.clearkeys", async (event: any) => {

Check warning on line 116 in apps/desktop/src/platform/main/main-ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/main/main-ssh-agent.service.ts#L116

Added line #L116 was not covered by tests
if (this.agentState != null) {
sshagent.clearKeys(this.agentState);

Check warning on line 118 in apps/desktop/src/platform/main/main-ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/main/main-ssh-agent.service.ts#L118

Added line #L118 was not covered by tests
}
});
}
Expand Down
47 changes: 22 additions & 25 deletions apps/desktop/src/platform/services/ssh-agent.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
withLatestFrom,
} from "rxjs";

import { AccountService } from "@bitwarden/common/auth/abstractions/account.service";

Check warning on line 21 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L21

Added line #L21 was not covered by tests
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";
Expand Down Expand Up @@ -54,7 +54,7 @@
private i18nService: I18nService,
private desktopSettingsService: DesktopSettingsService,
private configService: ConfigService,
private accountService: AccountService,

Check warning on line 57 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L57

Added line #L57 was not covered by tests
) {}

async init() {
Expand Down Expand Up @@ -115,39 +115,36 @@
),
),
// 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;

Check warning on line 120 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L120

Added line #L120 was not covered by tests
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(

Check warning on line 125 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L124-L125

Added lines #L124 - L125 were not covered by tests
(cipher) => cipher.type === CipherType.SshKey && !cipher.isDeleted,
);
const keys = sshCiphers.map((cipher) => {
return {

Check warning on line 129 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L128-L129

Added lines #L128 - L129 were not covered by tests
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));
coroiu marked this conversation as resolved.
Show resolved Hide resolved
return;

Check warning on line 138 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L135-L138

Added lines #L135 - L138 were not covered by tests
}

if (ciphers === undefined) {
return of(false).pipe(
switchMap((result) =>
ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result)),
),
);
ipc.platform.sshAgent

Check warning on line 142 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L142

Added line #L142 was not covered by tests
.signRequestResponse(requestId, false)
.catch((e) => this.logService.error("Failed to respond to SSH request", e));

Check warning on line 144 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L144

Added line #L144 was not covered by tests
}

const cipher = ciphers.find((cipher) => cipher.id == cipherId);

Check warning on line 147 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L147

Added line #L147 was not covered by tests
coroiu marked this conversation as resolved.
Show resolved Hide resolved

ipc.platform.focusWindow();
const dialogRef = ApproveSshRequestComponent.open(
Expand All @@ -158,7 +155,7 @@

return dialogRef.closed.pipe(
switchMap((result) => {
return ipc.platform.sshAgent.signRequestResponse(requestId, Boolean(result));
return ipc.platform.sshAgent.signRequestResponse(requestId, result);

Check warning on line 158 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L158

Added line #L158 was not covered by tests
}),
);
}),
Expand All @@ -166,13 +163,13 @@
)
.subscribe();

this.accountService.activeAccount$

Check warning on line 166 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L166

Added line #L166 was not covered by tests
.pipe(skip(1), takeUntil(this.destroy$))
.subscribe((account) => {
this.logService.info("Active account changed, clearing SSH keys");
ipc.platform.sshAgent

Check warning on line 170 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L169-L170

Added lines #L169 - L170 were not covered by tests
.clearKeys()
.catch((e) => this.logService.error("Failed to clear SSH keys", e));

Check warning on line 172 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L172

Added line #L172 was not covered by tests
});

combineLatest([
Expand All @@ -182,7 +179,7 @@
.pipe(
concatMap(async ([, enabled]) => {
if (!enabled) {
await ipc.platform.sshAgent.clearKeys();

Check warning on line 182 in apps/desktop/src/platform/services/ssh-agent.service.ts

View check run for this annotation

Codecov / codecov/patch

apps/desktop/src/platform/services/ssh-agent.service.ts#L182

Added line #L182 was not covered by tests
return;
}

Expand Down
Loading