[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.37%. Comparing base (2f72b6d) to head (22629d5).
⚠️ Report is 38 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #20523      +/-   ##
==========================================
+ Coverage   47.02%   47.37%   +0.34%     
==========================================
  Files        3951     3960       +9     
  Lines      119648   120760    +1112     
  Branches    18325    18532     +207     
==========================================
+ Hits        56266    57207     +941     
- Misses      59141    59207      +66     
- Partials     4241     4346     +105     

☔ 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

Reviewed the implementation of PrivateKey::sign() in the desktop SSH agent's crypto module, which delegates to the signature::Signer trait via the ssh-key crate's keypair types. The change is well-scoped: a single signing helper for Ed25519 and RSA keys, four unit tests covering both algorithm identification and round-trip sign/verify validation, and explicit doc comments calling out the in-memory key assumption (hardware-backed keys are not supported). Prior reviewer feedback on try_sign vs sign and clone/copy commentary has been addressed in commits 22629d5a3 and 1cf4d21ed.

Code Review Details

No code-level findings.

Dependency Changes

Package	Change	Ecosystem
signature	New (=2.2.0)	Cargo

Note: signature 2.2.0 is already present in Cargo.lock on main as a transitive dependency (pulled in via ssh-key/ed25519-dalek) with an identical checksum. This PR promotes it to a direct dependency in the ssh_agent crate. The supply chain risk profile is unchanged.

• ❓ : The PR description acknowledges "this is a new dependency that will need approval" but does not reference a VULN task or explicit AppSec sign-off. Since the crate is part of RustCrypto and already in the build tree, this may be a low-friction approval, but the dependency review process should still be confirmed before merge.

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

[neuronull]

I believe we don't need to return a Result using try_sign here, since we are storing the private keys in memory.

ref: https://docs.rs/signature/2.2.0/signature/trait.Signer.html#tymethod.try_sign

Suggested change

	match self {
	Self::Ed25519(kp) => kp.try_sign(data),
	Self::Rsa(kp) => kp.try_sign(data),
	}
	.map_err(|e| anyhow!("Signing failed: {}", e))
	}
	match self {
	Self::Ed25519(kp) => kp.sign(data),
	Self::Rsa(kp) => kp.sign(data),
	}
	}

[coltonhurst]

Good call, and as discussed internally, thanks for explaining the why there!

After looking at their docs and their implementation for sign vs try_sign, my current preference is to still use try_sign, because sign can panic. Even if we should never hit the case where it does panic (based on our usage), just because of the specific way they designed and implemented these (a unique case), I would still prefer to go the safer route.

You are more familiar with the usage of this in the whole context of ssh v2; if returning a Result causes problems for users of this function, then definitely let me know the implications! Worst case, while not my preference, another option is to still call try_sign but return a Signature on success, and on failure, add our own panic, so it's very visible what's happening. Then we can document it with a comment or something. (I don't see any mention of the fact sign can panic in their docs, which is not ideal.)

[neuronull]

To add clarity- the reason I was able to surface this at all is because I was looking at the diff and thinking about the error testing (specifically, how we would test the error path).

That led me to reading their docs, and then to this notion: if we have already created the key means the key is cryptographically valid, so the signing of data with it can't fail.

Their docs state "The main intended use case for signing errors is when communicating with external signers, e.g. cloud KMS, HSMs, or other hardware tokens."

and since we are storing the private keys in memory, it would appear we are safe not to use try_sign due to our use case.

My goal is just to make sure our code simple and correct, and that the testing we do is necessary. If we are having to create tests to inject failure cases that we'll never encounter, or code paths that can't be hit, then that's wasted effort.

@quexten , maybe your expertise can help us make sense of the RustCrypto trait!

---

In the event that we do keep using try_sign() , I would then ask these questions: what failures do we expect to occur here, and what will our upstream consumers of the sign() function need to take? The answers to these questions, to me, would guide our choices on what to do with the signature::Error

[coltonhurst]

My goal is just to make sure our code simple and correct, and that the testing we do is necessary. If we are having to create tests to inject failure cases that we'll never encounter, or code paths that can't be hit, then that's wasted effort.

I definitely agree. After our internal thread, I am confident we are fine on the crypto side of things. (Happy to hear @quexten's thoughts there though.)

My concern I'm trying to highlight in my response above is just from an API standpoint. If there is any possible error / panic / etc. that can return from a function we are using, I want to make sure we have a plan for it. Even if it should never happen, if it's still possible according to a lib's API or implementation, I would prefer to lean into the strength we get from Rust's design.

[neuronull]

If there is any possible error / panic / etc. that can return from a function we are using
Even if it should never happen

these statements are talking about using sign() , is that correct? (because the try_sign() returns Error, so it is expected to be able to occur, and my earlier comments about sign() are discussing the cases where we're not expecting it to not occur). If so,

I am confident we are fine on the crypto side of things

The above statement suggests you are content with using sign(), which is different from your prior preference:

my current preference is to still use try_sign

and in that case I can't tell what you're advocating for :( Please let me know if I'm misinterpreting something.

With

I would prefer to lean into the strength we get from Rust's design.

Could you expand on that? It's not clear what you're referring to.

If you're referring to

Worst case, while not my preference, another option is to still call try_sign but return a Signature on success, and on failure, add our own panic, so it's very visible what's happening

, I'm not following how that's an improvement over just using sign() ? I would think both panics are going to be equally visible?

---

To me the options are:

1. use sign() , much like we use mutex.lock().expect("mutex not to be poisoned") , and we can add a comment if that'd be helpful, to state why we are using sign and not try_sign.
2. use try_sign() and either:
   a. wrap in anyhow! as you have here
   b. return signature::Error directly
   c. use thiserror

As I indicated in my previous messages, my understanding is option 1 is most correct for us. But I'd like to get @quexten's thoughts for confidence, given the design that RustCrypto took for this trait.

And if we end up going with option 2 , my questions then become

what failures do we expect to occur here, and what will our upstream consumers of the sign() function need to take?

, and it may be that 2.a ends up being our approach, if there are valid error paths that can occur, and if we are only ever taking one action based on any error path that occurs. Would be great to get @quexten 's expertise to help us understand that.

[coltonhurst]

Sorry my message isn't clear - my main goal is to avoid using a function that can panic. Even if, in theory, we shouldn't ever hit that case, I would rather use try_sign() (option 2) and handle it on our side.

[neuronull]

That's alright, thank you for clarifying.

I'm hoping to gather more data (by means of @quexten's insight) before a decision is made, but

My take is essentially that based on the information present, it suggests we wouldn't in practice be able to encounter it (again hoping @quexten can help clarify this) , and if it's the case, then if we use try_sign() just because the sign() function technically could panic, but only theoretically panics (by means of the use cases we don't have) because of unfortunate design choices by the rustcrypto crate, then by doing so we are just inheriting their unfortunate design choices, by having to have logic to handle errors (and provide injections for in testing, and explanatory code comments) that we can't hit in practice. But, that's just my opinion on how to approach that situation. Let's see if that situation is our reality.

And that if we do actually have legitimate cases where this can Error for our usage, then I'm on board for using try_sign (would then pose my aforementioned inquiries).

FWIW, I tracked down where that was introduced, and here is the commit message (no PR) , and the content supports my theory. The initial implementation of the trait only had a function that returned the Error but rather than refactor the underlying in order to have distinct code paths, they simply provided a function that did what all their callsites were probably doing ( e.g. sign().expect("the signing not to fail because the key is stored in memory)

For many signature providers, it's not possible for an error to occur
when creating a signature. Support for handling errors which occur when
computing signatures is intended for use with cloud KMS, HSMs, or other
hardware tokens where things like I/O errors are possible.

To simplify usage in the event that only software signers are in use,
this commit splits the sign API into Result-based try_sign method
and another sign method that always assumes success and unwraps the
result of the former method.

[quexten]

I concur with @neuronull. I think we should use sign and not try_sign. If we can explicitly state why we trust a function to not panic, we don't need to deal with error cases that will not realistically occur, and doing so is standard practice in other parts of the codebase.

Here, we are working with the SSH key types from the ssh-key crate, which generally already validate that they are correct during creation / TryFrom. Signatures should never fail.

Just because the code path contains a panic condition does not mean it can actually happen, and in this case it cannot happen. The docs specifically state this is for cases where the signing operation can fail for cases like HSMs, where the key may not be available / the signing operation may not respond.

[coltonhurst]

@quexten thanks for your review here! This seems to come down to what we are comfortable with in regards to panic-able code. This is especially relevant in protection against future edits where consumers of this code may not be aware of the limitations. My current personal preference is to avoid it if we can, by making it extremely clear through Rust's powerful enum types (Result in this case).

While I don't think the cons of doing it this way outweigh the pros, if people are extremely against this approach, then we can leave the panic-able code in, but we need to heavily document all appropriate locations so users of this code are very aware of what the limitations are.

[coltonhurst]

Approach updated with comments in 22629d5

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

[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