[PM-22136] Expose encrypt_fido2_credentials to support cipher encryption in TS clients

[shane-melton]

🎟️ Tracking

PM-22136

📔 Objective

The TS clients use fully decrypted FIDO2 credentials which need to be re-encrypted separately before using the SDK to encrypt the rest of the Cipher. Otherwise, the FIDO2 credential encryption key will be lost.

Once the decrypted FIDO2 Credentials are removed from the LoginView in TS, this method can be removed.

Related Clients PR: bitwarden/clients#15337

⏰ 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 – 8d1fe362-520e-47e4-93a5-abfa1f8e0054

Great job, no security vulnerabilities found in this Pull Request

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 71.85%. Comparing base (5211667) to head (384f9d3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/bitwarden-vault/src/cipher/cipher_client.rs	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #331      +/-   ##
==========================================
- Coverage   71.88%   71.85%   -0.04%     
==========================================
  Files         240      240              
  Lines       19625    19635      +10     
==========================================
  Hits        14108    14108              
- Misses       5517     5527      +10     

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

[Hinton]

Can we please provide some more information about why this is needed? The mobile clients uses this implementation so if we have issues with fido 2 credentials not being encrypted correctly that would apply to mobile as well and is something we need to address.

Necessary while the FIDO2 credentials remain in the LoginView in an encrypted form.

As in you want to expose them decrypted? That shouldn't be necessary ever. Or you want to remove it once the SDK can load from state?

It's there in encrypted form so that we can generate a cipher key if needed as we have to re-encrypt the whole object.

[gbubemismith]

Changes look good. Just waiting for the request changes to come in before approving

[shane-melton]

@Hinton

Can we please provide some more information about why this is needed? The mobile clients uses this implementation so if we have issues with fido 2 credentials not being encrypted correctly that would apply to mobile as well and is something we need to address.

The TS clients do not currently use the SDK for creating fido 2 credentials. So, whenever we encrypt ciphers with new credentials with the SDK we do not have an encrypted copy to pass along to the SDK. In order to use the SDK for all encryption, we need to expose this method in the meantime to explicitly encrypt the fido2 credentials.

Mobile uses the SDK for Fido2 credential creation so they don't need to worry about encrypting the credentials manually.

Once the TS clients use the SDK for Fido2 credential management (PM-8313) we'll be able to remove this and the decrypted Fido2Credentials from the LoginView in the TS clients.

Or you want to remove it once the SDK can load from state?

Yep, we can remove the credentials from the LoginView once the SDK can load from state. Then we won't need to always pass the encrypted fido 2 credentials back and forth (as far as I understand it)

[Hinton]

There is a set_new_fido2_credentials on CipherView for this purpose which is currently used internally in the sdk for the fido authenticator. This can probably be exposed with some simple glue code.

sdk-internal/crates/bitwarden-vault/src/cipher/cipher.rs

Lines 563 to 575 in f2bc708

	pub fn set_new_fido2_credentials(
	&mut self,
	ctx: &mut KeyStoreContext<KeyIds>,
	creds: Vec<Fido2CredentialFullView>,
	) -> Result<(), CipherError> {
	let key = self.key_identifier();
	let ciphers_key = Cipher::decrypt_cipher_key(ctx, key, &self.key)?;
	require!(self.login.as_mut()).fido2_credentials =
	Some(creds.encrypt_composite(ctx, ciphers_key)?);
	Ok(())

cipher_view.set_new_fido2_credentials(creds);

[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

[shane-melton]

@Hinton Thanks for pointing that out! Updated in efaf7fd

[Hinton]

LGTM