[PM-2199] Implement userkey rotation for all TDE devices

[quexten]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-2199
Clients PR: bitwarden/clients#13576

📔 Objective

Adds trusted devices to the key-rotation v2 endpoint. This requires removing the secretverification request from the {identifier}/retrieve-keys endpoint. This is acceptable since it retrieves an encrypted copy of the keys, and also is existing behavior for e.g. PRF keys.

Further, this is a net-new, non-optional field (the device array) in the request body. But since userkey-rotation v2 is not rolled out and behind a feature-flag, we can add it as long as we make sure the client version contains the changes when activating the feature flag.

The validator behavior is as follows:
All trusted devices need to be included in the rotation request. No non-trusted devices shall be included in the response model. Trusted devices need to have the encryptedPublicKey and encryptedUserkey present.

📸 Screenshots

⏰ 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 – df28067d-134b-4fac-8691-bb4c1b2f2385

New Issues (4)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 164	details Method Put at line 164 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from model. This parameter val... Attack Vector
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 94	details Method Put at line 94 of /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs gets a parameter from a user request from model. This param... Attack Vector
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 104	details Method Patch at line 104 of /bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs gets a parameter from a user request from model. This pa... Attack Vector
	CSRF	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 133	details Method Put at line 133 of /src/Api/AdminConsole/Public/Controllers/GroupsController.cs gets a parameter from a user request from model. This parame... Attack Vector

[codecov]

Codecov Report

Attention: Patch coverage is 44.79167% with 53 lines in your changes missing coverage. Please review.

Project coverage is 47.77%. Comparing base (a3734f6) to head (c24c29c).

Files with missing lines	Patch %	Lines
...astructure.Dapper/Repositories/DeviceRepository.cs	0.00%	29 Missing ⚠️
...e.EntityFramework/Repositories/DeviceRepository.cs	0.00%	23 Missing ⚠️
...eyManagement/Validators/DeviceRotationValidator.cs	95.65%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##           km/userkey-rotation-v2    #5446      +/-   ##
==========================================================
- Coverage                   47.78%   47.77%   -0.01%     
==========================================================
  Files                        1538     1539       +1     
  Lines                       71264    71354      +90     
  Branches                     6396     6400       +4     
==========================================================
+ Hits                        34050    34091      +41     
- Misses                      35792    35840      +48     
- Partials                     1422     1423       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[quexten]

Note for reviewers: This is fixing an existing bug where the device trust status is incorrectly reported in the device list request.

[ike-kottlowski]

Have a question about the query.

[ike-kottlowski]

🎨 : I'm of the opinion that this should be a Stored Procedure. I understand this is essentially the same thing, but for consistency's sake it might make sense to either create a SP or to use Devices_Update via the Repository.Replace(T obj).

[quexten]

Hmm, the intent here was to stay consistent with what the other auth repositories do for rotation:

server/src/Infrastructure.Dapper/Auth/Repositories/WebAuthnCredentialRepository.cs

Lines 64 to 86 in 10756ca

	public UpdateEncryptedDataForKeyRotation UpdateKeysForRotationAsync(Guid userId, IEnumerable<WebAuthnLoginRotateKeyData> credentials)
	{
	return async (SqlConnection connection, SqlTransaction transaction) =>
	{
	const string sql = @"
	UPDATE WC
	SET
	WC.[EncryptedPublicKey] = UW.[encryptedPublicKey],
	WC.[EncryptedUserKey] = UW.[encryptedUserKey]
	FROM
	[dbo].[WebAuthnCredential] WC
	INNER JOIN
	OPENJSON(@JsonCredentials)
	WITH (
	id UNIQUEIDENTIFIER,
	encryptedPublicKey NVARCHAR(MAX),
	encryptedUserKey NVARCHAR(MAX)
	) UW
	ON UW.id = WC.Id
	WHERE
	WC.[UserId] = @UserId";
	var jsonCredentials = CoreHelpers.ClassToJsonData(credentials);

server/src/Infrastructure.Dapper/Auth/Repositories/EmergencyAccessRepository.cs

Lines 106 to 146 in 10756ca

	return async (SqlConnection connection, SqlTransaction transaction) =>
	{
	// Create temp table
	var sqlCreateTemp = @"
	SELECT TOP 0 *
	INTO #TempEmergencyAccess
	FROM [dbo].[EmergencyAccess]";
	await using (var cmd = new SqlCommand(sqlCreateTemp, connection, transaction))
	{
	cmd.ExecuteNonQuery();
	}
	// Bulk copy data into temp table
	using (var bulkCopy = new SqlBulkCopy(connection, SqlBulkCopyOptions.KeepIdentity, transaction))
	{
	bulkCopy.DestinationTableName = "#TempEmergencyAccess";
	var emergencyAccessTable = emergencyAccessKeys.ToDataTable();
	foreach (DataColumn col in emergencyAccessTable.Columns)
	{
	bulkCopy.ColumnMappings.Add(col.ColumnName, col.ColumnName);
	}
	emergencyAccessTable.PrimaryKey = new DataColumn[] { emergencyAccessTable.Columns[0] };
	await bulkCopy.WriteToServerAsync(emergencyAccessTable);
	}
	// Update emergency access table from temp table
	var sql = @"
	UPDATE
	[dbo].[EmergencyAccess]
	SET
	[KeyEncrypted] = TE.[KeyEncrypted]
	FROM
	[dbo].[EmergencyAccess] E
	INNER JOIN
	#TempEmergencyAccess TE ON E.Id = TE.Id
	WHERE
	E.[GrantorId] = @GrantorId
	DROP TABLE #TempEmergencyAccess";

Are these planned to be changed to SP's too?

[ike-kottlowski]

Well, now that's interesting 🫠.

Do we think the intent with the rotation queries being done this way is to capture the changes in one query transaction rather than a loop of transactions by using the Repository.Replace(T obj)?

[quexten]

Yeah, one big transaction query is required to ensure consistency during key rotation.

Where possible both on the Client <-> Server API, and on Server <-> DB we've been moving to one big atomic change to prevent any kind of connection loss from making the account move to an inconsistent / undecryptable state.

(For instance, for TDE, if we just use repository.replace, I think it would be possible that a temporary connection loss to the sql db, would lead to only some TDE devices having an updated copy of the userkey, and some devices would be able to log in, some would not).

I think this change to one big transaction was originally made during auth times by @jlf0dev though. Maybe he can confirm the intent.