Bug 197974 - [WebAuthN] Allow authenticators that support both CTAP and U2F to try U2F if CTAP fails in authenticatorGetAssertion
Summary: [WebAuthN] Allow authenticators that support both CTAP and U2F to try U2F if ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 181943
  Show dependency tree / graph
 
Reported: 2019-05-16 20:21 PDT by Jiewen Tan
Modified: 2019-05-18 17:42 PDT (History)
4 users (show)

See Also:

Attachments
Patch (28.44 KB, patch)
2019-05-16 20:43 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (28.20 KB, patch)
2019-05-17 15:36 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (27.48 KB, patch)
2019-05-17 15:45 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-05-16 20:21:05 PDT 
Allow authenticators that support both CTAP and U2F to try U2F if CTAP fails in authenticatorGetAssertion.
Comment 1 Radar WebKit Bug Importer 2019-05-16 20:24:06 PDT 
<rdar://problem/50879746>
Comment 2 Radar WebKit Bug Importer 2019-05-16 20:24:07 PDT Comment hidden (obsolete)
Comment 3 Jiewen Tan 2019-05-16 20:43:07 PDT 
Created attachment 370100 [details]
Patch
Comment 4 Jiewen Tan 2019-05-16 22:34:09 PDT 
Comment on attachment 370100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370100&action=review

> Source/WebKit/ChangeLog:16
> +        to ask a potential U2F credential once a kCtap2ErrInvalidCredential error is returned regarding to the first CTAP request,

I should probably relax all conditions to always downgrade to U2F whenever there is no responses to the first CTAP request.
Comment 5 Jiewen Tan 2019-05-17 00:57:20 PDT 
Comment on attachment 370100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370100&action=review

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp:93
> +        auto error = getResponseCode(data);

I should probably relax all conditions to always downgrade to U2F whenever there is no responses to the first CTAP request.
Comment 6 Jiewen Tan 2019-05-17 15:24:47 PDT 
Comment on attachment 370100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370100&action=review

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapHidAuthenticator.cpp:93
>> +        auto error = getResponseCode(data);
> 
> I should probably relax all conditions to always downgrade to U2F whenever there is no responses to the first CTAP request.

The reason behinds this decision is that CTAP2_ERR_NO_CREDENTIALS is recommended in the spec: https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#authenticatorGetAssertion. On contrary, I get CTAP2_ERR_INVALID_CREDENTIAL from some keys. Therefore, we should probably not check any error.

Also, bad user agents might somehow prefer U2F over CTAP, and therefore relaying parties might not be aware of it and supply an appid to indicate a U2F credential should be used instead. Therefore, we should omit that check as well.
Comment 7 Jiewen Tan 2019-05-17 15:36:25 PDT 
Created attachment 370163 [details]
Patch
Comment 8 Brent Fulgham 2019-05-17 15:42:25 PDT 
Comment on attachment 370163 [details]
Patch

Looks good. r=me
Comment 9 Jiewen Tan 2019-05-17 15:45:42 PDT 
Created attachment 370165 [details]
Patch
Comment 10 Jiewen Tan 2019-05-17 15:47:15 PDT 
(In reply to Brent Fulgham from comment #8)
> Comment on attachment 370163 [details]
> Patch
> 
> Looks good. r=me

Sorry, Brent. I modified a test and uploaded a new patch which obsolete your r+ flag. Could you r+ it again?
Comment 11 Brent Fulgham 2019-05-17 17:13:18 PDT 
Comment on attachment 370165 [details]
Patch

R=me
Comment 12 Jiewen Tan 2019-05-18 17:15:35 PDT 
(In reply to Brent Fulgham from comment #11)
> Comment on attachment 370165 [details]
> Patch
> 
> R=me

Thanks Brent for r+ this patch.
Comment 13 WebKit Commit Bot 2019-05-18 17:42:22 PDT 
Comment on attachment 370165 [details]
Patch

Clearing flags on attachment: 370165

Committed r245500: <https://trac.webkit.org/changeset/245500>
Comment 14 WebKit Commit Bot 2019-05-18 17:42:24 PDT 
All reviewed patches have been landed.  Closing bug.