Rework CTAP2.0 GetPinToken-workflow #237

msirringhaus · 2023-03-22T12:28:57Z

Trying to rework the GetPinToken-workflow, in order to work for tokens with and without UV.

Tested this with Yubikey BIO.
Would be great to if @andreydanil and @VincentVanlaer could also test this with their ThinC and Mooltipass respectively.

To test, run examples ctap2 and ctap2_discoverable_credentials with this patch and see if

the good case works
using the wrong finger / PIN will act as expected as well (either repeatedly ask you to touch it, or error out completely after 3 tries).

msirringhaus · 2023-03-22T13:52:59Z

Once #205 lands, this needs of course some rebasing, and then it should be hopefully rather straight forward to implement the CTAP2.1 auth-workflows.

andreydanil · 2023-03-22T15:35:35Z

Nice! I can confirm that this code change does not break ThinC CTAP2.0 token.

I wasn't able to build this change into Firefox due to a reason with a dependency with error error: could not compile authrs_bridge due to previous error.

Compiling authrs_bridge v0.1.0 (/Users/andreydanil/development/github/mozilla-unified/dom/webauthn/authrs_bridge)
error[E0004]: non-exhaustive patterns: `Ok(StatusUpdate::PinAuthInvalid)` not covered
    --> dom/webauthn/authrs_bridge/src/lib.rs:338:15
     |
     |         match status_rx.recv() {
     |               ^^^^^^^^^^^^^^^^ pattern `Ok(StatusUpdate::PinAuthInvalid)` not covered

I would love to test UV = Discouraged, Preferred, Required once I can build this into Firefox.

src/statemachine.rs

jschanck · 2023-03-22T16:58:52Z

src/statemachine.rs

+                                    callback.call(Err(e));
+                                }
+                            }
+                        }


Similarly, let's add a helper function for the error cases. There's a lot of duplication with sign, and right now they differ unnecessarily.

Sadly, they do NOT differ unnecessarily. They actually are different, because the spec (or at least how vendors interpret it) are different for GetAssertion and MakeCredential, in what they return in case of errors.

Can't we just handle the difference in repackage_pin_errors? If not, I think we should avoid using repackage_pin_errors entirely. A flat list of cases would be easier to follow.

Since we need repackage_pin_errors in other places as well, I've introduced a new function (which also uses it), that handles the differences, so we can move all of it out of statemachine.rs

jschanck

Looks good. Some minor comments below.

src/statemachine.rs

jschanck · 2023-03-23T21:28:57Z

src/statemachine.rs

+                                    callback.call(Err(e));
+                                }
+                            }
+                        }


Can't we just handle the difference in repackage_pin_errors? If not, I think we should avoid using repackage_pin_errors entirely. A flat list of cases would be easier to follow.

VincentVanlaer · 2023-03-24T08:26:55Z

I tested this PR with my mooltipass:

The ctap2 example works
The ctap2_discoverable_creds does not work. Registration of the multiple users works well, but fails on unwrapping name in https://github.com/mozilla/authenticator-rs/blob/ctap2-2021/examples/ctap2_discoverable_creds.rs#L342 I does have the same issue before this PR, so that's not a blocker

msirringhaus · 2023-03-24T12:39:44Z

Thank you @andreydanil and @VincentVanlaer for testing.

@VincentVanlaer: Could you open an issue for the failing ctap2_discoverable_creds-example?

jschanck · 2023-03-24T17:37:27Z

src/ctap2/commands/mod.rs

+        Ok(PinUvAuthResult::SuccessGetPinToken)
+    }
+
+    fn handle_auth_command_errors<D: FidoDevice>(


This does not really "unify the cases" as we're now just pushing the dependence on Self::command() into this function. I'm sorry to go back and forth on this minor point, but I think we should do the flat list of cases instead.

So reverting this change and doing what we did before? Or something else entirely?
"Avoiding duplicate code, if possible" and "having a flat list" unfortunately are at odds in this case.

Maybe it will be easier for me to just submit a follow up patch with what I'm imagining. This is not as clear as I'd like it to be, but it does appear to be correct.

jschanck · 2023-03-24T17:53:11Z

src/ctap2/commands/mod.rs

+        }
+
+        let pin_auth = dev
+            .get_pin_token(&self.client_data_hash(), self.pin())


This will ultimately call set_shared_secret on Device. Where are we doing that in the UsingInternalUv case?

Maybe we need to implement and integrate getPinUvAuthTokenUsingUvWithPermissions before we can land this patch?

Ah, you are right. I was under the impression, that we do not need the shared_secret, if we don't use a PIN. But we may need it for the hmac-extension (which I'm currently not sure, if it works correctly anyways).
I'll add a dev.establish_shared_secret()?; to the UsingInternalUv-case.

Waiting for getPinUvAuthTokenUsingUvWithPermissions will (probably) not change anything here, since the plan is to do that in a separate function (hence the name determine_pin_uv_auth_ctap_2_0()).
The InternalUv-case needs to work for CTAP2.0-only tokens as well.

OK, sounds good. I didn't realize that Internal UV did not require the shared secret in 2.0, but we do need it for the hmac extension (even though it's not currently working).

src/status_update.rs

examples/ctap2.rs

jschanck

Looks good!

jschanck · 2023-03-27T17:10:35Z

src/ctap2/commands/mod.rs

+        }
+
+        let pin_auth = dev
+            .get_pin_token(&self.client_data_hash(), self.pin())


OK, sounds good. I didn't realize that Internal UV did not require the shared secret in 2.0, but we do need it for the hmac extension (even though it's not currently working).

jschanck · 2023-03-27T17:18:20Z

src/ctap2/commands/mod.rs

+        Ok(PinUvAuthResult::SuccessGetPinToken)
+    }
+
+    fn handle_auth_command_errors<D: FidoDevice>(


Maybe it will be easier for me to just submit a follow up patch with what I'm imagining. This is not as clear as I'd like it to be, but it does appear to be correct.

VincentVanlaer · 2023-03-29T07:48:36Z

@VincentVanlaer: Could you open an issue for the failing ctap2_discoverable_creds-example?

I did some digging in the meantime and it seems that the mooltipass is not returning the name field, even though it is set when the credential is created (and is stored as I can see it on the screen). There does seem to be a possibility in the spec to not send the name field back when the user is not verified, but I consider that a discussion to be had on the mooltipass side whether this applies or not (I'm assuming it doesn't really apply and the mooltipass should send the name field)

msirringhaus force-pushed the fix_uv branch from aff97b0 to 117efca Compare March 22, 2023 16:30

jschanck reviewed Mar 22, 2023

View reviewed changes

msirringhaus force-pushed the fix_uv branch from 117efca to 515f27c Compare March 23, 2023 08:39

jschanck approved these changes Mar 23, 2023

View reviewed changes

Martin Sirringhaus added 2 commits March 24, 2023 08:23

Some CTAP 2.1 prep work

76f3691

Rework CTAP2.0 GetPinToken-workflow

e6144ca

msirringhaus force-pushed the fix_uv branch from 515f27c to a74b377 Compare March 24, 2023 12:32

jschanck reviewed Mar 24, 2023

View reviewed changes

jschanck self-requested a review March 24, 2023 17:54

andreydanil reviewed Mar 24, 2023

View reviewed changes

src/status_update.rs Show resolved Hide resolved

andreydanil reviewed Mar 24, 2023

View reviewed changes

examples/ctap2.rs Show resolved Hide resolved

Address pr237-feedback

98a9b1b

msirringhaus force-pushed the fix_uv branch from a74b377 to 98a9b1b Compare March 27, 2023 12:04

jschanck approved these changes Mar 27, 2023

View reviewed changes

jschanck merged commit 6700196 into mozilla:ctap2-2021 Mar 27, 2023

VincentVanlaer mentioned this pull request Mar 29, 2023

Webauthn Linux Browser Support Tracker mooltipass/minible#217

Open

4 tasks

msirringhaus mentioned this pull request Mar 29, 2023

ctap2_discoverable_creds-example fails for mooltipass #242

Open

VincentVanlaer mentioned this pull request Apr 16, 2023

Skip shared secret if pin and hmac-secret are not supported #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework CTAP2.0 GetPinToken-workflow #237

Rework CTAP2.0 GetPinToken-workflow #237

msirringhaus commented Mar 22, 2023

msirringhaus commented Mar 22, 2023

andreydanil commented Mar 22, 2023

jschanck Mar 22, 2023

msirringhaus Mar 23, 2023

jschanck Mar 23, 2023

msirringhaus Mar 24, 2023

jschanck left a comment

jschanck Mar 23, 2023

VincentVanlaer commented Mar 24, 2023

msirringhaus commented Mar 24, 2023

jschanck Mar 24, 2023

msirringhaus Mar 27, 2023

jschanck Mar 27, 2023

jschanck Mar 24, 2023

msirringhaus Mar 27, 2023

jschanck Mar 27, 2023

jschanck left a comment

jschanck Mar 27, 2023

jschanck Mar 27, 2023

VincentVanlaer commented Mar 29, 2023

Rework CTAP2.0 GetPinToken-workflow #237

Rework CTAP2.0 GetPinToken-workflow #237

Conversation

msirringhaus commented Mar 22, 2023

msirringhaus commented Mar 22, 2023

andreydanil commented Mar 22, 2023

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

jschanck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentVanlaer commented Mar 24, 2023

msirringhaus commented Mar 24, 2023

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

Choose a reason for hiding this comment

jschanck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentVanlaer commented Mar 29, 2023