Bug 191535 – [WebAuthN] Support U2F HID Authenticators on macOS

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jiewen Tan]

Support U2F HID Authenticators on macOS.

[Radar WebKit Bug Importer]

<rdar://problem/47102027>

[Jiewen Tan]

Created attachment 358560 [details]
Patch

[Jiewen Tan]

Comment on attachment 358560 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidAuthenticator.cpp:1
> +/*

No changes to the file. Just renaming.

> Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidAuthenticator.h:1
> +/*

No changes to the file. Just renaming.

> Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidDriver.cpp:1
> +/*

No changes to the file. Just renaming.

> Source/WebKit/UIProcess/WebAuthentication/Fido/CtapHidDriver.h:1
> +/*

No changes to the file. Just renaming.

[Jiewen Tan]

Created attachment 358561 [details]
Patch

[Jiewen Tan]

Created attachment 358578 [details]
Patch

[Jiewen Tan]

It looks like that our svn system doesn't like case sensitive renaming for folders, i.e. fido => Fido. I will revert the renaming change then.

[Jiewen Tan]

Created attachment 358633 [details]
Patch

[Brent Fulgham]

Comment on attachment 358633 [details]
Patch

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

Looks good, but you may need to land it manually (iOS project file seems unhappy).

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:120
>      // FIXME(191535): Support U2F authenticators.

Do you still need this comment? If you do, I don't think the Bugzilla # is right! :-)

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:138
> +        if (m_requestMessage->cmd() == FidoHidDeviceCommand::kCbor) {

Are there other request messages we are supporting (besides kCbor)? If not, consider early return here.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:219
> +                message = FidoHidMessage::create(m_currentChannel, FidoHidDeviceCommand::kError, { static_cast<uint8_t>(CtapDeviceResponseCode::kCtap1ErrInvalidCommand) });

Does this cause a console message a developer can see? We might need good logging here to explain that we don't support older device protocols.

> Source/WebKit/UIProcess/WebAuthentication/fido/U2fHidAuthenticator.cpp:52
> +    // FIXME(191520): We need a way to convert std::unique_ptr to UniqueRef.

You should talk to David Kilzer about that one...

[Jiewen Tan]

Comment on attachment 358633 [details]
Patch

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

Thanks Brent for r+ this patch. I will figure out what's going on with iOS bots before landing this patch.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/HidService.mm:120
>>      // FIXME(191535): Support U2F authenticators.
> 
> Do you still need this comment? If you do, I don't think the Bugzilla # is right! :-)

Yup, we don't need that anymore.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:138
>> +        if (m_requestMessage->cmd() == FidoHidDeviceCommand::kCbor) {
> 
> Are there other request messages we are supporting (besides kCbor)? If not, consider early return here.

We anticipate other request messages as well. kMsg for U2F, and kCbor for CTAP. Therefore, we can't add early return.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockHidConnection.cpp:219
>> +                message = FidoHidMessage::create(m_currentChannel, FidoHidDeviceCommand::kError, { static_cast<uint8_t>(CtapDeviceResponseCode::kCtap1ErrInvalidCommand) });
> 
> Does this cause a console message a developer can see? We might need good logging here to explain that we don't support older device protocols.

Oh no, this is mocking a U2F authenticator's response to a kCbor(CTAP) msg command. After this patch, we will support both U2F and CTAP authenticators, and therefore we don't need to tell developers we don't support old protocols anymore. :)

>> Source/WebKit/UIProcess/WebAuthentication/fido/U2fHidAuthenticator.cpp:52
>> +    // FIXME(191520): We need a way to convert std::unique_ptr to UniqueRef.
> 
> You should talk to David Kilzer about that one...

Will be glad to.

[Jiewen Tan]

Created attachment 358644 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 358644 [details]
Patch for landing

Clearing flags on attachment: 358644

Committed r239752: <https://trac.webkit.org/changeset/239752>

[Truitt Savell]

The new test added in https://trac.webkit.org/changeset/239752/webkit

http/wpt/webauthn/public-key-credential-get-success-u2f.https.html

is a flakey timeout and failure. History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-u2f.https.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-u2f.https-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/wpt/webauthn/public-key-credential-get-success-u2f.https-actual.txt
@@ -1,5 +1,5 @@
 
 PASS PublicKeyCredential's [[get]] with minimum options in a mock hid authenticator. 
-PASS PublicKeyCredential's [[get]] with more allow credentials in a mock hid authenticator. 
+FAIL PublicKeyCredential's [[get]] with more allow credentials in a mock hid authenticator. promise_test: Unhandled rejection with value: object "NotAllowedError: Operation timed out."
 PASS PublicKeyCredential's [[get]] with test of user presence in a mock hid authenticator.

[Truitt Savell]

Additionally:

http/wpt/webauthn/public-key-credential-get-failure-u2f-silent.https.html Is a flaky timeout:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-failure-u2f-silent.https.html



http/wpt/webauthn/public-key-credential-create-success-u2f.https.html is a flaky failure:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-success-u2f.https.html

[Jiewen Tan]

(In reply to Truitt Savell from comment #13)
> Additionally:
> 
> http/wpt/webauthn/public-key-credential-get-failure-u2f-silent.https.html Is
> a flaky timeout:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> get-failure-u2f-silent.https.html
> 
> 
> 
> http/wpt/webauthn/public-key-credential-create-success-u2f.https.html is a
> flaky failure:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> create-success-u2f.https.html

Don't worry. They will be covered by Bug 192061.