Skip to content

[PM-39454] feat(km): reject unlock data whose user key id does not match the stored one - #8111

Draft
quexten wants to merge 1 commit into
km/user-key-id-05-write-pathfrom
km/user-key-id-06-validate-key-id
Draft

[PM-39454] feat(km): reject unlock data whose user key id does not match the stored one#8111
quexten wants to merge 1 commit into
km/user-key-id-05-write-pathfrom
km/user-key-id-06-validate-key-id

Conversation

@quexten

@quexten quexten commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

When changing the password or changing KDF settings, the user-key MUST NOT be changed. Thus, we add the key id in the unlock data so that the server can validate and assert it.

@quexten
quexten force-pushed the km/user-key-id-06-validate-key-id branch from 0e8999b to 39e02ae Compare July 31, 2026 13:33
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.90%. Comparing base (dc7519e) to head (1dc7f99).

Additional details and impacted files
@@                      Coverage Diff                      @@
##           km/user-key-id-05-write-path    #8111   +/-   ##
=============================================================
  Coverage                         62.90%   62.90%           
=============================================================
  Files                              2304     2304           
  Lines                            100400   100412   +12     
  Branches                           9041     9043    +2     
=============================================================
+ Hits                              63152    63165   +13     
+ Misses                            35062    35061    -1     
  Partials                           2186     2186           

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

@quexten
quexten force-pushed the km/user-key-id-06-validate-key-id branch from 39e02ae to f0f9329 Compare July 31, 2026 15:20
@quexten quexten added ai-review Request a Claude code review t:feature Change Type - Feature Development labels Jul 31, 2026
@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This change closes the remaining key-id gaps on the write path: ChangeKdfCommand now asserts the unlock data's key id is unchanged before delegating to MasterPasswordService (mirroring the existing cross-layer salt assertion, and documented as such), and BaseRotateUserAccountKeysData.ValidateContainedKeyIdMatches verifies that the key id carried in master password unlock data names the same key as the rotation's authoritative UserKeyId for both MasterPasswordRotateUserAccountKeysData and PasswordChangeAndRotateUserAccountKeysData. I traced both rotation entry points end to end: the comparison is request-internal, so it is independent of the BaseRotateUserAccountKeysAsync ordering that assigns user.UserKeyId; it runs before any persistence; TDE and Key Connector rotations carry no unlock key id and are correctly untouched; and the skip-when-contained-is-absent / reject-when-only-the-unlock-side-reports-one asymmetry is the safe direction, since the rotation's key id is the only value persisted. KeyId compares by value via Equals/==/!=, the [UserKeyId] attribute keeps malformed ids out before these checks, and the new unit tests plus the ordering-dependent integration test (unlock data keeps the fixture key id, rotation key id is overridden) fail loudly rather than silently if the setup order is ever changed.

Code Review Details

No new findings. Both maintainability threads from the earlier review pass are resolved, and the feature is unreleased (KeyId does not exist on main), so the stricter rejection introduces no client compatibility risk.

Comment thread src/Core/Auth/UserFeatures/UserMasterPassword/MasterPasswordService.cs Outdated
Comment thread src/Core/Auth/UserFeatures/UserMasterPassword/Data/UpdateExistingPasswordData.cs Outdated
@quexten
quexten marked this pull request as ready for review July 31, 2026 15:40
@quexten
quexten requested review from a team as code owners July 31, 2026 15:40
@quexten
quexten requested review from eligrubb and rr-bw July 31, 2026 15:40
@quexten
quexten marked this pull request as draft July 31, 2026 16:14
@quexten
quexten force-pushed the km/user-key-id-06-validate-key-id branch from 0c52d78 to 1dc7f99 Compare July 31, 2026 17:29
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 t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant