[PM-24263] Pin protected key envelope unlock #372

quexten · 2025-07-31T17:07:17Z

🎟️ Tracking

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

📔 Objective

Adds enrollment functionality for Password(PIN)Protected user key envelope. Both crypto initialization via the init request, and a function exposing the raw key material are provided. The latter is required since unlock is not yet done via the init methods on WASM/clients.

⏰ 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-07-31T17:12:12Z

Checkmarx One – Scan Summary & Details – 02371255-09e1-4185-b886-5cdf8c1a6eb5

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

…pin-unlock

coroiu

Looks good to me, but I don't have a ton of experience with the key store. Let me know if there's anything you'd like me to take a deeper look at!

quexten · 2025-08-14T12:26:26Z

Looks like we had a deadlock on the keystore lock, caught by the CI / tests which should be fixed in the latest revision.

codecov · 2025-08-14T12:41:14Z

Codecov Report

❌ Patch coverage is 92.54658% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.56%. Comparing base (eb6bff0) to head (9c293f5).

Files with missing lines	Patch %	Lines
crates/bitwarden-uniffi/src/crypto.rs	0.00%	12 Missing ⚠️

Additional details and impacted files

@@                                Coverage Diff                                @@
##           km/beeep/safe-password-protected-key-envelope     #372      +/-   ##
=================================================================================
+ Coverage                                          74.36%   74.56%   +0.19%     
=================================================================================
  Files                                                255      255              
  Lines                                              22352    22513     +161     
=================================================================================
+ Hits                                               16622    16786     +164     
+ Misses                                              5730     5727       -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.

mzieniukbw · 2025-08-14T15:06:20Z

crates/bitwarden-core/src/key_management/crypto_client.rs

+    /// Decrypts a `PasswordProtectedKeyEnvelope`, returning the user key, if successful.
+    /// This is a stop-gap solution, until initialization of the SDK is used.
+    #[cfg(feature = "wasm")]
+    pub fn unseal_password_protected_key_envelope(


💡 Shall we add some unit test coverage ?

Added test for the enroll -> re-enroll with encrypted -> unseal flow.

Hinton · 2025-08-14T14:54:02Z

crates/bitwarden-core/src/client/encryption_settings.rs

+
+    #[error("Wrong Pin")]
+    WrongPin,


question: What error do we return if you pass in the wrong password? It would seem reasonable that we return a similar error for both.

I suspect we derive the master key and then attempt to decrypt the user key which would fail and you'd get a decrypt error? Maybe it would be more elegant to handle that with a better error message in which case both wrong password and pin could use the same error?

With master-key decryption you cannot know if:

Your email is different to the one used on creation

Your kdf settings are different to the one used on creation

Your password is different "wrong" to the one used on creation

(Your master-key-wrapped-user-key was tampered with)

and all of these would result in the same error. In most cases the true reason would be "wrong password" but there are reasonable cases such as the KDF settings being out of sync, that would lead to the same error path.

With the PasswordProtectedKeyEnvelope, it's only:

Your password is different "wrong"

(Your key envelope was tampered with)
so for me it feels more appropriate to use the "WrongPin" error.

(I'm OK changing both to return "WrongPassword", but then we should document that this could also mean that KDF/salt(email) can be wrong for masterkey encrypted items).

Hinton · 2025-08-14T15:01:35Z

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

Can we lift this into the previous PR or is there a good reason this is handled here?

GitHub hid what this references, but I've lifted anything that I found to be non-pin-generic to the previous PR, which should make the diff a lot cleaner.

Hinton · 2025-08-14T15:15:36Z

crates/bitwarden-core/src/client/internal.rs

+            use crate::key_management::SymmetricKeyId;
+            let ctx = &mut self.key_store.context_mut();
+            let decrypted_user_key_id = pin_protected_user_key_envelope
+                .unseal(SymmetricKeyId::Local("tmp_unlock_pin"), &pin, ctx)


@dani-garcia did we have any convention for local IDs? Or is it just the wild west.

I think the approach here is to push the other work dani proposed ( https://bitwarden.atlassian.net/browse/PM-18102), so that we don't have to deal with naming collisions..

Hinton · 2025-08-14T15:19:08Z

crates/bitwarden-crypto/src/error.rs

@@ -69,6 +69,9 @@ pub enum CryptoError {

    #[error("Encoding error, {0}")]
    EncodingError(#[from] EncodingError),
+
+    #[error("Password protected key envelope error, {0}")]
+    PasswordProtectedKeyEnvelopeError(#[from] PasswordProtectedKeyEnvelopeError),


nit: We try and avoid having Error suffixes in the variant names.

Suggested change

PasswordProtectedKeyEnvelopeError(#[from] PasswordProtectedKeyEnvelopeError),

PasswordProtectedKeyEnvelope(#[from] PasswordProtectedKeyEnvelopeError),

Is this required to live in CryptoError or could we lift it up. I'm a bit concerned about the size of CryptoError since it's intended as a low level error.

Would it be more acceptable to be a variant of CryptoClientError? In that case we can lift it up to there.

(Lifted for now)

Hinton · 2025-08-14T15:19:48Z

crates/bitwarden-crypto/examples/protect_key_with_password.rs

Lift to previous PR?

Hinton · 2025-08-14T15:21:14Z

crates/bitwarden-core/src/key_management/non_generic_wrappers.rs

question: What happens if we want to sent another generic of PasswordProtectedKeyEnvelope? We've reserved the name of the generic now.

I think in this specific case it's OK. The reason is that the generic parameter is KeyIds, of which only one instance is ever supposed to exist as far as I'm aware - the one defined in core, describing all Bitwarden keys in the key hierarchy.

I would have preferred to not even make the key envelope generic in the first place, but had to do so in order to have the public interface take a KeyStoreContext and key ids instead of literal keys.

For cases where we have actually varying generic parameters, we should make the names of the non-generic wrappers such that they don't collide. I.e "CoseKeyBytes". "LegacyBitwardenKeyBytes", etc.

Note: I've lifted this into the previous PR too, because it's not specific to pin unlock.

matt-livefront · 2025-08-14T20:33:58Z

crates/bitwarden-core/src/key_management/crypto.rs

+    user_key_encrypted_pin: EncString,
+}
+
+pub(super) fn enroll_pin(


❓ Does this need to be exposed to uniffi? I'm not seeing in the Swift bindings.

Thanks for catching this! It seems we still separate out clients which is confusing, but it should be fixed in the latest revision of the PR, and I have verified this gets emitted to the swift bindings locally.

…pin-unlock

…o km/pin-unlock

sonarqubecloud · 2025-08-15T13:15: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

quexten added 4 commits July 31, 2025 18:48

Add password protected key envelope initialization

fc6c32b

Cleanup

162a083

Fix clippy

2e6e9e5

Add tests and fix builds

1986d62

quexten added 2 commits July 31, 2025 19:14

Cleanup and increase test coverage

41ba6c3

Fix pin envelope initialization

8e5fa67

quexten changed the title ~~Km/pin unlock~~ [PM-24263] Pin protected key envelope unlock Jul 31, 2025

quexten and others added 5 commits August 13, 2025 13:32

Merge branch 'km/beeep/safe-password-protected-key-envelope' into km/…

02f438c

…pin-unlock

Move clone

8b447b3

Only expose stop-gap key envelope api on wasm

9cad777

Move imports to internal feature

ca1a346

Move imports to wasm flagged section

2efaf4c

quexten marked this pull request as ready for review August 13, 2025 11:47

quexten requested review from a team as code owners August 13, 2025 11:47

quexten requested review from dani-garcia, mzieniukbw and coroiu and removed request for a team and dani-garcia August 13, 2025 11:47

coroiu approved these changes Aug 13, 2025

View reviewed changes

quexten added 2 commits August 14, 2025 14:17

Fix deadlock in PinEnvelope sdk init

4a99de2

Cargo fmt

1357d4b

mzieniukbw approved these changes Aug 14, 2025

View reviewed changes

mzieniukbw reviewed Aug 14, 2025

View reviewed changes

Hinton reviewed Aug 14, 2025

View reviewed changes

matt-livefront reviewed Aug 14, 2025

View reviewed changes

quexten added 10 commits August 15, 2025 12:06

Add enroll_pin_with_encrypted_pin

fbdced4

Apply feedback

50d2164

Merge branch 'km/beeep/safe-password-protected-key-envelope' into km/…

cd53925

…pin-unlock

Merge branch 'km/pin-unlock' of github.com:bitwarden/sdk-internal int…

f3c5004

…o km/pin-unlock

Fix build

4291314

Undo uniffi changes

660aaf1

Attempt to fix build

ecab3dc

Properly expose uniffi calls for PinEnvelope

9e79c97

Add test coverage for pin envelope

b127343

Lift PasswordProtectedKeyEnvelopeError to CryptoClientError

9c293f5

	PasswordProtectedKeyEnvelopeError(#[from] PasswordProtectedKeyEnvelopeError),
	PasswordProtectedKeyEnvelope(#[from] PasswordProtectedKeyEnvelopeError),

[PM-24263] Pin protected key envelope unlock #372

Are you sure you want to change the base?

[PM-24263] Pin protected key envelope unlock #372

Uh oh!

Conversation

quexten commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

coroiu left a comment

Choose a reason for hiding this comment

Uh oh!

quexten commented Aug 14, 2025

Uh oh!

codecov bot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quexten Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Aug 15, 2025

Quality Gate passed

Uh oh!

Uh oh!

quexten commented Jul 31, 2025 •

edited

Loading

github-actions bot commented Jul 31, 2025 •

edited

Loading

codecov bot commented Aug 14, 2025 •

edited

Loading

quexten Aug 15, 2025 •

edited

Loading

quexten Aug 15, 2025 •

edited

Loading

quexten Aug 15, 2025 •

edited

Loading