[PM-7840] Implement the stubbed out Passkey uniffi API #779

dani-garcia · 2024-05-13T12:32:34Z

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-rs instead
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 CheckUserOptions to 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.rs file. Also implemented conversion between these types and passkey types using From/TryFrom when possible.

There are still some open questions:

Some of the callbacks from passkey-rs force us to return StatusCode or Ctap2Error, 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 Passkey to Fido2CredentialView has a few hardcoded values, is that expected?
I left a few // TODO(Fido2): comments around to mention some other small questions I have

github-actions · 2024-05-13T12:37:15Z

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

codecov · 2024-05-13T12:38:10Z

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

Files	Patch %	Lines
...ates/bitwarden/src/platform/fido2/authenticator.rs	0.00%	328 Missing ⚠️
crates/bitwarden/src/platform/fido2/mod.rs	29.57%	100 Missing ⚠️
crates/bitwarden/src/vault/cipher/login.rs	2.29%	85 Missing ⚠️
crates/bitwarden/src/platform/fido2/types.rs	31.68%	69 Missing ⚠️
crates/bitwarden/src/platform/fido2/client.rs	0.00%	54 Missing ⚠️
crates/bitwarden-uniffi/src/platform/fido2.rs	0.00%	44 Missing ⚠️
crates/bitwarden/src/vault/cipher/cipher.rs	0.00%	28 Missing ⚠️
crates/bitwarden/src/platform/fido2/crypto.rs	0.00%	24 Missing ⚠️
crates/bitwarden/src/platform/fido2/traits.rs	0.00%	1 Missing ⚠️

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

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

coroiu

Given it a quick look and added some comments where I found issues

crates/bitwarden/src/platform/fido2/authenticator.rs

- Add Options to MakeCredentialRequest - Add `is_verification_enabled` to `Fido2UserInterface` - Prepend `Fido2` to interfaces

…and make_credential when running check_user

coroiu

Errors looking good! I think we could even expand on this a little bit and allow the platform to return it's own codes, that way iOS and Android can create their own UserInterfaceRequired errors that they can throw when user verification is requested inside of a flow without UI access

crates/bitwarden/src/platform/fido2/authenticator.rs

Hinton

Looks like a good start, plenty of TODOs left but some initial comments.

languages/swift/iOS/App/ContentView.swift

crates/bitwarden/src/vault/cipher/login.rs

crates/bitwarden/src/platform/client_platform.rs

crates/bitwarden/src/platform/fido2/crypto.rs

crates/bitwarden/src/platform/fido2/mod.rs

coroiu

Reviewed up to authenticator.rs, going to continue tomorrow!

crates/bitwarden-uniffi/src/platform/fido2.rs

crates/bitwarden/src/platform/fido2/authenticator.rs

coroiu · 2024-05-24T14:10:36Z

crates/bitwarden/src/platform/fido2/authenticator.rs

+            let picked = this
+                .authenticator
+                .user_interface
+                .pick_credential_for_authentication(creds)
+                .await?;


issue/question: This should only be called get_assertion, is that the case?

Ah yeah very true, I've updated this to only be called when doing authentication/assertion in 7a4b8b1

coroiu · 2024-05-24T14:18:00Z

crates/bitwarden/src/platform/fido2/authenticator.rs

+            let mut picked = this
+                .authenticator
+                .user_interface
+                .pick_credential_for_creation(creds, cred.clone().into())
+                .await?;


issue: this needs to be updated to use check_user_and_pick_credential_for_creation and be called from the UserInterface instead now that we've added support for UIHint we should have all the information needed to make the call there instead

Oh yep! I've updated it to be called from check_user in 7a4b8b1

crates/bitwarden/src/platform/fido2/authenticator.rs

crates/bitwarden/src/platform/fido2/mod.rs

coroiu

This is the last of my review :)

crates/bitwarden/src/platform/fido2/types.rs

coroiu · 2024-05-28T06:47:41Z

crates/bitwarden/src/platform/fido2/types.rs

+// To keep compatibility with passkey-rs ClientData, we want extra_client_data to return a type
+// that serializes to Object | Unit. If we just used an Option it would serialize to Object | None
+// So we create our own wrapper types that serialize to the correct values and make the ClientData
+// implementation return that.
+#[derive(Clone)]
+pub(super) struct OptionalAndroidClientData {
+    data: Option<AndroidClientData>,
+}
+
+#[derive(Serialize, Clone)]
+struct AndroidClientData {
+    android_package_name: String,
+}


question: Does the AndroidClientData struct need to be optional though? The functions that need it will never be used by anything other than Android?

The value is optional because we need a generic type that implements serialize and covers both the DefaultWithExtraData and DefaultWithCustomHash branches of the enum.

This way, extra_client_data returns the following:

For DefaultWithExtraData we return Some(AndroidClientData), which gets serialized and then flattened, adding the androidPackageName field to ClientDataJson

For DefaultWithCustomHash we return None, which gets serialized and then flattened, and leaves the ClientDataJson as-is

We could also change the type to be something like this, which might work as well:

#[derive(Clone, Serialize)] #[serde(rename_all = "camelCase")] pub(super) struct SerializableClientData { #[serde(skip_serializing_if = "Option::is_none")] android_package_name: Option<String>, }

crates/bitwarden/src/platform/fido2/types.rs

# Conflicts: # crates/bitwarden/Cargo.toml # crates/bitwarden/src/platform/client_platform.rs # crates/bitwarden/src/platform/fido2/authenticator.rs # crates/bitwarden/src/platform/fido2/client.rs # crates/bitwarden/src/platform/fido2/traits.rs # crates/bitwarden/src/platform/mod.rs

…sert UV in client

dani-garcia · 2024-05-30T12:08:37Z

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_credential and update_credential only get the selected_credential from the mutex, modify it as needed and call the clients save_credential, which simplifies them quite a bit.
On check_user we either call the client's check_user or check_user_and_pick_credential_for_creation, depending on the provided hint.
On find_credentials, we only call pick_credential_for_authentication when doing authentication.
On Client we 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()

Hinton · 2024-05-31T12:56:12Z

Would be nice if we could write some test cases, but looks good.

# Conflicts: # crates/bitwarden-crypto/src/uniffi_support.rs # crates/bitwarden-uniffi/src/uniffi_support.rs # crates/bitwarden/src/platform/fido2/authenticator.rs # crates/bitwarden/src/platform/fido2/client.rs # crates/bitwarden/src/uniffi_support.rs # crates/bitwarden/src/vault/cipher/login.rs

dani-garcia · 2024-06-03T11:47:47Z

Updated to remove the usages of Sensitive from the types

coroiu

Looking good! Thanks for adding the b64. decoding, will be nice to have it if we need it!

coroiu · 2024-06-04T07:37:26Z

crates/bitwarden/src/platform/fido2/authenticator.rs

-                _ => return Err("Only one credential should match the id".into()),
-            };
+            // Get the previously selected cipher and update the credential
+            let selected = this.authenticator.get_selected_credential()?;


suggestion(non-blocking): maybe just add a sanity check here to verify that the id matches?

Ah nice! Added it in f852dbf

dani-garcia force-pushed the ps/uniffi-passkey-improvements branch from bb0ea1d to 0b8bd33 Compare May 13, 2024 13:50

dani-garcia added 2 commits May 13, 2024 15:53

Improvements to Passkey uniffi API

419101d

Here we go again with the uniffi workaround

d2041a3

dani-garcia force-pushed the ps/uniffi-passkey-improvements branch from 0b8bd33 to d2041a3 Compare May 13, 2024 14:25

dani-garcia added 2 commits May 13, 2024 17:14

Remove stub credential

07642dc

Add a warning about silent discover

9e704a4

dani-garcia requested a review from coroiu May 13, 2024 16:03

coroiu reviewed May 14, 2024

View reviewed changes

dani-garcia added 9 commits May 14, 2024 12:19

Changes based on review

c21f94a

Fmt

26a3c63

Change API based on latest spec

16b8b03

- Add Options to MakeCredentialRequest - Add `is_verification_enabled` to `Fido2UserInterface` - Prepend `Fido2` to interfaces

Update passkey-rs and respect the UV value provided in get_assertion …

27ca115

…and make_credential when running check_user

Update passkey-rs for async is_verification_enabled

c63d806

Merge branch 'main' into ps/uniffi-passkey-improvements

82dac1b

Handle URL parse error

3a82eec

Return valid authenticator data

77f07a0

Use valid error codes

499ebb9

dani-garcia changed the title ~~Implement the stubbed out Passkey uniffi API~~ [PM-7840] Implement the stubbed out Passkey uniffi API May 14, 2024

Merge branch 'main' into ps/uniffi-passkey-improvements

5478d3c

dani-garcia marked this pull request as ready for review May 14, 2024 15:11

dani-garcia requested a review from coroiu May 14, 2024 15:11

coroiu reviewed May 15, 2024

View reviewed changes

crates/bitwarden/src/platform/fido2/authenticator.rs Outdated Show resolved Hide resolved

dani-garcia and others added 3 commits May 15, 2024 11:21

Remove unneeded require_resident_key field

3a03ba5

Change where we transform UV (#786)

2832bcf

Actually call pick_credential_for_authentication

ad3e4de

Hinton reviewed May 17, 2024

View reviewed changes

dani-garcia added 2 commits May 17, 2024 17:55

Remove crypto copied from passkey-rs

1e8c37f

Fix some review comments

55938d6

dani-garcia requested a review from Hinton May 24, 2024 08:56

coroiu reviewed May 24, 2024

View reviewed changes

requested_uv and uv conversion

8caaeed

coroiu reviewed May 27, 2024

View reviewed changes

crates/bitwarden/src/platform/fido2/mod.rs Outdated Show resolved Hide resolved

coroiu reviewed May 28, 2024

View reviewed changes

dani-garcia added 6 commits May 28, 2024 12:19

Remove comments

5143d1d

Remove some unnecessary locks and takes

432e06a

Simplify ClientData

99b7f56

Update pick_credential_for_creation, move functions to check_user, in…

7a4b8b1

…sert UV in client

Support b64 credential IDs

2d297b7

Hinton requested a review from coroiu May 31, 2024 12:38

Hinton previously approved these changes May 31, 2024

View reviewed changes

dani-garcia added 2 commits June 3, 2024 13:40

Update iOS example

7a2680f

dani-garcia dismissed Hinton’s stale review via 7a2680f June 3, 2024 11:47

dani-garcia requested a review from Hinton June 3, 2024 11:47

dani-garcia added 2 commits June 3, 2024 15:18

Merge branch 'main' into ps/uniffi-passkey-improvements

e111192

Update passkey-rs

75dc8d3

coroiu previously approved these changes Jun 4, 2024

View reviewed changes

Validate that the provided credential ID matches what is stored

f852dbf

dani-garcia dismissed coroiu’s stale review via f852dbf June 4, 2024 10:13

dani-garcia requested a review from coroiu June 4, 2024 10:14

coroiu approved these changes Jun 4, 2024

View reviewed changes

Hinton approved these changes Jun 4, 2024

View reviewed changes

dani-garcia merged commit ea93ac5 into main Jun 4, 2024
104 checks passed

dani-garcia deleted the ps/uniffi-passkey-improvements branch June 4, 2024 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-7840] Implement the stubbed out Passkey uniffi API #779

[PM-7840] Implement the stubbed out Passkey uniffi API #779

dani-garcia commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

coroiu left a comment

coroiu left a comment

Hinton left a comment

coroiu left a comment

coroiu May 24, 2024

dani-garcia May 30, 2024

coroiu May 24, 2024

dani-garcia May 30, 2024

coroiu left a comment

coroiu May 28, 2024

dani-garcia May 30, 2024

dani-garcia commented May 30, 2024 •

edited

Loading

Hinton commented May 31, 2024

dani-garcia commented Jun 3, 2024

coroiu left a comment

coroiu Jun 4, 2024

dani-garcia Jun 4, 2024

[PM-7840] Implement the stubbed out Passkey uniffi API #779

[PM-7840] Implement the stubbed out Passkey uniffi API #779

Conversation

dani-garcia commented May 13, 2024 • edited Loading

Type of change

Objective

github-actions bot commented May 13, 2024 • edited Loading

New Issues

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

coroiu left a comment

Choose a reason for hiding this comment

coroiu left a comment

Choose a reason for hiding this comment

Hinton left a comment

Choose a reason for hiding this comment

coroiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coroiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia commented May 30, 2024 • edited Loading

Hinton commented May 31, 2024

dani-garcia commented Jun 3, 2024

coroiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-garcia commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

dani-garcia commented May 30, 2024 •

edited

Loading