[PM-31760] Implement sign() for PrivateKey

[coltonhurst]

🎟️ Tracking

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

📔 Objective

• Implement sign() for PrivateKey in the crypto module for SSH v2.
• Add tests

There were two main approaches I looked at taking:

1. (this pr) Pulling in the signature::Signer trait that ssh_key has implementations for. While this is a new dependency that will need approval, it's still part of RustCrypto; it comes from their traits repository.
2. (not this pr) Attempting to sign without pulling in a new dependency; this would use ssh_key's sign function on their own private key. This returns their SshSig type though, which is more high level than we probably want at this implementation level.

A note on versions:

• The signature crate recently released v3. The latest stable version released was v2.2. However, we can't use v3 yet, as ssh_key doesn't yet have a release with signature v3 supported. This is being worked on, though! (link: ssh-key: bump signature crate dependency to v3 RustCrypto/SSH#501)

🧪 Testing

No QA needed at this stage. Only the added unit tests were run.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.41%. Comparing base (fe720fd) to head (32ac4d6).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #20523      +/-   ##
==========================================
+ Coverage   47.40%   47.41%   +0.01%     
==========================================
  Files        3972     3972              
  Lines      121413   121445      +32     
  Branches    18635    18635              
==========================================
+ Hits        57558    57586      +28     
- Misses      59490    59494       +4     
  Partials     4365     4365              

☔ 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.

[coltonhurst]

Opening this as ready for review. It is ready for engineering review (FYI @neuronull & @quexten). However, because this PR adds a dependency, the dependency will need to be reviewed by AppSec. If an approach using the proposed new dependency is the desired & approved one, then I'll submit the dep for AppSec's review.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds a sign() method to the PrivateKey enum in the desktop SSH agent's crypto module, dispatching to the ssh_key keypair implementations for Ed25519 and RSA via the signature::Signer trait. The change is well-scoped, includes algorithm-identity and end-to-end verify tests for both supported algorithms, and documents the in-memory-only signing contract along with the external-signer caveat. Prior reviewer feedback on placement of the signature dependency, sign vs try_sign choice, and doc-comment wording has been addressed in subsequent commits.

Code Review Details

No actionable findings on the current head.

Notes for context (not findings):

• signature = "=2.2.0" is promoted to a direct dependency of ssh_agent; it is already present transitively (via ed25519, ed25519-dalek, rsa, ssh-key), so this is not a net-new dependency to the workspace.
• The Signer::sign trait panics on internal signing failure by contract; this trade-off was discussed in earlier review threads and the chosen behavior is documented inline at apps/desktop/desktop_native/ssh_agent/src/crypto/mod.rs:38-51. The new PrivateKey::sign is not yet called from production code paths.
• Test-side .unwrap() usage matches the established pattern in this module.

[quexten]

@coltonhurst @neuronull signature does not require an AppSec review. It already is a (transitive) dependency of both sdk-internal but also desktop-native. All you are doing is making it a direct dependency. It is not a net new dependency.

From the confluence:

This applies to any third-party library, open-source package, or external software component not already in use in the target codebase.

signature is not a library that is not already use in the target codebase, since it is already used as part of e.g. the eliptic curve implementations.

[coltonhurst]

@coltonhurst @neuronull signature does not require an AppSec review. It already is a (transitive) dependency of both sdk-internal but also desktop-native. All you are doing is making it a direct dependency. It is not a net new dependency.

From the confluence:

This applies to any third-party library, open-source package, or external software component not already in use in the target codebase.

signature is not a library that is not already use in the target codebase, since it is already used as part of e.g. the eliptic curve implementations.

Yeah, I was aware it was a transitive dependency, was going to submit it just to be safe. I'll seek clarity on that.

[coltonhurst]

@coltonhurst @neuronull signature does not require an AppSec review. It already is a (transitive) dependency of both sdk-internal but also desktop-native. All you are doing is making it a direct dependency. It is not a net new dependency.
From the confluence:

This applies to any third-party library, open-source package, or external software component not already in use in the target codebase.

signature is not a library that is not already use in the target codebase, since it is already used as part of e.g. the eliptic curve implementations.

Yeah, I was aware it was a transitive dependency, was going to submit it just to be safe. I'll seek clarity on that.

Update from AppSec; we are good to promote signature to a direct dependency.

[neuronull]

Thanks for making changes to use the fallible function and documentation. Looks good overall, just some opinion takes.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud