[PM-18938] Isolate decryption support for type 0 user keys

[eligrubb]

🎟️ Tracking

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

📔 Objective

Isolates Type 0 key decryption to the master_key.rs:decrypt_user_key() flow. All other uses now raise new bitwarden-crypto error UnsupportedOperationError::DecryptionNotImplementedForKey.

This is another small step in deprecating use of legacy Type 0 keys in the sdk, see tech breakdown. Type 0 keys have been deprecated from active use for a long time, this continues the process of removing their functionality while still supporting master key decryption for old accounts.

Type 0-based encryption was already deprecated in a prior PR, this change isolates decryption. Prior work isolated clients to use only the master_key.rs:decrypt_user_key path (in TS clients, via PureCrypto.decrypt_user_key_with_master_key #465). Other uses of type 0 keys are expected to now throw an error.

🚨 Breaking Changes

⏰ 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]

Checkmarx One – Scan Summary & Details – f375a543-492b-4a00-ab70-a336ae13da35

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

[github-actions]

🔍 SDK Breaking Change Detection Results

SDK Version: km/deprecate-type-0 (ae9ba48)
Completed: 2025-12-01 17:28:47 UTC
Total Time: 217s

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

---

Breaking change detection completed. View SDK workflow

[codecov]

Codecov Report

❌ Patch coverage is 87.96296% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.52%. Comparing base (2cca3d4) to head (ae9ba48).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/bitwarden-crypto/src/store/context.rs	88.05%	8 Missing ⚠️
crates/bitwarden-crypto/src/keys/master_key.rs	90.62%	3 Missing ⚠️
...rates/bitwarden-crypto/src/enc_string/symmetric.rs	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #585      +/-   ##
==========================================
+ Coverage   79.46%   79.52%   +0.05%     
==========================================
  Files         302      302              
  Lines       32331    32423      +92     
==========================================
+ Hits        25691    25783      +92     
  Misses       6640     6640              

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

[quexten]

Approving, but I have one optional comment / request. Other than that looks good; Nice work!

[claude]

Claude finished @eligrubb's task —— View job

---

Code Review Complete

Summary

This PR successfully isolates Type 0 key decryption support to the master_key.rs:decrypt_user_key() flow while preventing all other Type 0 decryption operations. The changes introduce a new DecryptionNotImplementedForKey error and update four strategic locations to block Type 0 operations. The implementation properly maintains backward compatibility for master key decryption while deprecating the legacy encryption type.

PR Description Quality

The PR description is well-structured and comprehensive:

• Clear objective linking to Jira task with tech breakdown reference
• Proper context about Type 0 deprecation history
• Explicit statement about what changes (isolation of decryption support)
• References to prior work ([PM-24377] Adds decrypt_user_key_with_master_key into PureCrypto #465)
• Good checklist coverage

Findings

Finding 1: Inconsistent error naming in master_key.rs:decrypt_user_key() (already identified by @quexten)

💭 In master_key.rs:156, when decrypting COSE encrypted user keys fails, the code uses EncryptionNotImplementedForKey error. Now that there's a separate DecryptionNotImplementedForKey error, this should be switched for consistency since this is a decryption operation.

Context

// Line 155-157
EncString::Cose_Encrypt0_B64 { .. } => {
    return Err(CryptoError::OperationNotSupported(
        crate::error::UnsupportedOperationError::DecryptionNotImplementedForKey, // ✓ Now uses correct error
    ));
}

Status: This has actually been addressed in the current PR (line 156 uses DecryptionNotImplementedForKey), so @quexten's comment is already resolved.

Finding 2: Test moved without tracking relationship to isolated code path

💭 The test test_decrypt_cbc256 was moved from symmetric.rs:583 to master_key.rs:333, but it's not immediately clear this still validates the isolated decryption path works correctly. Consider adding a comment explaining this test validates the one remaining Type 0 decryption path (master key decryption).

Suggested addition

#[test]
// This test validates the ONLY remaining Type 0 decryption path:
// master_key.rs:decrypt_user_key() for backward compatibility with legacy accounts
fn test_decrypt_cbc256() {

Finding 3: Missing test coverage for blocked COSE user key decryption

⚠️ The PR blocks COSE encrypted user keys in master_key.rs:decrypt_user_key():155-158 but there's no test validating this new error path. Given the security-sensitive nature of crypto code, this should have explicit test coverage.

Suggested test

#[test]
fn test_decrypt_user_key_fails_for_cose() {
    let master_key = MasterKey::generate(rand::thread_rng());
    
    // Create a COSE-encrypted EncString (dummy values are fine for this test)
    let cose_enc_str = EncString::Cose_Encrypt0_B64 {
        data: vec![0u8; 32],
    };
    
    let result = master_key.decrypt_user_key(cose_enc_str);
    assert!(
        matches!(
            result,
            Err(CryptoError::OperationNotSupported(
                crate::error::UnsupportedOperationError::DecryptionNotImplementedForKey
            ))
        ),
        "Expected COSE user key decryption to fail"
    );
}

Finding 4: Comprehensive test coverage for new restrictions

👍 Excellent test coverage added in context.rs and symmetric.rs:

• test_encrypt_decrypt_data_fails_when_key_is_type_0 validates both encrypt and decrypt blocking in KeyStoreContext
• test_wrap_unwrap_key_fails_when_key_is_type_0 validates key wrapping/unwrapping is blocked
• test_decrypt_fails_for_cbc256_keys validates direct decryption is blocked
• Tests use proper error matching with helpful assertion messages

Finding 5: Direct AES call preserves exact behavior for master key path

👍 The isolated path in master_key.rs:146-148 now calls crate::aes::decrypt_aes256 directly instead of going through the trait system, which correctly bypasses the new restriction. This is the right approach for maintaining backward compatibility.

Security & Backward Compatibility Assessment

Backward Compatibility: ✓ Maintained

• Master key decryption path (the ONLY legitimate Type 0 use case) still works via direct AES call
• Test test_decrypt_user_key_aes_cbc256_b64 validates legacy user key decryption
• Existing encrypted vaults remain decryptable

Security: ✓ Improved

• Type 0 keys now blocked in 4 critical locations: symmetric.rs:335, context.rs:172, context.rs:520, and encryption was already blocked previously
• New DecryptionNotImplementedForKey error provides clear feedback
• No secrets exposed in error messages (proper error handling)

Crypto Safety: ✓ Appropriate

• Changes only affect key types/operations, not crypto algorithms themselves
• No modifications to core crypto primitives (bitwarden-crypto/src/aes.rs untouched)
• Per CLAUDE.md guidelines: crypto algorithm changes require justification — this PR only restricts which keys can decrypt, not how decryption works

Test Coverage Analysis

Patch Coverage: 88.78% (12 lines missing coverage per Codecov)

Missing coverage breakdown:

• context.rs: 8 lines (likely error path branches)
• symmetric.rs: 2 lines (error return paths)
• master_key.rs: 2 lines (error return paths)

⚠️ The missing coverage appears to be primarily in error handling paths. While the happy paths and expected error cases are well-tested, some edge cases in error propagation may lack coverage.

Good Practices Observed

• Systematic approach: changed all 4 locations where Type 0 decryption occurred
• Clear error semantics with new dedicated error type
• Extensive test updates to validate new behavior
• Direct AES call for isolated path avoids architectural complexity

Action Items

1. ⚠️ Add test coverage for COSE user key decryption failure in master_key.rs (see Finding 3)
2. 💭 Consider adding comment to test_decrypt_cbc256 explaining it validates the isolated path (see Finding 2)
3. ✓ No action needed for Finding 1 - @quexten's feedback is already addressed in current code

Status of Previous Review Comments

@quexten's review comment about switching to DecryptionNotImplementedForKey in master_key.rs:decrypt_user_key() — Already resolved in the current code (line 156 uses the correct error).

---

[Thomas-Avery]

Nice work! LGTM