Skip to content

Conversation

@quexten
Copy link
Contributor

@quexten quexten commented Oct 28, 2025

🎟️ Tracking

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

Fixes:
bitwarden/clients#17028
bitwarden/clients#14076

📔 Objective

Updates the bitwarden-ssh dependencies to rc releases from RustCrypto. This allows us to:

  • Drop our PKCS5 patch crate
  • Fix import of putty keys
  • Fix import of various other broken keys (linewidth issues and others)

It also replaces the RSA crate that has a vulnerability (timing attack) - though, it is unlikely that this actually affects import / was abusable.

RSA key generation test vectors are broken because the underlying RSA generation algorithm changed with rustcrypto's massive re-implementation of RSA, hence new test keys are provided.

Note: This pins the dependencies to be different from the workspace crates, as those are not yet ready to be updated. Eventually we will update those bit-by-bit too and then drop the dependency pinning.

⏰ 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
Copy link
Contributor

github-actions bot commented Oct 28, 2025

Logo
Checkmarx One – Scan Summary & Detailsa8f9fe4c-f837-4029-a76a-109d9b3664d7

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

@quexten quexten marked this pull request as ready for review October 29, 2025 18:41
@quexten quexten requested a review from a team as a code owner October 29, 2025 18:41
@quexten quexten requested a review from addisonbeck October 29, 2025 18:41
@quexten quexten changed the title [BEEEP] Update ssh dependencies and fix imports of various SSH keys [BEEEP] Update bitwarden-ssh RustCrypto dependencies, fixing imports of various SSH keys Oct 29, 2025
@claude
Copy link

claude bot commented Oct 29, 2025

Claude finished @quexten's task —— View job


Code Review Complete

This PR successfully updates bitwarden-ssh to RustCrypto RC releases, addressing key import issues and removing the timing-vulnerable RSA crate.


Summary of Changes

  • Updated 11 cryptographic dependencies to RustCrypto RC releases
  • Removed the pkcs5 patch dependency (no longer needed with RC releases)
  • Fixed PuTTY key import support (previously broken)
  • Added regression test for issue 17028 (linewidth/formatting issues)
  • Replaced RSA 0.9 with 0.10-rc.10 (addresses timing attack vulnerability)
  • Updated RSA test vectors due to algorithm changes in RustCrypto's RSA implementation
  • Removed workaround code for RSA-to-PKCS8 conversion (now natively supported)

Critical Issues

None identified.


Suggested Improvements

Finding 1: Missing documentation comment explaining temporary dependency pinning

The Cargo.toml pins RC versions that differ from workspace dependencies, but lacks a comment explaining this is temporary. This could lead to confusion during future dependency updates.

Suggested addition (crates/bitwarden-ssh/Cargo.toml:27)
[dependencies]
# NOTE: These dependencies are pinned to RustCrypto RC releases temporarily.
# They should be updated to match workspace versions once all crates are ready
# for the RustCrypto RC releases. See: PM-27518
bitwarden-error = { workspace = true }
bitwarden-vault = { workspace = true }

Finding 2: Generator test coverage missing line

Codecov reports 1 missing line in generator.rs:54. The untested line appears to be the error path in PrivateKey::new() when creating an RSA key.

Context
fn create_rsa_key<R: CryptoRng + ?Sized>(
    rng: &mut R,
    bits: usize,
) -> Result<ssh_key::PrivateKey, error::KeyGenerationError> {
    let rsa_keypair = ssh_key::private::RsaKeypair::random(rng, bits)
        .map_err(KeyGenerationError::KeyGeneration)?;
    let private_key =
        ssh_key::PrivateKey::new(ssh_key::private::KeypairData::from(rsa_keypair), "")
            .map_err(KeyGenerationError::KeyGeneration)?;  // Line 54 - error path not tested
    Ok(private_key)
}

While achieving 100% coverage for error paths in key generation may not be practical (would require inducing cryptographic failures), consider adding a comment documenting why this error path isn't covered.


Finding 3: Type parameter verbosity in PEM label matching

The explicit type parameters pkcs8::PrivateKeyInfo::<(), (), ()> and pkcs8::EncryptedPrivateKeyInfo::<()> work but seem verbose.

Context (crates/bitwarden-ssh/src/import.rs:25-27)
match label {
    pkcs8::PrivateKeyInfo::<(), (), ()>::PEM_LABEL => import_pkcs8_key(encoded_key, None),
    pkcs8::EncryptedPrivateKeyInfo::<()>::PEM_LABEL => import_pkcs8_key(

This appears to be required due to API changes in pkcs8 0.11-rc. If these are the only valid type parameters for these types, consider filing an upstream issue to simplify the API, or add a comment explaining why these specific parameters are needed.


Good Practices Observed

  • Comprehensive test coverage for key import scenarios (PuTTY, regression cases)
  • Security-focused: replaced vulnerable RSA crate
  • Clean removal of workaround code (RSA conversion, pkcs5 patch)
  • Tests updated to match new RSA generation output

Action Items

  1. ♻️ Add documentation comment explaining temporary RC version pinning (Finding 1)
  2. 💭 Consider documenting why generator.rs:54 error path isn't covered (Finding 2)
  3. 💭 Optional: Investigate if pkcs8 API could be simplified upstream (Finding 3)

PR Description Quality

The PR description is comprehensive and well-structured:

  • ✅ Clear objective explaining what changed and why
  • ✅ Proper Jira tracking reference
  • ✅ Context about security vulnerability
  • ✅ Explanation of breaking test vector changes
  • ✅ Note about temporary dependency pinning

@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 95.23810% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.47%. Comparing base (efed963) to head (3addd8d).

Files with missing lines Patch % Lines
crates/bitwarden-ssh/src/generator.rs 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #536      +/-   ##
==========================================
+ Coverage   79.46%   79.47%   +0.01%     
==========================================
  Files         302      302              
  Lines       32331    32335       +4     
==========================================
+ Hits        25691    25698       +7     
+ Misses       6640     6637       -3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

addisonbeck
addisonbeck previously approved these changes Oct 31, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 26, 2025

🔍 SDK Breaking Change Detection Results

SDK Version: km/beeep/ssh-import-fix (3addd8d)
Completed: 2025-11-26 12:31:00 UTC
Total Time: 215s

Client Status Details
typescript ✅ No breaking changes detected TypeScript compilation passed with new SDK version - View Details

Breaking change detection completed. View SDK workflow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants