[PM-20493] Add key wrapping / wrapped key facade for encstring & expose keywrap to purecrypto #221

quexten · 2025-04-21T16:30:04Z

🎟️ Tracking

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

📔 Objective

Refactors how wrapped keys are represented in the SDK. We now add a facade around them, that only exposes the unwrap method. Additionally, this exposes key-wrapping to wasm via PureCrypto, in order to act as a backwards compatibility layer for all clients code that has not been migrated to use the SDK entirely yet.

Note: This adds a new module - "safe" to the bitwarden-crypto crate. Any of the methods or structs exposed here should be used first and foremost before anything else within the crypto crate.

🦖 System evolution - "Safe module"

The safe module is meant to serve as a repository for developers to draw from first and foremost. We may want to re-consider moving all other unsafer methods (RSA encrypt, direct encrypt of data, pin-key, masterkey) into a module - hazmat - later on and moving safe to the top level, once the safe crate contains sufficient good abstractions.

The abstractions contained here should be simple to understand, easy to reason about, easy to use in combination and very hard to mis-use.

For instance, master-password unlock, and pin unlock could be represented as:

// envelope contains the KDF settings in the encstring serialization
let envelope: PasswordProtectedKeyEnvelope = PasswordProtectedKeyEnvelope::seal(&password, &user_key, &kdf_settings);
let user_key = envelope.unseal(&password);

which completely eliminates the need for developers to care about synchronizing kdf settings and salts, or even caring about things like a masterkey existing.

⏰ 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 · 2025-04-21T16:34:58Z

Checkmarx One – Scan Summary & Details – 342d90e3-c8c5-4747-aabd-73cb0a1a7d8c

Great job, no security vulnerabilities found in this Pull Request

codecov · 2025-04-21T16:39:02Z

Codecov Report

Attention: Patch coverage is 79.31034% with 66 lines in your changes missing coverage. Please review.

Project coverage is 66.86%. Comparing base (fe76be2) to head (db41105).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/bitwarden-wasm-internal/src/pure_crypto.rs	0.00%	18 Missing ⚠️
...ates/bitwarden-crypto/src/enc_string/asymmetric.rs	86.36%	9 Missing ⚠️
crates/bitwarden-crypto/src/store/context.rs	75.86%	7 Missing ⚠️
crates/bitwarden-uniffi/src/auth/mod.rs	0.00%	6 Missing ⚠️
crates/bitwarden-core/src/auth/login/api_key.rs	0.00%	5 Missing ⚠️
crates/bitwarden-core/src/auth/login/password.rs	0.00%	5 Missing ⚠️
crates/bitwarden-core/src/mobile/crypto.rs	72.22%	5 Missing ⚠️
crates/bitwarden-vault/src/cipher/cipher.rs	90.00%	4 Missing ⚠️
crates/bitwarden-core/src/auth/auth_client.rs	0.00%	3 Missing ⚠️
crates/bitwarden-core/src/auth/tde.rs	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
- Coverage   67.05%   66.86%   -0.19%     
==========================================
  Files         212      213       +1     
  Lines       15989    15993       +4     
==========================================
- Hits        10721    10694      -27     
- Misses       5268     5299      +31

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

…to km/wrapped-key

quexten · 2025-04-23T17:02:16Z

crates/bitwarden-vault/src/cipher/cipher.rs

@@ -456,7 +460,8 @@ impl CipherView {
        ctx: &mut KeyStoreContext<KeyIds>,
        key: SymmetricKeyId,
    ) -> Result<(), CryptoError> {
-        let old_ciphers_key = Cipher::decrypt_cipher_key(ctx, key, &self.key)?;
+        let old_ciphers_key =
+            Cipher::decrypt_cipher_key(ctx, key, &self.key.clone().map(|k| k.into()))?;


Is there a representation that allows us to do this better, especially without the clone? (Could use some advice from someone with more Rust experience here)

In this specific case, because we're setting the key immediately after and we take the cipher view by mutable reference, you can change clone() by take(), this will take the option by value leaving a None behind, which avoids the clone:

Suggested change

Cipher::decrypt_cipher_key(ctx, key, &self.key.clone().map(|k| k.into()))?;

Cipher::decrypt_cipher_key(ctx, key, &self.key.take().map(Into::into))?;

This won't work for the general case where we don't want to mutate the Option though.

If this is the pattern we want to go with, I feel like what we want to do is change the key field in Cipher from an EncString to a WrappedSymmetricKey, which would save us from doing the conversion in the first place.

If this is the pattern we want to go with, I feel like what we want to do is change the key field in Cipher from an EncString to a WrappedSymmetricKey, which would save us from doing the conversion in the first place.

Do we need to consider the uniffi bindings / wasm bindings here? I see they are exposed, so would this be a breaking change?

I think for Uniffi and WASM EncString is basically a type alias to string, so assuming we do the same thing with WrappedSymmetricKey (implement Serialize/Deserialize and do the uniffi type conversion), it should basically work.

dani-garcia · 2025-04-23T18:44:17Z

crates/bitwarden-crypto/src/safe/key_wrap.rs

+impl AsRef<EncString> for WrappedSymmetricKey {
+    fn as_ref(&self) -> &EncString {
+        &self.0
+    }
+}
+
+impl From<WrappedSymmetricKey> for EncString {
+    fn from(wrapped: WrappedSymmetricKey) -> Self {
+        wrapped.0
+    }
+}


I feel like having these two conversions kind of defeats the point of the facade, as they allow you to extract the inner EncString accidentally quite easily.

It might be better to have some explicit functions for access to the inner value (maybe even crate-internal if possible):

pub fn into_inner(self) -> EncString { self.0 } pub fn as_inner(&self) -> &EncString { &self.0 }

Agreed. I mostly want to make it so that the user has to explicitly think about and choose to convert them, I don't think banning conversion outside of the crate is possible (yet?).

dani-garcia · 2025-04-23T18:48:55Z

crates/bitwarden-crypto/src/store/context.rs

@@ -167,9 +164,10 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
        &self,


We should probably consider renaming the function wrap_symmetric_key or something like that as well

sonarqubecloud · 2025-04-25T10:41:00Z

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

quexten added 2 commits April 18, 2025 20:16

Add WrappedSymmetricCryptoKey

1b5d6d4

Cleanup

3f84a20

quexten added 3 commits April 22, 2025 17:09

Cargo fmt

3542d51

Migrate more code to use keywrap

df634d7

Expose to purecrypto and fix build issues

f2998a8

quexten changed the title ~~Add key wrapping / wrapped key facade for encstring~~ [PM-20493] Add key wrapping / wrapped key facade for encstring & expose keywrap to purecrypto Apr 22, 2025

quexten and others added 13 commits April 22, 2025 18:35

Cleanup

16dc1e4

Cargo fmt

e53ef35

Fix clippy error

f416613

Fix docs

69e8ce2

Cleanup

aa064f1

Add comment

d148b6a

Fix build

5b4b5a3

Merge branch 'main' into km/wrapped-key

475ab53

Merge branch 'main' into km/wrapped-key

0c39894

Cargo fmt

b04717e

Merge branch 'km/wrapped-key' of github.com:bitwarden/sdk-internal in…

7dbd209

…to km/wrapped-key

Fix clippy warn

df7b62e

Cargo fmt

2661f93

quexten commented Apr 23, 2025

View reviewed changes

quexten marked this pull request as ready for review April 23, 2025 17:20

quexten requested a review from a team as a code owner April 23, 2025 17:20

quexten requested a review from addisonbeck April 23, 2025 17:20

dani-garcia reviewed Apr 23, 2025

View reviewed changes

quexten added 4 commits April 25, 2025 11:50

Cleanup naming

623587f

Make inner conversion explicit

bdbd6d3

Cleanup

f8a2053

Cargo fmt

7c3511d

quexten added 2 commits April 25, 2025 12:18

Fix docs

d91477d

Fix docs

db41105

quexten requested a review from dani-garcia April 25, 2025 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-20493] Add key wrapping / wrapped key facade for encstring & expose keywrap to purecrypto #221

[PM-20493] Add key wrapping / wrapped key facade for encstring & expose keywrap to purecrypto #221

quexten commented Apr 21, 2025 •

edited

Loading

github-actions bot commented Apr 21, 2025 •

edited

Loading

codecov bot commented Apr 21, 2025 •

edited

Loading

quexten Apr 23, 2025

dani-garcia Apr 23, 2025

quexten Apr 25, 2025

dani-garcia Apr 25, 2025

dani-garcia Apr 23, 2025 •

edited

Loading

quexten Apr 25, 2025

dani-garcia Apr 23, 2025

sonarqubecloud bot commented Apr 25, 2025

	Cipher::decrypt_cipher_key(ctx, key, &self.key.clone().map(\|k\| k.into()))?;
	Cipher::decrypt_cipher_key(ctx, key, &self.key.take().map(Into::into))?;

		@@ -167,9 +164,10 @@ impl<Ids: KeyIds> KeyStoreContext<'_, Ids> {
		&self,

[PM-20493] Add key wrapping / wrapped key facade for encstring & expose keywrap to purecrypto #221

Are you sure you want to change the base?

[PM-20493] Add key wrapping / wrapped key facade for encstring & expose keywrap to purecrypto #221

Conversation

quexten commented Apr 21, 2025 • edited Loading

🎟️ Tracking

📔 Objective

🦖 System evolution - "Safe module"

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Apr 21, 2025 • edited Loading

Great job, no security vulnerabilities found in this Pull Request

codecov bot commented Apr 21, 2025 • edited Loading

Codecov Report

quexten Apr 23, 2025

Choose a reason for hiding this comment

dani-garcia Apr 23, 2025

Choose a reason for hiding this comment

quexten Apr 25, 2025

Choose a reason for hiding this comment

dani-garcia Apr 25, 2025

Choose a reason for hiding this comment

dani-garcia Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

quexten Apr 25, 2025

Choose a reason for hiding this comment

dani-garcia Apr 23, 2025

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 25, 2025

Quality Gate passed

quexten commented Apr 21, 2025 •

edited

Loading

github-actions bot commented Apr 21, 2025 •

edited

Loading

codecov bot commented Apr 21, 2025 •

edited

Loading

dani-garcia Apr 23, 2025 •

edited

Loading