[PM-10405] Add bitwarden-ssh crate with keygeneration support #35

quexten · 2024-11-21T11:52:04Z

🎟️ Tracking

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

📔 Objective

Adds a new bitwarden-ssh crate to the sdk. This should later include both import-parsing and key generation.
This PR also exposes this for the web clients (not mobile yet).

⏰ 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 · 2024-11-21T11:54:36Z

Checkmarx One – Scan Summary & Details – 5d2ad67b-0b5d-4bfe-8494-fe5db2c305b9

No New Or Fixed Issues Found

…ernal into km/pm-10942/ssh-keygen

crates/bitwarden-ssh/src/lib.rs

crates/bitwarden-wasm-internal/src/ssh.rs

codecov · 2024-11-21T12:42:56Z

Codecov Report

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

Project coverage is 63.14%. Comparing base (3cdf320) to head (d8fe809).

Files with missing lines	Patch %	Lines
crates/bitwarden-ssh/src/lib.rs	0.00%	34 Missing ⚠️
crates/bitwarden-wasm-internal/src/ssh.rs	0.00%	12 Missing ⚠️
crates/bitwarden-ssh/src/error.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   63.39%   63.14%   -0.26%     
==========================================
  Files         184      187       +3     
  Lines       12935    12982      +47     
==========================================
- Hits         8200     8197       -3     
- Misses       4735     4785      +50

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

Hinton

Great start, let's get this function unit tested if possible, and remove some of the dependencies. I also started a discussion in slack about the divergence from our standard rand generator.

crates/bitwarden-ssh/Cargo.toml

Hinton · 2024-11-22T08:53:50Z

crates/bitwarden-ssh/Cargo.toml

+uniffi = { workspace = true, optional = true }
+uuid = { workspace = true }
+wasm-bindgen = { workspace = true, optional = true }
+tsify-next = { workspace = true, optional = true }


Unused? Only needed for full errors.

Hinton · 2024-11-22T08:57:25Z

crates/bitwarden-ssh/src/lib.rs

+pub fn generate_keypair(
+    key_algorithm: KeyAlgorithm,
+) -> Result<models::SshKey, error::KeyGenerationError> {
+    // sourced from cryptographically secure entropy source, with sources for all targets: https://docs.rs/getrandom
+    // if it cannot be securely sourced, this will panic instead of leading to a weak key
+    let mut rng: ChaCha8Rng = ChaCha8Rng::from_entropy();


issue: Please create a new private function which takes mut rng: impl rand::RngCore, as input. This way we can unit test it using chacha iniitalized from a seed. There are some examples in bitwarden-crypto on how to do this.

Hinton · 2024-11-22T09:01:54Z

crates/bitwarden-ssh/src/models.rs

+pub struct SshKey {
+    pub private_key: String,
+    pub public_key: String,
+    pub key_fingerprint: String,
+}


suggestion: I think the naming here can be confusing. We already have a concept of SshKey, and they are not the same type. We can either re-use SshKeyView, or change this to be a DTO, we generally call these the same as the function and append result, i.e. GenerateKeypairResult.

With a DTO we can also lift this into the lib.rs to keep it closer to the function producing it.

Hinton · 2024-11-22T09:03:06Z

crates/bitwarden-ssh/src/lib.rs

+    Rsa4096,
+}
+
+pub fn generate_keypair(


suggestion: keypair generally means an RSA keypair. In this case we generate an ssh keypair and we may benefit from including ssh in the name, to avoid confusion on the calling side where we can depend on bitwarden-crypto and bitwarden-ssh.

Hinton · 2024-11-22T09:06:39Z

crates/bitwarden-ssh/src/lib.rs

+
+            let private_key = ssh_key::PrivateKey::new(
+                ssh_key::private::KeypairData::from(rsa_keypair),
+                "".to_string(),


https://docs.rs/ssh-key/0.6.7/ssh_key/private/struct.PrivateKey.html#method.new accepts Into<String> which should work with string slices, i.e "".

Hinton · 2024-11-22T09:07:51Z

crates/bitwarden-ssh/src/lib.rs

+            };
+
+            let rsa_keypair = ssh_key::private::RsaKeypair::random(&mut rng, bits)
+                .map_err(|e| error::KeyGenerationError::KeyGenerationError(e.to_string()))?;


suggestion: Please import the used error rather than always using the full path.

Suggested change

.map_err(|e| error::KeyGenerationError::KeyGenerationError(e.to_string()))?;

.map_err(|e| KeyGenerationError::KeyGenerationError(e.to_string()))?;

Hinton · 2024-11-22T09:12:43Z

crates/bitwarden-wasm-internal/src/ssh.rs

+#[wasm_bindgen]
+pub enum KeyAlgorithm {
+    Ed25519,
+    Rsa3072,
+    Rsa4096,
+}
+
+// impl conversion
+impl From<KeyAlgorithm> for bitwarden_ssh::KeyAlgorithm {
+    fn from(key_algorithm: KeyAlgorithm) -> Self {
+        match key_algorithm {
+            KeyAlgorithm::Ed25519 => bitwarden_ssh::KeyAlgorithm::Ed25519,
+            KeyAlgorithm::Rsa3072 => bitwarden_ssh::KeyAlgorithm::Rsa3072,
+            KeyAlgorithm::Rsa4096 => bitwarden_ssh::KeyAlgorithm::Rsa4096,
+        }
+    }
+}
+
+#[wasm_bindgen]
+pub struct SshKey {
+    private_key: String,
+    public_key: String,
+    key_fingerprint: String,
+}
+
+impl From<bitwarden_ssh::models::SshKey> for SshKey {
+    fn from(key: bitwarden_ssh::models::SshKey) -> Self {
+        SshKey {
+            private_key: key.private_key,
+            public_key: key.public_key,
+            key_fingerprint: key.key_fingerprint,
+        }
+    }
+}
+
+impl SshKey {
+    pub fn private_key(&self) -> &str {
+        &self.private_key
+    }
+
+    pub fn public_key(&self) -> &str {
+        &self.public_key
+    }
+
+    pub fn key_fingerprint(&self) -> &str {
+        &self.key_fingerprint
+    }
+}


@coroiu Do we need to re-define the types? We should look into a way to conditionally apply wasm_bindgen in the original crates, similar to our uniffi flow.

You should be able to use #[cfg_attr(feature = "wasm", wasm_bindgen)] at the source but ideally bitwarden_ssh::models::SshKey would implement serialize and tsify instead. See https://contributing.bitwarden.com/architecture/sdk/password-manager/web/interoperability#how-to-use-tsify

Add bitwarden-ssh crate with keygeneration support

8c9c7b3

quexten requested review from Hinton and dani-garcia November 21, 2024 11:52

quexten and others added 3 commits November 21, 2024 03:55

Merge branch 'main' into km/pm-10942/ssh-keygen

6abd0cf

Fix formatting

bbf26ba

Merge branch 'km/pm-10942/ssh-keygen' of github.com:bitwarden/sdk-int…

c9172d8

…ernal into km/pm-10942/ssh-keygen

github-advanced-security bot found potential problems Nov 21, 2024

View reviewed changes

quexten removed request for Hinton and dani-garcia November 21, 2024 12:12

Fix clippy issues

9419971

quexten force-pushed the km/pm-10942/ssh-keygen branch from 47e2059 to 9419971 Compare November 21, 2024 12:31

Run cargo fmt

2749dc2

github-advanced-security bot found potential problems Nov 21, 2024

View reviewed changes

crates/bitwarden-wasm-internal/src/ssh.rs Fixed Show fixed Hide fixed

crates/bitwarden-wasm-internal/src/ssh.rs Fixed Show fixed Hide fixed

Fix further clippy errors

0c5a83d

quexten force-pushed the km/pm-10942/ssh-keygen branch from 8024d3a to 0c5a83d Compare November 21, 2024 12:43

quexten requested review from Hinton and dani-garcia November 21, 2024 12:53

Clean up cargo dependencies

ca92d44

quexten marked this pull request as ready for review November 21, 2024 17:55

Hinton reviewed Nov 22, 2024

View reviewed changes

quexten added 7 commits November 22, 2024 10:37

Remove unused cargo deps

0b9ee40

Switch to threadrng and clean up imports

b048502

Move to GenerateKeypairResult

1f08281

Implement tsify on sshkeygenresult

17f0d90

Cargo fmt

aeecd69

Clean up naming

f2e1acd

Fix formatting

d8fe809

quexten requested a review from Hinton November 22, 2024 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-10405] Add bitwarden-ssh crate with keygeneration support #35

[PM-10405] Add bitwarden-ssh crate with keygeneration support #35

quexten commented Nov 21, 2024

github-actions bot commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

Hinton left a comment

Hinton Nov 22, 2024

Hinton Nov 22, 2024

Hinton Nov 22, 2024

Hinton Nov 22, 2024

Hinton Nov 22, 2024

Hinton Nov 22, 2024

Hinton Nov 22, 2024

Hinton Nov 22, 2024

coroiu Nov 22, 2024 •

edited

Loading

	.map_err(\|e\| error::KeyGenerationError::KeyGenerationError(e.to_string()))?;
	.map_err(\|e\| KeyGenerationError::KeyGenerationError(e.to_string()))?;

[PM-10405] Add bitwarden-ssh crate with keygeneration support #35

Are you sure you want to change the base?

[PM-10405] Add bitwarden-ssh crate with keygeneration support #35

Conversation

quexten commented Nov 21, 2024

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Nov 21, 2024 • edited Loading

No New Or Fixed Issues Found

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

Hinton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coroiu Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

coroiu Nov 22, 2024 •

edited

Loading