Skip to content

[PM-30584] Add support for key-connector-migration setting key#7136

Merged
quexten merged 14 commits intomainfrom
km/key-connector-set-key
Mar 11, 2026
Merged

[PM-30584] Add support for key-connector-migration setting key#7136
quexten merged 14 commits intomainfrom
km/key-connector-set-key

Conversation

@quexten
Copy link
Contributor

@quexten quexten commented Mar 4, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-30584
#7136
bitwarden/sdk-internal#809
bitwarden/clients#19360

📔 Objective

We do not want to keep the same master-key that was used during password derivation, but use a separately sampled key - named "key-connector-key". This means that the conversion flow now requires:

  • Posting the key to key-connector
  • Setting the "key" field on the user to "key-connector-key-wrapped-user-key"

To keep backwards compatibility, we make the request body optional, but if present, the request body contains the key-connector-key-wrapped-user-key. This is subsequently set to the user object.

This will unblock setting the master-key to state during unlock and login, which will improve unlock time, since we can remove double-kdf-derivation.

📸 Screenshots

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Logo
Checkmarx One – Scan Summary & Detailsf8adeafa-196c-4f37-b3b2-25b8fe88e081


New Issues (4) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 HIGH Path_Traversal /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs: 56
detailsMethod at line 56 of /src/Api/Controllers/SelfHosted/SelfHostedOrganizationLicensesController.cs gets dynamic data from the model element. This ...
Attack Vector
2 MEDIUM CSRF /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 202
detailsMethod at line 202 of /src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs gets a parameter from a user request from model. Thi...
Attack Vector
3 MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 217
detailsMethod at line 217 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow...
Attack Vector
4 MEDIUM CSRF /src/Api/Auth/Controllers/AccountsController.cs: 291
detailsMethod at line 291 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow...
Attack Vector

Fixed Issues (1) Great job! The following issues were fixed in this Pull Request
Severity Issue Source File / Package
MEDIUM CSRF /src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 531

@quexten quexten changed the title Add support for key-connector-migration setting key [PM-30584] Add support for key-connector-migration setting key Mar 4, 2026
@quexten quexten force-pushed the km/key-connector-set-key branch from 03a81f1 to 23aaddb Compare March 4, 2026 14:07
@codecov
Copy link

codecov bot commented Mar 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.40%. Comparing base (bf9bc84) to head (c08c8dc).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7136      +/-   ##
==========================================
+ Coverage   57.38%   57.40%   +0.01%     
==========================================
  Files        2031     2032       +1     
  Lines       89283    89311      +28     
  Branches     7936     7941       +5     
==========================================
+ Hits        51237    51265      +28     
  Misses      36205    36205              
  Partials     1841     1841              

☔ 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 quexten marked this pull request as ready for review March 5, 2026 11:25
@quexten quexten requested a review from a team as a code owner March 5, 2026 11:25
@quexten quexten requested a review from Thomas-Avery March 5, 2026 11:25
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

Code changes look good. Would be nice to get some coverage for the user service method.

@quexten quexten marked this pull request as draft March 10, 2026 11:29
@quexten quexten marked this pull request as draft March 10, 2026 11:29
@sonarqubecloud
Copy link

@quexten quexten marked this pull request as ready for review March 10, 2026 12:34
@quexten quexten requested a review from Thomas-Avery March 10, 2026 12:34
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

LGTM

@quexten quexten merged commit 79089a0 into main Mar 11, 2026
45 checks passed
@quexten quexten deleted the km/key-connector-set-key branch March 11, 2026 08:15
quexten added a commit to bitwarden/sdk-internal that referenced this pull request Mar 13, 2026
## 🎟️ Tracking

<!-- Paste the link to the Jira or GitHub issue or otherwise describe /
point to where this change is coming from. -->
https://bitwarden.atlassian.net/browse/PM-30584
bitwarden/server#7136
#809
bitwarden/clients#19360

## 📔 Objective

<!-- Describe what the purpose of this PR is, for example what bug
you're fixing or new feature you're adding. -->
We do not want to keep the same master-key that was used during password
derivation, but use a separately sampled key - named
"key-connector-key". This means that the conversion flow now requires:
- Posting the key to key-connector
- Setting the "key" field on the user to
"key-connector-key-wrapped-user-key"

A new request endpoint is used compared to the previous
key-connector-migration endpoint. This new endpoint always requires the
key-connector-key-wrapped-user-key in the request body.

This will unblock setting the master-key to state during unlock and
login, which will improve unlock time, since we can remove
double-kdf-derivation.

## 📸 Screenshots

<!-- Required for any UI changes; delete if not applicable. Use fixed
width images for better display. -->
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