Skip to content

[PM-41071] Remove cipher key encryption flag - #1374

Open
shane-melton wants to merge 2 commits into
vault/pm-41073from
vault/pm-41071/remove-cipher-key-encryption-flag
Open

[PM-41071] Remove cipher key encryption flag#1374
shane-melton wants to merge 2 commits into
vault/pm-41073from
vault/pm-41071/remove-cipher-key-encryption-flag

Conversation

@shane-melton

@shane-melton shane-melton commented Aug 11, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

PM-41071

📔 Objective

Remove the internal cipher-key-encryption feature flag from the SDK and always use cipher key encryption when encrypting ciphers.

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: vault/pm-41071/remove-cipher-key-encryption-flag (58c7b98)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
typescript ❌ Breaking changes detected Compilation failed with new SDK version. A corresponding pull request addressing the breaking changes must be ready for merge in bitwarden/clients. - View Details
android ❌ Breaking changes detected Compilation failed with new SDK version. A corresponding pull request addressing the breaking changes must be ready for merge in bitwarden/android. - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.87640% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.20%. Comparing base (3b12971) to head (58c7b98).

Files with missing lines Patch % Lines
...den-vault/src/cipher/cipher_client/admin/create.rs 66.66% 1 Missing ⚠️
...bitwarden-vault/src/cipher/cipher_client/create.rs 66.66% 1 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           vault/pm-41073    #1374      +/-   ##
==================================================
+ Coverage           86.18%   86.20%   +0.01%     
==================================================
  Files                 500      500              
  Lines               73129    73105      -24     
==================================================
- Hits                63028    63021       -7     
+ Misses              10101    10084      -17     

☔ View full report in Codecov by Harness.
📢 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.

@shane-melton shane-melton added the ai-review Request a Claude code review label Aug 11, 2026
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the removal of the internal cipher-key-encryption flag and the consolidation of CipherView::generate_cipher_key + load_cipher_key_slot into a single load_cipher_key_slot(&mut self, ctx) that generates-and-stores on demand. Traced both encryption paths (encrypt_legacy_field_encryption and encrypt_blob_cipher_with_wrapping_key) and confirmed all fields, attachment keys and the emitted Cipher.key are consistently keyed off the same per-cipher key, wrapped under the explicitly supplied slot — so rotation's Local slot case still holds, and legacy key: None ciphers remain decryptable. Verified the flag removal is complete (no remaining references, Flags has no deny_unknown_fields so persisted state with the old key still deserializes), that CipherEditRequest.key carries existing cipher keys through edits, and that merge_with_cipher only pulls unencrypted metadata from the local cipher. The now-unconditional validate_attachment_keys() in encrypt_cipher_for_rotation tightens behavior for keyless (v1) attachments and matches the SDK's own check_for_old_attachments rotation guard.

Code Review Details

No findings at or above the reporting threshold.

Notes considered and intentionally not raised as findings:

  • admin/edit.rs re-encrypts original_cipher_view solely to supply merge fallbacks, which now also generates a throwaway cipher key — pre-existing behavior, and only unencrypted fields are read from that result.
  • Editing a legacy keyless cipher that has v1 (keyless) attachments now always adds a cipher key; v1 attachment contents still decrypt under the user/org key via AttachmentFile::key_identifier(), and this matched the prior flag-enabled behavior.

@shane-melton
shane-melton marked this pull request as ready for review August 11, 2026 15:47
@shane-melton
shane-melton requested review from a team as code owners August 11, 2026 15:47

@gbubemismith gbubemismith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good

@shane-melton
shane-melton requested a review from nikwithak August 11, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review breaking-change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants