bitwarden
[PM-7840] Implement the stubbed out Passkey uniffi API
Type of change
- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
Objective
This PR adds our fork of passkey-rs to bitwarden to implement the previously stubbed out API. With it come some changes:
- Swapped most parts of the stubbed implementations to use
passkey-rsinstead - Some API callbacks were changed to take decrypted items, the clients would need these decrypted to show the user anyway, and we can skip a few rounds of decrypting/encrypting this way. Note that the FIDO2 credentials private keys are never exposed decrypted to the clients.
- Added a separate
Fido2CredentialNewView, the only difference being that it doesn't contain the private key. This is used to send to the clients in the cipher select callback. Everywhere else we send back the encrypted field but at this point we don't have a key to encrypt the field yet. We could send a dummy value but that seems error prone. - Changed
CheckUserOptionsto be a struct instead of an enum. This was a mistake from the previous PR. - Moved a lot of types that were previously distributed among a few files into a specific
types.rsfile. Also implemented conversion between these types andpasskeytypes usingFrom/TryFromwhen possible.
There are still some open questions:
- Some of the callbacks from
passkey-rsforce us to returnStatusCodeorCtap2Error, how do we want to handle these? At the moment I'm just logging the error and returing a constant value. Do we need specific error values for certain errors to be spec compliant? - The conversion from
PasskeytoFido2CredentialViewhas a few hardcoded values, is that expected? - I left a few
// TODO(Fido2):comments around to mention some other small questions I have
Checkmarx One – Scan Summary & Details – e962bca3-4295-475c-8833-97ce82eb9a02
New Issues
| Severity | Issue | Source File / Package | Checkmarx Insight |
|---|---|---|---|
 |
Use_of_Hardcoded_Password | /languages/kotlin/app/src/main/java/com/bitwarden/myapplication/MainActivity.kt: [73](https://github.com/bitwarden/sdk/blob/ps/uniffi-passkey-improvements//languages/kotlin/app/src/main/java/com/bitwarden/myapplication/MainActivity.kt# L73) | Attack Vector |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
Attention: Patch coverage is 9.39431% with 733 lines in your changes missing coverage. Please review.
Project coverage is 56.67%. Comparing base (
db2ca31) to head (f852dbf).
Additional details and impacted files
@@ Coverage Diff @@
## main #779 +/- ##
==========================================
- Coverage 58.55% 56.67% -1.88%
==========================================
Files 178 181 +3
Lines 11405 11915 +510
==========================================
+ Hits 6678 6753 +75
- Misses 4727 5162 +435
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Okay, I've updated the code to use the new API check_user_and_pick_credential_for_creation and made sure to remove most UI logic from the Storage implementation:
-
Now
save_credentialandupdate_credentialonly get theselected_credentialfrom the mutex, modify it as needed and call the clientssave_credential, which simplifies them quite a bit. -
On
check_userwe either call the client'scheck_userorcheck_user_and_pick_credential_for_creation, depending on the provided hint. -
On
find_credentials, we only callpick_credential_for_authenticationwhen doing authentication. -
On
Clientwe now store the request UV values, which I forgot to do.
I've gone though the call flow a few times to make sure nothing is missing or out of order, and wrote this summary of the call order as it should be now:
passkey::Authenticator::make_credential()
self.required_uv.insert()
passkey::make_credential()
if exclude_list not empty:
passkey::CredentialStore::find_credentials()
trait bitwarden::Fido2CredentialStore::find_credentials()
if found excluded credential:
passkey::UserValidationMethod::check_user(InformExcludedCredentialFound)
self.required_uv.get()
trait bitwarden::Fido2UserInterface::check_user()
return
passkey::UserValidationMethod::check_user(RequestNewCredential)
self.required_uv.get()
trait bitwarden::Fido2UserInterface::check_user_and_pick_credential_for_creation()
self.selected_credential.insert()
passkey::CredentialStore::save_credential()
self.selected_credential.get()
self.selected_credential.insert()
trait bitwarden::Fido2CredentialStore::save_credential()
self.selected_credential.get()
bitwarden::Client::register()
self.required_uv.insert()
passkey::Client::register()
passkey::make_credential()
# From here, it's the same as inside passkey::make_credential() above
self.selected_credential.get()
passkey::Authenticator::get_assertion()
self.required_uv.insert()
passkey::get_assertion()
passkey::CredentialStore::find_credentials()
trait bitwarden::Fido2CredentialStore::find_credentials()
trait bitwarden::Fido2UserInterface::pick_credential_for_authentication()
self.selected_credential.insert()
if found credential:
passkey::UserValidationMethod::check_user(RequestExistingCredential)
self.required_uv.get()
trait bitwarden::Fido2UserInterface::check_user()
else:
passkey::UserValidationMethod::check_user(InformNoCredentialsFound)
self.required_uv.get()
trait bitwarden::Fido2UserInterface::check_user()
if counter:
passkey::CredentialStore::update_credential()
self.selected_credential.get()
self.selected_credential.insert()
trait bitwarden::Fido2CredentialStore::save_credential()
self.selected_credential.get()
bitwarden::Client::authenticate()
self.required_uv.insert()
passkey::Client::authenticate()
passkey::get_assertion()
# From here, it's the same as inside passkey::get_assertion() above
self.selected_credential.get()
