Skip to content
Draft
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
4c0e7a4
MVP rewrite
quexten Oct 16, 2025
e652b5d
Cleanup
quexten Oct 16, 2025
8272221
Cleanup
quexten Oct 16, 2025
458c865
Cleanup
quexten Oct 16, 2025
9d399fe
Cleanup
quexten Oct 16, 2025
7908056
Fix test
quexten Oct 16, 2025
1fb20e8
Cleanup
quexten Oct 16, 2025
0f36693
Fix loading
quexten Oct 16, 2025
e387125
Fix windows build
quexten Oct 16, 2025
d524785
Fix windows
quexten Oct 16, 2025
c7be485
Fix errors
quexten Oct 16, 2025
bc0819b
Cleanup
quexten Oct 16, 2025
b49e3c9
Apply cargo sort
quexten Oct 16, 2025
6aecbc4
Update message
quexten Oct 16, 2025
8696497
Fix cancel signing
quexten Oct 16, 2025
5818e0f
Run cargo sort
quexten Oct 16, 2025
be0011c
Merge branch 'main' into km/beeep/clean-agent-rewrite
quexten Oct 16, 2025
b92377b
Set flag to default off
quexten Oct 16, 2025
1905f3f
Re-sort with locked version
quexten Oct 16, 2025
3ae5786
Clean up windows
quexten Oct 16, 2025
5857782
Undo chromium importer changes
quexten Oct 16, 2025
64583e3
Add known hosts parsing and add comments
quexten Oct 17, 2025
fa6e86f
Fix build
quexten Oct 17, 2025
eebdc02
Cleanup
quexten Oct 17, 2025
6a7b2a1
Ssh sig support
quexten Oct 17, 2025
cd073ad
Undo proc reload
quexten Oct 17, 2025
dc155cd
Delete comment
quexten Oct 17, 2025
c222e7d
Undo change
quexten Oct 17, 2025
0e2412f
Undo change
quexten Oct 17, 2025
51a1d52
Undo change
quexten Oct 17, 2025
e21304c
Cleanup
quexten Oct 17, 2025
98b0e87
Merge branch 'main' into km/beeep/clean-agent-rewrite
quexten Oct 17, 2025
17ac7a3
Fix display impl
quexten Oct 17, 2025
c8775c6
Merge branch 'km/beeep/clean-agent-rewrite' of github.com:bitwarden/cโ€ฆ
quexten Oct 17, 2025
d87d772
Cargo fmt
quexten Oct 17, 2025
1d4167f
Cleanup
quexten Oct 17, 2025
fd1331d
Fix test
quexten Oct 17, 2025
1eb3a71
Fix verification of sigs and add test vectors
quexten Oct 17, 2025
96fa507
Fix clippy
quexten Oct 17, 2025
164c0bd
Fix
quexten Oct 17, 2025
5fbecdd
Fix
quexten Oct 17, 2025
4f07320
Fix integration run
quexten Oct 17, 2025
f1482b8
Force linux for test
quexten Oct 17, 2025
538bbc7
Cargo fmt
quexten Oct 17, 2025
90bcfa4
Fix build
quexten Oct 17, 2025
5ac9ae5
Run cargo fmt
quexten Oct 17, 2025
5b4dd5c
Move to tests
quexten Oct 17, 2025
cf3c676
Cargo fmt
quexten Oct 17, 2025
616ffb4
Cleanup
quexten Oct 17, 2025
4a9186c
Cargo format
quexten Oct 17, 2025
81771da
Address feedback
quexten Oct 31, 2025
f91b376
Apply feedback
quexten Oct 31, 2025
dd2074b
Remove allow unused
quexten Oct 31, 2025
c90b1c2
Removing debug logs
quexten Oct 31, 2025
2e2b251
Apply small improvement
quexten Oct 31, 2025
8887b6a
Rewrite pid matching
quexten Oct 31, 2025
7812458
Apply feedback
quexten Oct 31, 2025
069b3e1
Apply feedback
quexten Oct 31, 2025
141728f
Remove unused tag
quexten Oct 31, 2025
419320e
Split big function
quexten Oct 31, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
819 changes: 696 additions & 123 deletions apps/desktop/desktop_native/Cargo.lock

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion apps/desktop/desktop_native/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ members = [
"napi",
"process_isolation",
"proxy",
"ssh_agent",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great, it was on my mental list to bring this out of core.

"windows_plugin_authenticator"
]

Expand Down Expand Up @@ -55,7 +56,7 @@ security-framework = "=3.5.0"
security-framework-sys = "=2.15.0"
serde = "=1.0.209"
serde_json = "=1.0.127"
sha2 = "=0.10.8"
sha2 = "=0.10.9"
simplelog = "=0.12.2"
ssh-encoding = "=0.2.0"
ssh-key = { version = "=0.6.7", default-features = false }
Expand Down
28 changes: 14 additions & 14 deletions apps/desktop/desktop_native/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,20 @@ tracing = { workspace = true }
typenum = { workspace = true }
zeroizing-alloc = { workspace = true }

[target.'cfg(target_os = "linux")'.dependencies]
ashpd = { workspace = true }
libc = { workspace = true }
oo7 = { workspace = true }

zbus = { workspace = true, optional = true }
zbus_polkit = { workspace = true, optional = true }

[target.'cfg(target_os = "macos")'.dependencies]
core-foundation = { workspace = true, optional = true }
desktop_objc = { path = "../objc" }
security-framework = { workspace = true, optional = true }
security-framework-sys = { workspace = true, optional = true }

[target.'cfg(windows)'.dependencies]
widestring = { workspace = true, optional = true }
windows = { workspace = true, features = [
Expand All @@ -74,19 +88,5 @@ windows-future = { workspace = true }
[target.'cfg(windows)'.dev-dependencies]
keytar = { workspace = true }

[target.'cfg(target_os = "macos")'.dependencies]
core-foundation = { workspace = true, optional = true }
security-framework = { workspace = true, optional = true }
security-framework-sys = { workspace = true, optional = true }
desktop_objc = { path = "../objc" }

[target.'cfg(target_os = "linux")'.dependencies]
oo7 = { workspace = true }
libc = { workspace = true }
ashpd = { workspace = true }

zbus = { workspace = true, optional = true }
zbus_polkit = { workspace = true, optional = true }

[lints]
workspace = true
1 change: 1 addition & 0 deletions apps/desktop/desktop_native/napi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ napi = { workspace = true, features = ["async"] }
napi-derive = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }
ssh_agent = { path = "../ssh_agent" }
tokio = { workspace = true }
tokio-stream = { workspace = true }
tokio-util = { workspace = true }
Expand Down
26 changes: 26 additions & 0 deletions apps/desktop/desktop_native/napi/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -236,3 +236,29 @@ export declare namespace autotype {
export function getForegroundWindowTitle(): string
export function typeInput(input: Array<number>, keyboardShortcut: Array<string>): void
}
export declare namespace sshagent_v2 {
export interface PrivateKey {
privateKey: string
name: string
cipherId: string
}
export interface SshKey {
privateKey: string
publicKey: string
keyFingerprint: string
}
export interface SshUiRequest {
cipherId?: string
isList: boolean
processName: string
isForwarding: boolean
namespace?: string
}
export function serve(callback: (err: Error | null, arg: SshUiRequest) => any): Promise<SshAgentState>
export function stop(agentState: SshAgentState): void
export function isRunning(agentState: SshAgentState): boolean
export function setKeys(agentState: SshAgentState, newKeys: Array<PrivateKey>): void
export function lock(agentState: SshAgentState): void
export function clearKeys(agentState: SshAgentState): void
export class SshAgentState { }
}
191 changes: 191 additions & 0 deletions apps/desktop/desktop_native/napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1053,3 +1053,194 @@ pub mod autotype {
})
}
}

#[napi]
pub mod sshagent_v2 {
use std::sync::Arc;

use napi::{
bindgen_prelude::Promise,
threadsafe_function::{ErrorStrategy::CalleeHandled, ThreadsafeFunction},
};
use ssh_agent::agent::{ui_requester, PlatformListener};
use ssh_agent::{
self,
agent::{ui_requester::UiRequestMessage, BitwardenDesktopAgent},
memory::UnlockedSshItem,
protocol::types::KeyPair,
};
use tokio::{self, sync::Mutex};
use tracing::{error, info};

#[napi]
pub struct SshAgentState {
agent: BitwardenDesktopAgent,
}
Comment on lines +1081 to +1083
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like there aren't any impls on this struct and it has a single member. Is there a reason not to just use the BitwardenDesktopAgent itself?

It'd be helpful to add a doc comment describing what this struct's purpose is, if it's to be kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct has the #[napi] macro applied. BitwardenDesktopAgent doesn't, and the crate does not have access to napi as a dependency. Added a comment.


#[napi(object)]
pub struct PrivateKey {
pub private_key: String,
pub name: String,
pub cipher_id: String,
}

#[napi(object)]
pub struct SshKey {
pub private_key: String,
pub public_key: String,
pub key_fingerprint: String,
}

#[napi(object)]
pub struct SshUIRequest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does UI stand for in this case, does it stand for UserInterface? If yes, I'm wondering if we could improve the naming of it, could we just call it SshRequest? It could be something I'm missing but if this napi layer is the interface, it seems like we don't have to couple the request /response to a single external (i.e. we could write tests that submit requests that aren't actually coming from the Ui)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, SSH request sounds like the low level information that the agent itself gets from a client. UI request already has the information parsed, and additional metadata, such as the requesting process added. I don't know if there is a better name than this, but it is meant to be shown in a graphical user interface, the only implementation of which is the desktop client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, I don't really like this struct, and would much prefer to expose an enum not having to do this weird conversion. but, the intent of this PR is to keep the interface exposed out of desktop_native the same to be able to feature flag the move. We can't move everything at once.

pub cipher_id: Option<String>,
pub is_list: bool,
pub process_name: String,
pub is_forwarding: bool,
pub namespace: Option<String>,
}

#[allow(clippy::unused_async)] // FIXME: Remove unused async!
#[napi]
pub async fn serve(
callback: ThreadsafeFunction<SshUIRequest, CalleeHandled>,
) -> napi::Result<SshAgentState> {
let (auth_request_tx, mut auth_request_rx) =
tokio::sync::mpsc::channel::<ssh_agent::agent::ui_requester::UiRequestMessage>(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));
let ui_requester =
ui_requester::UiRequester::new(auth_request_tx, Arc::new(Mutex::new(auth_response_rx)));

tokio::spawn(async move {
let _ = ui_requester;

while let Some(request) = auth_request_rx.recv().await {
let cloned_response_tx_arc = auth_response_tx_arc.clone();
let cloned_callback = callback.clone();
tokio::spawn(async move {
let auth_response_tx_arc = cloned_response_tx_arc;
let callback = cloned_callback;

let js_request = match request.clone() {
UiRequestMessage::ListRequest {
request_id: _,
connection_info,
} => SshUIRequest {
cipher_id: None,
is_list: true,
process_name: connection_info.peer_info().process_name().to_string(),
is_forwarding: connection_info.is_forwarding(),
namespace: None,
},
UiRequestMessage::AuthRequest {
request_id: _,
connection_info,
cipher_id,
} => SshUIRequest {
cipher_id: Some(cipher_id),
is_list: false,
process_name: connection_info.peer_info().process_name().to_string(),
is_forwarding: connection_info.is_forwarding(),
namespace: None,
},
UiRequestMessage::SignRequest {
request_id: _,
connection_info,
cipher_id,
namespace,
} => SshUIRequest {
cipher_id: Some(cipher_id),
is_list: false,
process_name: connection_info.peer_info().process_name().to_string(),
is_forwarding: connection_info.is_forwarding(),
namespace: Some(namespace),
},
};

let promise_result: Result<Promise<bool>, napi::Error> =
callback.call_async(Ok(js_request)).await;
match promise_result {
Ok(promise_result) => match promise_result.await {
Ok(result) => {
let _ = auth_response_tx_arc
.lock()
.await
.send((request.id(), result))
.expect("should be able to send auth response to agent");
}
Err(e) => {
error!(error = %e, "Calling UI callback promise was rejected");
let _ = auth_response_tx_arc
.lock()
.await
.send((request.id(), false))
.expect("should be able to send auth response to agent");
}
},
Err(e) => {
error!(error = %e, "Calling UI callback could not create promise");
let _ = auth_response_tx_arc
.lock()
.await
.send((request.id(), false))
.expect("should be able to send auth response to agent");
}
}
});
}
});

let agent = BitwardenDesktopAgent::new(ui_requester);
let agent_copy = agent.clone();
PlatformListener::spawn_listeners(agent_copy);

Ok(SshAgentState { agent })
}

#[napi]
pub fn stop(agent_state: &mut SshAgentState) -> napi::Result<()> {
info!("Stopping SSH Agent");
agent_state.agent.stop();
Ok(())
}

#[napi]
pub fn is_running(agent_state: &mut SshAgentState) -> bool {
agent_state.agent.is_running()
}

#[napi]
pub fn set_keys(
agent_state: &mut SshAgentState,
new_keys: Vec<PrivateKey>,
) -> napi::Result<()> {
agent_state.agent.set_keys(
new_keys
.iter()
.filter_map(|k| {
let private_key =
ssh_agent::protocol::types::PrivateKey::try_from(k.private_key.clone())
.ok()?;
Some(UnlockedSshItem::new(
KeyPair::new(private_key, k.name.clone()),
k.cipher_id.clone(),
))
})
.collect(),
);
Ok(())
}

#[napi]
pub fn lock(agent_state: &mut SshAgentState) -> napi::Result<()> {
agent_state.agent.lock();
Ok(())
}

#[napi]
pub fn clear_keys(agent_state: &mut SshAgentState) -> napi::Result<()> {
set_keys(agent_state, vec![])
}
}
54 changes: 54 additions & 0 deletions apps/desktop/desktop_native/ssh_agent/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
[package]
name = "ssh_agent"
edition = { workspace = true }
license = { workspace = true }
version = { workspace = true }
publish = { workspace = true }

[features]
default = ["dep:windows"]
manual_test = []

[target.'cfg(windows)'.dependencies]
windows = { workspace = true, features = [
"Win32_System_Pipes",
], optional = true }
windows-future = { workspace = true }

[dependencies]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: We cannot use workspace deps. These conflict with core because core uses bitwarden-russh. There are direct conflicts and transitive dependency conflicts, so we pin these here. We can clean this up after dropping v1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense ,though it looks like there are workspace deps being used? Did you mean that we can only use some workspace deps?

Also noting that we should add a link to Jira ticket for FF removal to make sure we circle back and take care of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually, we prefer to have all deps in the workspace root, and never in specific sub crates. In this case, the specific set of dependencies that must be different in the v2 version of SSH agent, is set in the ssh agent crate. This means that the v1 agent still uses the outdated old deps, while the v2 agent uses the new deps. Both deps are compiled into the binary, but without conflicting.

It is fine for the ssh agent crate to use workspace deps as long as they are not conflicting. The ones overwritten / pinned here were conflicting.

I will make the FF when all other things look good, let's keep this comment open and unresolved.

anyhow = { workspace = true }
base64 = { workspace = true }
block-padding = { version = "=0.4.0-rc.4" }
byteorder = { workspace = true }
bytes = { workspace = true }
ed25519 = { workspace = true, features = ["pkcs8"] }
ed25519-dalek = { version = "2.2.0", features = ["rand_core"] }
futures = { workspace = true }
homedir = { workspace = true }
inout = { version = "=0.2.0-rc.6" }
log = { workspace = true }
num_enum = "0.7.4"
p256 = "0.13.2"
p384 = "0.13.1"
p521 = "0.13.3"
pin-project = { workspace = true }
rsa = { version = "=0.10.0-rc.8", features = ["sha2"] }
sha2 = "0.10.9"
signature = "3.0.0-rc.4"
ssh-encoding = "=0.3.0-rc.2"
ssh-key = { version = "=0.7.0-rc.3", features = [
"encryption",
"ed25519",
"rsa",
"ecdsa",
"getrandom",
] }
sysinfo = { workspace = true, features = ["windows"] }
test-log = "0.2.18"
tokio = { workspace = true, features = ["io-util", "sync", "macros", "net", "full"] }
tokio-util = { workspace = true, features = ["codec"] }
tracing = { workspace = true }
tracing-subscriber.workspace = true

[lints]
workspace = true
Loading
Loading