[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

…ic keyencryptable trait impl

…uniffi

quexten · 2025-05-07T10:24:54Z

crates/bitwarden-crypto/src/uniffi_support.rs

@@ -43,3 +59,11 @@ impl UniffiCustomTypeConverter for UnsignedSharedKey {
        obj.to_string()
    }
 }
+
+// Uniffi doesn't emit unused types, this is a dummy record to ensure that the custom type
+// converters are emitted


Can be removed when #221 is merged.

sonarqubecloud · 2025-05-07T10:39:27Z

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

## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-21377 ## 📔 Objective Add remaining missing functions needed to port encrypt service to sdk. Ref: https://github.com/bitwarden/clients/blob/87a1f4e8ac928edfb05a283372550d0792c71827/libs/common/src/key-management/crypto/abstractions/encrypt.service.ts#L105-L188 Note: The way that this uses the key context is not ideal, but this should not be handled in this PR. This PR is to make the public interface available to unblock clients changes. Follow-up work (such as #221) can expose better apis from bitwarden-crypto and move PureCrypto to it. ## ⏰ 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

dani-garcia

Looks pretty good to me, just some minor details

dani-garcia · 2025-05-12T09:20:57Z

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

+/// key_to_wrap.
+///
+/// Wrapped keys such as cipher keys, or attachment keys, are used to create a layer of indirection,
+/// so that keys can be shared mor granularly, and so that data can be rotated more easily.


Suggested change

/// so that keys can be shared mor granularly, and so that data can be rotated more easily.

/// so that keys can be shared more granularly, and so that data can be rotated more easily.

dani-garcia · 2025-05-12T09:25:05Z

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

+impl std::fmt::Debug for WrappedSymmetricKey {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("WrappedSymmetricKey")
+            .field("inner_enc_string", &self.as_inner())
+            .finish()
+    }
+}


In this case we can probably just tag WrappedSymmetricKey with #[derive(Debug)] directly instead of implementing it by hand.

dani-garcia · 2025-05-12T09:31:01Z

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

    /// * `new_key_id` - The key id where the decrypted key will be stored. If it already exists, it
    ///   will be overwritten
-    /// * `encrypted_key` - The key to decrypt
+    /// * `key_to_unwrap` - The key to decrypt


There's still a reference to decryption instead of unwrapping (The key to decrypt -> The key to unwrap). To avoid the repetition with the comment, we could also rename the parameter to more closely resemble it's type too:

Suggested change

/// * `key_to_unwrap` - The key to decrypt

/// * `wrapped_key` - The key to unwrap

dani-garcia · 2025-05-12T09:33:01Z

crates/bitwarden-crypto/src/keys/master_key.rs

        make_user_key(rand::thread_rng(), self)
    }

    /// Encrypt the users user key
-    pub fn encrypt_user_key(&self, user_key: &SymmetricCryptoKey) -> Result<EncString> {
+    pub fn encrypt_user_key(&self, user_key: &SymmetricCryptoKey) -> Result<WrappedSymmetricKey> {


Question: Will we want to rename these to also use the wrap/unwrap naming?

dani-garcia · 2025-05-12T09:40:33Z

crates/bitwarden-core/src/auth/pin.rs

@@ -70,12 +70,12 @@ mod tests {

        let master_key = MasterKey::derive(password, email, &kdf).unwrap();

-        let user_key = "2.Q/2PhzcC7GdeiMHhWguYAQ==|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=";
+        let user_key: EncString = "2.Q/2PhzcC7GdeiMHhWguYAQ==|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap();


We implement FromStr for WrappedSymmetricKey as well, so if you remove the .into() below you can skip the type here and rust will infer it correctly as a WrappedSymmetricKey:

Suggested change

let user_key: EncString = "2.Q/2PhzcC7GdeiMHhWguYAQ==|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap();

let user_key = "2.Q/2PhzcC7GdeiMHhWguYAQ==|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap();

This also probably applies to the other test cases that were updated in this PR

dani-garcia · 2025-05-12T09:44:35Z

crates/bitwarden-uniffi/src/auth/mod.rs

@@ -110,10 +110,13 @@ impl AuthClient {
        password: String,
        encrypted_user_key: String,
    ) -> Result<String> {
+        let encrypted_user_key = encrypted_user_key
+            .parse::<EncString>()


We can also parse into WrappedSymmetricKey here and remove the .into()

Suggested change

.parse::<EncString>()

.parse::<WrappedSymmetricKey>()

dani-garcia · 2025-05-12T09:46:20Z

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

+        wrapped_key: String,
+        wrapping_key: Vec<u8>,
+    ) -> Result<Vec<u8>, CryptoError> {
+        let wrapped_key: WrappedSymmetricKey = EncString::from_str(&wrapped_key)?.into();


Suggested change

let wrapped_key: WrappedSymmetricKey = EncString::from_str(&wrapped_key)?.into();

let wrapped_key = WrappedSymmetricKey::from_str(&wrapped_key)?;

dani-garcia · 2025-05-12T09:47:49Z

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

@@ -453,7 +454,7 @@ 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.take())?;


Now that the keys are wrapped, the take here can be removed.

dani-garcia · 2025-05-12T09:54:18Z

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

-                    *attachment_key = dec_attachment_key.encrypt(ctx, new_key)?;
+                    let tmp_attachment_key_id = SymmetricKeyId::Local("attachment_key");
+                    ctx.unwrap_symmetric_key(old_key, tmp_attachment_key_id, attachment_key)?;
+                    *attachment_key = ctx.wrap_symmetric_key(new_key, tmp_attachment_key_id)?;


I'm wondering if we should offer a function in the context to do the unwrap/wrap for you, as we're already using it a couple of times here.

dani-garcia · 2025-05-12T10:09:31Z

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

@@ -489,8 +490,9 @@ impl CipherView {
        if let Some(attachments) = &mut self.attachments {
            for attachment in attachments {
                if let Some(attachment_key) = &mut attachment.key {
-                    let dec_attachment_key: Vec<u8> = attachment_key.decrypt(ctx, old_key)?;
-                    *attachment_key = dec_attachment_key.encrypt(ctx, new_key)?;
+                    let tmp_attachment_key_id = SymmetricKeyId::Local("attachment_key");


Usually we declare these ids as const, we should probably do the same for consistency.

quexten and others added 30 commits March 7, 2025 11:44

Add xchacha20poly1305 crypto primitives

72bcf39

Cargo fmt

0e68201

Cleanup

a58c53f

Cleanup

ba2d199

Remove poly1305 dependency

7647256

Move comment

6bab049

Adjust interface according to feedback

6b241ec

Remove comment

ced7213

Fix clippy warning

44e5ffd

Merge branch 'main' into km/pm-15096/xchacha20-poly1305

366d79f

tmp

77e0252

Cleanup

27fb7cf

Merge branch 'km/pm-15096/xchacha20-poly1305' into km/cose

94eed96

Fix build

eab453f

Merge branch 'main' into km/cose

941748a

Add keyids

43b117c

Remove blake3 dependency

4c7c724

Remove poly1305

5455f17

Remove hash parse error

c9882c6

tmp

cac04d3

Rename encrypt/decrypt to encapsulate/decapsulate and remove asymmetr…

df9327c

…ic keyencryptable trait impl

Merge branch 'km/poc-update-asymmetric-interface' into km/cose

d6ab6ca

Only allow symmetric keys for encapsulation

8df3dff

Remove unused imports

16c1a84

Cleanup

351394f

Cargo fmt

e221150

Add comments

ccf04c3

Fix tests

cce0a95

Cargo fmt

b075993

Rename key encapsulation function

20420e7

quexten added 3 commits April 28, 2025 16:04

Remove unneeded ref

a1a5a02

Fix comment

b21df8b

Rename SymmetricCryptoKey::generate_* to ::make_*

91b2465

quexten changed the base branch from main to km/cose April 29, 2025 11:45

quexten added 5 commits April 29, 2025 14:05

Merge branch 'km/cose' into km/wrapped-key

b22ad28

Use wrapped key in more places

7643a54

Wasm custom type for wrappedSymmetricKey

3b4e445

Cleanup

1b7dd69

Cargo fmt

64d2d50

Base automatically changed from km/cose to main May 6, 2025 15:13

quexten marked this pull request as draft May 6, 2025 17:50

quexten added 8 commits May 6, 2025 20:06

Merge branch 'main' into km/wrapped-key

c40208c

Cleanup

efcc194

Cargo fmt

3c20a6c

Fix formatting

f04fb2d

Cargo fmt

a2dd716

Add dummy record to force WrappedSymmetricCryptoKey to be emitted in …

28d1e6f

…uniffi

Cargo fmt

10904f3

Give Uniffi dummy record unique name

66c9ae6

quexten commented May 7, 2025

View reviewed changes

quexten added 3 commits May 7, 2025 12:32

Move keywrap out of context

6eb359d

Fix clippy warn

efa1310

Cleanup

34cf082

quexten marked this pull request as ready for review May 7, 2025 10:47

quexten removed the request for review from addisonbeck May 7, 2025 11:50

quexten marked this pull request as draft May 8, 2025 09:48

quexten mentioned this pull request May 8, 2025

[PM-21377] Add missing purecrypto functions #267

Merged

dani-garcia reviewed May 12, 2025

View reviewed changes

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

quexten May 7, 2025

sonarqubecloud bot commented May 7, 2025

dani-garcia left a comment

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

dani-garcia May 12, 2025

	/// so that keys can be shared mor granularly, and so that data can be rotated more easily.
	/// so that keys can be shared more granularly, and so that data can be rotated more easily.

	/// * `key_to_unwrap` - The key to decrypt
	/// * `wrapped_key` - The key to unwrap

	let user_key: EncString = "2.Q/2PhzcC7GdeiMHhWguYAQ==\|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=\|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap();
	let user_key = "2.Q/2PhzcC7GdeiMHhWguYAQ==\|GpqzVdr0go0ug5cZh1n+uixeBC3oC90CIe0hd/HWA/pTRDZ8ane4fmsEIcuc8eMKUt55Y2q/fbNzsYu41YTZzzsJUSeqVjT8/iTQtgnNdpo=\|dwI+uyvZ1h/iZ03VQ+/wrGEFYVewBUUl/syYgjsNMbE=".parse().unwrap();

	let wrapped_key: WrappedSymmetricKey = EncString::from_str(&wrapped_key)?.into();
	let wrapped_key = WrappedSymmetricKey::from_str(&wrapped_key)?;

[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

Choose a reason for hiding this comment

sonarqubecloud bot commented May 7, 2025

Quality Gate passed

dani-garcia 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quexten commented Apr 21, 2025 •

edited

Loading