SSH agent rxjs tweaks #11148

coroiu · 2024-09-19T10:50:05Z

🎟️ Tracking

📔 Objective

Just some suggested changes to #10293

📸 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

codecov · 2024-09-19T10:58:24Z

Codecov Report

Attention: Patch coverage is 0% with 20 lines in your changes missing coverage. Please review.

Project coverage is 34.80%. Comparing base (b4af394) to head (bd31d02).
Report is 2 commits behind head on auth/beeep/ssh-agent.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...desktop/src/platform/services/ssh-agent.service.ts	0.00%	20 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           auth/beeep/ssh-agent   #11148      +/-   ##
========================================================
- Coverage                 34.81%   34.80%   -0.01%     
========================================================
  Files                      2681     2681              
  Lines                     82135    82134       -1     
  Branches                  15466    15465       -1     
========================================================
- Hits                      28594    28588       -6     
- Misses                    52637    52642       +5     
  Partials                    904      904

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

github-actions · 2024-09-19T10:59:12Z

Checkmarx One – Scan Summary & Details – 6758cf12-19b3-4272-b0af-db69da7e1cbf

New Issues

Issue	Source File / Package	Checkmarx Insight
Client_Privacy_Violation	/apps/desktop/src/vault/app/vault/view.component.html: 583	Attack Vector
Client_Privacy_Violation	/apps/desktop/src/vault/app/vault/view.component.html: 60	Attack Vector
Client_Privacy_Violation	/apps/desktop/src/vault/app/vault/view.component.html: 56	Attack Vector
Client_Privacy_Violation	/apps/desktop/src/vault/app/vault/view.component.html: 50	Attack Vector

coroiu · 2024-09-19T11:13:40Z

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

+        //   - If the vault is unlocked, we will continue with the flow.
+        // switchMap is used here to prevent multiple requests from being processed at the same time,
+        // and will cancel the previous request if a new one is received.
+        switchMap(([message, status]) => {


I just realized that I did a 180 turn here and went back to switchMap which is probably not what we want. The thing is that something is caching theses requests anyways so it doesn't actually seemt to have any effect.

Thank you for the PR! I don't think replacing the requests is the right UX though. It feels like they should be queued. I'll play around with the code a bit.

Yeah no I agree, I think my brain just had a glitch 🤷‍♂️

Ok, playing around with this, it does not actually matter, because requests are serialized already in the rust layer. I didn't check thoroughly, but I believe this is because the confirm function for BitwardenDesktopAgent locks the get_ui_response receiver, and so the next confirm request needs to wait anyways.

I'm OK with this behavior for now, though this means that timing out requests does not work as intended. The proper solution (I think) would involve much more involved communication, where we track request id's on request / response and use something like tokio::sync::broadcast::channel to have all concurrent calls of confirm listen to the response and filter by request id?

I'm not sure whether it's worth it to implement here or leave that for a later refactor. Thoughts?

Actually, should probably fix this.

quexten · 2024-09-24T10:46:58Z

Merging and continuing work on the agent branch. Thanks for the changes!

* Add ssh agent, generator & import * Move ssh agent code to bitwarden-russh crate * Remove generator component * Cleanup * Cleanup * Remove left over sshGenerator reference * Cleanup * Add documentation to sshkeyimportstatus * Fix outdated variable name * Update apps/desktop/src/platform/preload.ts Co-authored-by: Andreas Coroiu <[email protected]> * Rename renderersshagent * Rename MainSshAgentService * Improve clarity of 'id' variables being used * Improve clarity of 'id' variables being used * Update apps/desktop/src/vault/app/vault/add-edit.component.html Co-authored-by: Andreas Coroiu <[email protected]> * Fix outdated cipher/messageid names * Rename SSH to Ssh * Make agent syncing more reactive * Move constants to top of class * Make sshkey cipher filtering clearer * Add stricter equality check on ssh key unlock * Fix build and messages * Fix incorrect featureflag name * Replace anonymous async function with switchmap pipe * Fix build * Update apps/desktop/desktop_native/napi/src/lib.rs Co-authored-by: Andreas Coroiu <[email protected]> * Revert incorrectly renamed 'Ssh' usages to SSH * Run cargo fmt * Clean up ssh agent sock path logic * Cleanup and split to platform specific files * Small cleanup * Pull out generator and importer into core * Rename renderersshagentservice to sshagentservice * Rename cipheruuid to cipher_id * Drop ssh dependencies from napi crate * Clean up windows build * Small cleanup * Small cleanup * Cleanup * Add rxjs pipeline for agent services * [PM-12555] Pkcs8 sshkey import & general ssh key import tests (#11048) * Add pkcs8 import and tests * Add key type unsupported error * Remove unsupported formats * Remove code for unsupported formats * Fix encrypted pkcs8 import * Add ed25519 pkcs8 unencrypted test file * SSH agent rxjs tweaks (#11148) * feat: rewrite sshagent.signrequest as purely observable * feat: fail the request when unlock times out * chore: clean up, add some clarifying comments * chore: remove unused dependency * fix: result `undefined` crashing in NAPI -> Rust * Allow concurrent SSH requests in rust * Remove unwraps * Cleanup and add init service init call * Fix windows * Fix timeout behavior on locked vault --------- Co-authored-by: Andreas Coroiu <[email protected]>

* [PM-10395] Add new item type ssh key (#10360) * Implement ssh-key cipher type * Fix linting * Fix edit and view components for ssh-keys on desktop * Fix tests * Remove ssh key type references * Remove add ssh key option * Fix typo * Add tests * [PM-10399] Add ssh key import export for bitwarden json (#10529) * Add ssh key import export for bitwarden json * Remove key type from ssh key export * [PM-10406] Add privatekey publickey and fingerprint to both add-edit and view co… (#11046) * Add privatekey publickey and fingerprint to both add-edit and view components * Remove wrong a11y title * Fix testid * [PM-10098] SSH Agent & SSH Key creation for Bitwarden Desktop (#10293) * Add ssh agent, generator & import * Move ssh agent code to bitwarden-russh crate * Remove generator component * Cleanup * Cleanup * Remove left over sshGenerator reference * Cleanup * Add documentation to sshkeyimportstatus * Fix outdated variable name * Update apps/desktop/src/platform/preload.ts Co-authored-by: Andreas Coroiu <[email protected]> * Rename renderersshagent * Rename MainSshAgentService * Improve clarity of 'id' variables being used * Improve clarity of 'id' variables being used * Update apps/desktop/src/vault/app/vault/add-edit.component.html Co-authored-by: Andreas Coroiu <[email protected]> * Fix outdated cipher/messageid names * Rename SSH to Ssh * Make agent syncing more reactive * Move constants to top of class * Make sshkey cipher filtering clearer * Add stricter equality check on ssh key unlock * Fix build and messages * Fix incorrect featureflag name * Replace anonymous async function with switchmap pipe * Fix build * Update apps/desktop/desktop_native/napi/src/lib.rs Co-authored-by: Andreas Coroiu <[email protected]> * Revert incorrectly renamed 'Ssh' usages to SSH * Run cargo fmt * Clean up ssh agent sock path logic * Cleanup and split to platform specific files * Small cleanup * Pull out generator and importer into core * Rename renderersshagentservice to sshagentservice * Rename cipheruuid to cipher_id * Drop ssh dependencies from napi crate * Clean up windows build * Small cleanup * Small cleanup * Cleanup * Add rxjs pipeline for agent services * [PM-12555] Pkcs8 sshkey import & general ssh key import tests (#11048) * Add pkcs8 import and tests * Add key type unsupported error * Remove unsupported formats * Remove code for unsupported formats * Fix encrypted pkcs8 import * Add ed25519 pkcs8 unencrypted test file * SSH agent rxjs tweaks (#11148) * feat: rewrite sshagent.signrequest as purely observable * feat: fail the request when unlock times out * chore: clean up, add some clarifying comments * chore: remove unused dependency * fix: result `undefined` crashing in NAPI -> Rust * Allow concurrent SSH requests in rust * Remove unwraps * Cleanup and add init service init call * Fix windows * Fix timeout behavior on locked vault --------- Co-authored-by: Andreas Coroiu <[email protected]> * Fix libc dependency being duplicated * fix SSH casing (#11840) * Move ssh agent behind feature flag (#11841) * Move ssh agent behind feature flag * Add separate flag for ssh agent * [PM-14215] fix unsupported key type error message (#11788) * Fix error message for import of unsupported ssh keys * Use triple equals in add-edit component for ssh keys --------- Co-authored-by: Andreas Coroiu <[email protected]> Co-authored-by: aj-bw <[email protected]>

coroiu added 4 commits September 19, 2024 12:34

feat: rewrite sshagent.signrequest as purely observable

072c699

feat: fail the request when unlock times out

affeba2

chore: clean up, add some clarifying comments

ed907a1

chore: remove unused dependency

dff43ec

coroiu requested a review from quexten September 19, 2024 10:50

coroiu requested a review from a team as a code owner September 19, 2024 10:50

coroiu requested review from justindbaur and removed request for a team and justindbaur September 19, 2024 10:50

coroiu commented Sep 19, 2024

View reviewed changes

coroiu mentioned this pull request Sep 19, 2024

[PM-10098] SSH Agent & SSH Key creation for Bitwarden Desktop #10293

Merged

fix: result undefined crashing in NAPI -> Rust

bd31d02

quexten merged commit 75ae8ba into auth/beeep/ssh-agent Sep 24, 2024
33 of 35 checks passed

quexten deleted the acoroiu/ssh-agent-rxjs-tweaks branch September 24, 2024 10:47

quexten mentioned this pull request Oct 1, 2024

PM-10393 SSH keys #10825

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSH agent rxjs tweaks #11148

SSH agent rxjs tweaks #11148

coroiu commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

coroiu Sep 19, 2024

quexten Sep 23, 2024

coroiu Sep 23, 2024

quexten Sep 24, 2024 •

edited

Loading

quexten Sep 24, 2024

quexten commented Sep 24, 2024

SSH agent rxjs tweaks #11148

SSH agent rxjs tweaks #11148

Conversation

coroiu commented Sep 19, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

github-actions bot commented Sep 19, 2024 • edited Loading

New Issues

coroiu Sep 19, 2024

Choose a reason for hiding this comment

quexten Sep 23, 2024

Choose a reason for hiding this comment

coroiu Sep 23, 2024

Choose a reason for hiding this comment

quexten Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

quexten Sep 24, 2024

Choose a reason for hiding this comment

quexten commented Sep 24, 2024

coroiu commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

quexten Sep 24, 2024 •

edited

Loading