Add WebAuthn authentication flow #6792

scotttrinh · 2024-02-07T15:10:24Z

Spec: https://w3c.github.io/webauthn
WebAuthn Primers
- https://webauthn.guide/#authentication
- https://webauthn.me/introduction

Builds on #6791

Authentication is almost the same as registration:

Fetch the "options" from the server which include a challenge and allowCredentials that identifies the user based on the provided email + user_handle
The authenticator will sign with the stored private key and generate an assertion which includes the signed challenge, and other information about the authenticator.
The server verifies the signature and some other metadata, and if it passes considers the session authenticated.

Like the registration flow, it uses PKCE to do the final exchange of the actual token.

jaclarke · 2024-02-12T12:29:12Z

edb/server/protocol/auth_ext/webauthn.py

+        )
+        result_json = json.loads(result.decode())
+        user_handles: set[str] = {x["user_handle"] for x in result_json}
+        assert len(user_handles) == 1


Why do we assert that there's only a single user_handle associated with a given email? I thought we wanted to allow you to use the same email with multiple authenticators: #6791 (comment)
Unless they should all have the same user_handle (which I guess was the reason for dropping the exclusive constraint on (.email, .user_handle)?), in which case in create_registration_options_for_email we should check if there's already a WebAuthnFactor for that email and re-use the user_handle. I don't know if there's some security issue with being able to retrieve an email's user_handle like that though?

Yeah, we're supposed to use the same user_handle if the user is "the same user" to allow multiple WebAuthn factors for a single user. You're right that I'm missing that check from the create_registration_options_for_email but I need to think about the UX here a bit more.

The ostensible reason for allowing multiple factors is to allow what are basically back-up factors (or multi-factors) so you can seamlessly move between passkeys, hardware authenticators, etc. but in our "registration" flow it's not super obvious that you might have multiple identities with the same email which point to a single User in the downstream application because we kind of leave that up to the application developer to do.

But, yeah I think at the very least we should reuse the same user handle if you provide an existing email, will fix.

@jaclarke ⚡ ebda3e8 (#6792)

From testing it seems there's another issue with this assert. If the email isn't registered, then there's no user-handles,the assert fails and we return an ise. I'm not sure what the correct error should be here? I know we're trying not to leak the existence of emails, but I don't think we can get around that as we can only return a challenge if we have the stored credentials for an email.

Yeah, good catch: assert is the wrong tool here, we need to raise a specific exception for proper error handling.

As far as data leakage is concerned, I think we talked about register being fine to leak whether an email is signed up or not, but authenticate would always pretend it succeeds. From that perspective, I think we can return a sensible error here.

@jaclarke ⚡ https://github.com/edgedb/edgedb/pull/6792/files/40d636291fca7bb5db36ebd142e7e58fa2214bdd..1431063a019694d9028966b1ae1cbce2d8c6e93b

Turns out I needed a bit more work to ensure the invariant that multiple factors with the same email have the same user_handle. Added a test for the new trigger.

jaclarke · 2024-02-12T12:37:51Z

edb/server/protocol/auth_ext/_static/webauthn-authenticate.js

+  });
+
+  if (!authenticateResponse.ok) {
+    console.error(


I don't think this handles the case where email verification is required, so we'll end up returning a misleading error that webauthn auth failed.

Good point. Now that we have stable type names in our error json, I can detect this case and at least throw a more helpful error. We still navigate you to the "error" state of the sign in page in the email+password case, so I think that throwing the correct error should do the right thing.

@jaclarke ⚡ 64eaa1e (#6792)

jaclarke · 2024-02-21T10:33:58Z

edb/lib/ext/auth.edgeql

+                    select detached ext::auth::WebAuthnFactor
+                    filter .email = __new__.email
+                    and not .id = __new__.id
+                ).user_handle) ?? true,


Minor thing, but I think ?? true probably isn't needed. If the result of the select is an empty set, the assertion just won't run anyway.

TIL about the empty set behavior of assert. I'll add an example to the documentation for assert.

I think it just follows the normal set behaviour, where if the function arg isn't set of (like in count, for example) then it's expanded out. So assert({true, false}) is executed as {assert(true), assert(false)}, and similarly assert({}) becomes {}.

That makes sense. The documentation hints at this to since it says:

If the input bool is false, assert raises a QueryAssertionError. Otherwise, this function returns true. (emphasis mine)

I think it'd just be helpful to show some examples of calling this function with a sets to show that. I'll follow up.

jaclarke · 2024-02-21T10:40:24Z

edb/server/protocol/auth_ext/webauthn.py

-        assert len(user_handles) == 1
+        if len(user_handles) > 1:
+            raise errors.WebAuthnAuthenticationFailed(
+                "Multiple user handles found for the email."
+            )


Could still be an assert. It's really more of an ISE if there's multiple user handles, since the db should enforce that.

yeah, that's a good point. I'd want to still log a useful error message in the ISE, so I'll throw something together.

Empty set isn't evaluated by `assert`, so this passes when the select returns an empty set. And since `=` returns the empty set for comparison, the logic is the same.

scotttrinh force-pushed the webauthn-authentication branch from dd9fed5 to f6dd579 Compare February 7, 2024 15:42

scotttrinh force-pushed the webauthn-registration branch from 8c68aa2 to d404dab Compare February 7, 2024 20:21

scotttrinh force-pushed the webauthn-authentication branch 2 times, most recently from 3fa39a0 to 095b654 Compare February 8, 2024 16:43

Base automatically changed from webauthn-registration to master February 8, 2024 17:50

scotttrinh force-pushed the webauthn-authentication branch from 095b654 to 06109e0 Compare February 8, 2024 17:51

scotttrinh requested a review from jaclarke February 8, 2024 17:52

jaclarke reviewed Feb 12, 2024

View reviewed changes

scotttrinh requested a review from jaclarke February 12, 2024 17:09

scotttrinh force-pushed the webauthn-authentication branch 2 times, most recently from 00cd8cd to 04a063e Compare February 16, 2024 20:47

scotttrinh added 5 commits February 20, 2024 11:25

Add WebAuthn authentication flow

b38d60e

Remove extra b64 roundtrip

eddebaa

Allow WebAuthnFactors to share user_handle

52999d2

More useful error for missing email verification

1a63e08

Get WebAuthnFactor by exclusive properties

fc34ac4

scotttrinh force-pushed the webauthn-authentication branch from b3608e1 to 91c23fc Compare February 20, 2024 16:25

CQA

40d6362

scotttrinh force-pushed the webauthn-authentication branch from 91c23fc to 40d6362 Compare February 20, 2024 16:54

elprans approved these changes Feb 20, 2024

View reviewed changes

Use specific error for missing or invalid state

3dd2036

scotttrinh force-pushed the webauthn-authentication branch 3 times, most recently from 1f14960 to 60a5c24 Compare February 21, 2024 02:16

Every WebAuthnFactor email shares a user_handle

1431063

scotttrinh force-pushed the webauthn-authentication branch from 60a5c24 to 1431063 Compare February 21, 2024 02:16

jaclarke reviewed Feb 21, 2024

View reviewed changes

jaclarke approved these changes Feb 21, 2024

View reviewed changes

scotttrinh added 2 commits February 21, 2024 08:21

Remove unnecessary coalesce

644e869

Empty set isn't evaluated by `assert`, so this passes when the select returns an empty set. And since `=` returns the empty set for comparison, the logic is the same.

Make violating email_shares_user_handle trigger ISE

3f12556

scotttrinh merged commit bbec7fc into master Feb 21, 2024
23 checks passed

scotttrinh deleted the webauthn-authentication branch February 21, 2024 15:02

scotttrinh mentioned this pull request Feb 23, 2024

Allow ext::auth to register and authenticate with WebAuthn #6725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WebAuthn authentication flow #6792

Add WebAuthn authentication flow #6792

scotttrinh commented Feb 7, 2024

jaclarke Feb 12, 2024

scotttrinh Feb 12, 2024

scotttrinh Feb 12, 2024

jaclarke Feb 16, 2024

scotttrinh Feb 16, 2024

scotttrinh Feb 21, 2024

jaclarke Feb 12, 2024

scotttrinh Feb 12, 2024

scotttrinh Feb 12, 2024

jaclarke Feb 21, 2024

scotttrinh Feb 21, 2024

jaclarke Feb 21, 2024

scotttrinh Feb 21, 2024

jaclarke Feb 21, 2024

scotttrinh Feb 21, 2024

Add WebAuthn authentication flow #6792

Add WebAuthn authentication flow #6792

Conversation

scotttrinh commented Feb 7, 2024

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment