Skip to content

[PM-33435] Add new user key rotation endpoint with MP support#7216

Open
Thomas-Avery wants to merge 8 commits intokm/pm-33162from
km/pm-33435/new-endpoint
Open

[PM-33435] Add new user key rotation endpoint with MP support#7216
Thomas-Avery wants to merge 8 commits intokm/pm-33162from
km/pm-33435/new-endpoint

Conversation

@Thomas-Avery
Copy link
Contributor

@Thomas-Avery Thomas-Avery commented Mar 13, 2026

🎟️ Tracking

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

📔 Objective

Adds a new endpoint for user key rotation without a password change. This implements the rotation for master password users and setups things for future implementation of key connector and TDE users.

This builds off the refactor done in #7201.

@Thomas-Avery Thomas-Avery self-assigned this Mar 13, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2026

Logo
Checkmarx One – Scan Summary & Detailsc57d727b-a77b-422c-a5c5-ba79cf93a3a3


New Issues (2) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 HIGH Cxb9102459-6a3a Nuget-AutoMapper-12.0.1
detailsRecommended version: 15.1.1
Description: AutoMapper is vulnerable to a Denial-of-Service (DoS) attack. Versions prior to 15.1.1 and 16.x prior to 16.1.1, when mapping deeply nested object ...
Attack Vector: NETWORK
Attack Complexity: LOW
Vulnerable Package
2 MEDIUM CSRF /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 154
detailsMethod at line 154 of /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs gets a parameter from a user request from request. T...
Attack Vector

@Thomas-Avery Thomas-Avery changed the title Add new user key rotation endpoint with MP support [PM-33435] Add new user key rotation endpoint with MP support Mar 16, 2026
@codecov
Copy link

codecov bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 94.89796% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.59%. Comparing base (b0e5467) to head (bc6bfe5).

Files with missing lines Patch % Lines
...gement/Models/Requests/UnlockMethodRequestModel.cs 86.95% 2 Missing and 1 partial ⚠️
...ent/Controllers/AccountsKeyManagementController.cs 94.28% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           km/pm-33162    #7216      +/-   ##
===============================================
+ Coverage        57.55%   57.59%   +0.04%     
===============================================
  Files             2032     2037       +5     
  Lines            89504    89602      +98     
  Branches          7954     7963       +9     
===============================================
+ Hits             51510    51604      +94     
- Misses           36149    36151       +2     
- Partials          1845     1847       +2     

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

@Thomas-Avery Thomas-Avery marked this pull request as ready for review March 16, 2026 19:13
@Thomas-Avery Thomas-Avery requested a review from a team as a code owner March 16, 2026 19:14
@Thomas-Avery Thomas-Avery requested review from eligrubb and removed request for a team March 16, 2026 19:14

namespace Bit.Api.KeyManagement.Models.Requests;

public class WrappedAccountCryptographicStateRequestModel
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ We already have AccountKeysRequestModel. Might be better to combine this with https://bitwarden.atlassian.net/browse/PM-22384 ?
If this is outside of scope, could we comment on the AccountKeysRequestModel that it's deprecated and will be superseded by WrappedAccountCryptographicStateRequestModel ?

Copy link
Contributor Author

@Thomas-Avery Thomas-Avery Mar 18, 2026

Choose a reason for hiding this comment

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

The problem is AccountsKeyRequestModel has been augmented to handle both v1 and v2 encryption payloads (plus has some leftover properties) . @quexten had requested to make a new model that only accepts v2 encryption payloads.

I don't mind adding a comment on AccountKeysRequestModel but we will have to make it clear we can only fully move over to WrappedAccountCryptographicStateRequestModel after we stop v1 encryption rotation support.

I'm not following on the ticket linked (PM-22384). That one is already done and where we added v2 encryption support to AccountsKeyRequestModel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a note on the new request model bc6bfe5.

I also created a ticket for tracking the tech debt for user key rotation https://bitwarden.atlassian.net/browse/PM-33860. I don't think now is the correct time to mark AccountKeysRequestModel as obsolete. It is used in many places that will still need to support v1 encryption payloads.

Copy link
Contributor

Choose a reason for hiding this comment

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

I provided wrong link, i meant https://bitwarden.atlassian.net/browse/PM-23751

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets leave that out of scope for this PR. Since this is a new endpoint I was planning on doing the QA after merging and introducing usage on the client. For removing those properties I would want to do a regression test.

@sonarqubecloud
Copy link

@Thomas-Avery Thomas-Avery requested a review from mzieniukbw March 18, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants