[BEEEP|PM27032] Clean ssh agent rewrite #16265

quexten · 2025-09-02T21:37:47Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27032

📔 Objective

This PR re-implements the SSH agent completely, and does the following changes:

Drop bitwarden-russh, and keep all code in desktop_native. It is a fresh codebase.
- bitwarden-russh will be archived and unmaintained
Clean up the code to be more idiomatic rust.
- For instance, use newtypes, make clear parsers and serializers
- Link to the relevant SSH sections
- This should include testvectors for parsing of each message
Make the code readable
Make the code easy to reason about

This work is feature flagged. After the code is rolled out, bitwarden-russh can be dropped entirely and the module in core can be dropped.

This re-write also fixes a swath of bugs, not mentioned individually here, and we probably need to revisit some of the backlog. One of these is signing with RSA on sha256 / 512.

Note: The error handling is not optimal in this PR, but adding proper error types here seems like it would even more increase the scope, so unless reviewers consider it blocking, I would suggest this as follow-up tech debt.

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

github-actions · 2025-09-02T21:47:20Z

Checkmarx One – Scan Summary & Details – bd6f927f-903e-4edf-aa62-1073b96fbf03

Great job! No new security vulnerabilities introduced in this pull request

codecov · 2025-09-02T21:47:45Z

Codecov Report

❌ Patch coverage is 19.88773% with 999 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.92%. Comparing base (5281da8) to head (4a9186c).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...top/desktop_native/ssh_agent/src/protocol/types.rs	44.16%	177 Missing ⚠️
apps/desktop/desktop_native/napi/src/lib.rs	0.00%	115 Missing ⚠️
...op_native/ssh_agent/src/protocol/agent_listener.rs	0.00%	82 Missing ⚠️
.../desktop_native/ssh_agent/src/protocol/requests.rs	47.01%	80 Missing ⚠️
...esktop_native/ssh_agent/src/agent/desktop_agent.rs	0.00%	78 Missing ⚠️
...desktop/desktop_native/ssh_agent/src/memory/mod.rs	0.00%	75 Missing ⚠️
...desktop_native/ssh_agent/src/agent/ui_requester.rs	0.00%	64 Missing ⚠️
...p/desktop_native/ssh_agent/src/protocol/replies.rs	0.00%	57 Missing ⚠️
...ve/ssh_agent/src/transport/unix_listener_stream.rs	0.00%	54 Missing ⚠️
...esktop/src/autofill/main/main-ssh-agent.service.ts	0.00%	50 Missing ⚠️
... and 7 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16265      +/-   ##
==========================================
- Coverage   39.16%   38.92%   -0.24%     
==========================================
  Files        3456     3469      +13     
  Lines       97968    99194    +1226     
  Branches    14730    14739       +9     
==========================================
+ Hits        38368    38615     +247     
- Misses      57932    58910     +978     
- Partials     1668     1669       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

neuronull

I've taken some solid time to review the high level here, and though I'm still coming up to speed on the protocol, I really like the move to bring the agent implementation into the client repo, very on board with that. I also think the code is well organized into appropriate modules. It'd be great to see unit tests (or integration tests I guess 🤔 ), that exercise the combinations of requests+replies to assert expected behavior.

I'm holding off on giving a full code review with detailed suggestions, can do that when it's fully ready. But the high level approach looks good to me and a great improvement.

I fully support this effort!

quexten · 2025-10-17T10:28:59Z

@neuronull I've added a bit of unit tests for at least parsing / signature logic.
For integration tests I wanted some opinion. I don't know of a good way to do this in rust without pulling in a large amount of deps, including russh (mainline) which we wanted to drop.
So instead, scripting it together such that we just run sshd / ssh could work. We can run an ssh server in userspace, then use an ssh client and git commits to try using it, all via https://doc.rust-lang.org/std/process/struct.Command.html calls. Does this sound reasonable?

~~This would use a custom MockBitwardenDesktopSshAgent implementation, so the UI / IPC logic would still stay untested, but at least the core agent would have integration tests?~~

Integration test is added (runs on linux only).

sonarqubecloud · 2025-10-17T13:14:05Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
6.6% Duplication on New Code

See analysis details on SonarQube Cloud

neuronull

Did a first pass... after a few hours, I need to take a break. Will publish what I have and try to get to the rest tomorrow.

neuronull · 2025-10-22T17:33:16Z

apps/desktop/desktop_native/Cargo.toml

    "napi",
    "process_isolation",
    "proxy",
+    "ssh_agent",


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

neuronull · 2025-10-22T17:34:36Z

apps/desktop/desktop_native/ssh_agent/Cargo.toml

+], optional = true }
+windows-future = { workspace = true }
+
+[dependencies]


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.

neuronull · 2025-10-22T17:42:16Z

apps/desktop/desktop_native/ssh_agent/src/lib.rs

+pub mod agent;
+pub mod knownhosts;
+pub mod memory;
+pub mod protocol;
+pub mod transport;


⛏️ nit on scoping. Seems like these could be pub(crate) ? Also, it'd be more clean to expose the structs/functions that actually need to be used externally to this library, for example

mod protocol; pub(crate) use protocol::types::{KeyPair, PrivateKey};

neuronull · 2025-10-22T17:43:35Z

apps/desktop/desktop_native/ssh_agent/tests/integration.rs

+
+#[cfg(target_os = "linux")]
+#[tokio::test]
+#[test_log::test]


is this necessary?

Which part is this referring to? The test logging? If so, yes, by default the tracing does not log in tests.

neuronull · 2025-10-22T17:44:38Z

apps/desktop/desktop_native/ssh_agent/tests/integration.rs

@@ -0,0 +1,208 @@
+#[cfg(target_os = "linux")]


It looks like all elements in this file have this gate. Seems like we could do this more globally at a different level.

apps/desktop/desktop_native/ssh_agent/src/lib.rs

neuronull · 2025-10-22T20:15:23Z

apps/desktop/desktop_native/ssh_agent/src/transport/peer_info.rs

+    #[cfg(windows)]
+    NamedPipe,
+    UnixSocket,


seems off to have a gate on the named pipe but not the unix socket

The intent is to have this reflect the transports that could be implemented on each platform. Windows has unix socket support on cygwin, and WSL1/2. However, rusts default unix socket implementation does not support Windows UDS. There are some alternative libraries, but I have not looked into that yet / it is a future feature request to support it.

Still, that's why it makes sense to have unix socket on Windows non-flagged.

apps/desktop/desktop_native/ssh_agent/src/transport/peer_info.rs

neuronull · 2025-10-22T20:21:47Z

apps/desktop/desktop_native/ssh_agent/src/transport/named_pipe_listener_stream.rs

+        agent: BitwardenDesktopAgent,
+    ) -> Result<(), anyhow::Error> {
+        info!("Starting SSH Named Pipe listener");
+        let (tx, rx) = tokio::sync::mpsc::channel(16);


does it imply we have a limit of 16 concurrent connections? can you add a comment describing what these channels are used for

neuronull · 2025-10-22T20:29:36Z

@neuronull I've added a bit of unit tests for at least parsing / signature logic. For integration tests I wanted some opinion. I don't know of a good way to do this in rust without pulling in a large amount of deps, including russh (mainline) which we wanted to drop. So instead, scripting it together such that we just run sshd / ssh could work. We can run an ssh server in userspace, then use an ssh client and git commits to try using it, all via https://doc.rust-lang.org/std/process/struct.Command.html calls. Does this sound reasonable?

~~This would use a custom MockBitwardenDesktopSshAgent implementation, so the UI / IPC logic would still stay untested, but at least the core agent would have integration tests?~~

Integration test is added (runs on linux only).

I missed this comment, just saw it. I flagged the integration test in one of my review comments. Firstly, I think it's awesome to be introducing integration tests for this, so thank you. I do think we should make sure that they are executed in CI and done in a separate CI job from the unit tests.

Long term I'd like to see our desktop integration tests run in their own GH workflow, and we can detect whether to run a given int test based on what files were changed in the PR.

neuronull self-requested a review September 3, 2025 14:51

neuronull reviewed Oct 15, 2025

View reviewed changes

quexten added 3 commits October 16, 2025 11:50

MVP rewrite

4c0e7a4

Cleanup

e652b5d

Cleanup

8272221

quexten force-pushed the km/beeep/clean-agent-rewrite branch from c5e0bdf to 8272221 Compare October 16, 2025 09:53

quexten added 9 commits October 16, 2025 12:04

Cleanup

458c865

Cleanup

9d399fe

Fix test

7908056

Cleanup

1fb20e8

Fix loading

0f36693

Fix windows build

e387125

Fix windows

d524785

Fix errors

c7be485

Cleanup

bc0819b

quexten changed the title ~~[Poc/RFC] Clean ssh agent rewrite~~ [BEEEP|PM27032] Clean ssh agent rewrite Oct 16, 2025

quexten added 12 commits October 16, 2025 14:53

Apply cargo sort

b49e3c9

Update message

6aecbc4

Fix cancel signing

8696497

Run cargo sort

5818e0f

Merge branch 'main' into km/beeep/clean-agent-rewrite

be0011c

Set flag to default off

b92377b

Re-sort with locked version

1905f3f

Clean up windows

3ae5786

Undo chromium importer changes

5857782

Add known hosts parsing and add comments

64583e3

Fix build

fa6e86f

Cleanup

eebdc02

quexten requested a review from a team as a code owner October 17, 2025 10:22

quexten requested a review from dereknance October 17, 2025 10:22

quexten added 11 commits October 17, 2025 14:02

Fix

164c0bd

Fix

5fbecdd

Fix integration run

4f07320

Force linux for test

f1482b8

Cargo fmt

538bbc7

Fix build

90bcfa4

Run cargo fmt

5ac9ae5

Move to tests

5b4dd5c

Cargo fmt

cf3c676

Cleanup

616ffb4

Cargo format

4a9186c

neuronull mentioned this pull request Oct 22, 2025

fix rsa key signing bitwarden/bitwarden-russh#14

Closed

neuronull requested changes Oct 22, 2025

View reviewed changes

quexten removed the request for review from dereknance October 23, 2025 23:05

quexten added 10 commits October 31, 2025 11:55

Address feedback

81771da

Apply feedback

f91b376

Remove allow unused

dd2074b

Removing debug logs

c90b1c2

Apply small improvement

2e2b251

Rewrite pid matching

8887b6a

Apply feedback

7812458

Apply feedback

069b3e1

Remove unused tag

141728f

Split big function

419320e

quexten marked this pull request as draft November 12, 2025 14:52

[BEEEP|PM27032] Clean ssh agent rewrite #16265

Are you sure you want to change the base?

[BEEEP|PM27032] Clean ssh agent rewrite #16265

Conversation

quexten commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

codecov bot commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

neuronull left a comment

Choose a reason for hiding this comment

Uh oh!

quexten commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 17, 2025

Quality Gate passed

Uh oh!

neuronull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neuronull commented Oct 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quexten commented Sep 2, 2025 •

edited

Loading

github-actions bot commented Sep 2, 2025 •

edited

Loading

codecov bot commented Sep 2, 2025 •

edited

Loading

quexten commented Oct 17, 2025 •

edited

Loading