Bug 188624 – [WebAuthn] Support NFC authenticators for iOS

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Support NFC authenticators for iOS.

[Radar WebKit Bug Importer]

<rdar://problem/43354214>

[Jiewen Tan]

Created attachment 376796 [details]
Patch

[Jiewen Tan]

Created attachment 376812 [details]
Patch

[Jiewen Tan]

Created attachment 376814 [details]
Patch

[Jiewen Tan]

Created attachment 376823 [details]
Patch

[Jiewen Tan]

Comment on attachment 376823 [details]
Patch

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

Will upload another patch without those two files after EWS runs.

> Source/WTF/wtf/Platform.h:1556
> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0

Oops. Somehow forgot to checkout this file.

> Source/WebKit/Platform/IPC/HandleMessage.h:116
> +//        ASSERT(decoder.isInvalid());

Oops. Somehow forgot to checkout this file.

[Jiewen Tan]

Comment on attachment 376823 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
> +        // FIXME: Somehow elminiate the need for the copy?

elminiate => eliminate

> LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html:12
> +    function checkResult(credential)

Should probably move this to util and share it between NFC and HID.

> LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html:38
> +    {

Should probably move this to util and share it between NFC and HID.

> LayoutTests/http/wpt/webauthn/public-key-credential-get-success-nfc.https.html:12
> +    {

Should probably move this to util and share it between NFC and HID.

> LayoutTests/http/wpt/webauthn/public-key-credential-get-success-nfc.https.html:29
> +    {

Should probably move this to util and share it between NFC and HID.

[Jiewen Tan]

Comment on attachment 376823 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:72
> +        respondAsync(WTFMove(callback), (Vector<uint8_t>(apduResponse->data())));

Remove the outer parenthesis of Vector<uint8_t>(apduResponse->data()).

[Brent Fulgham]

Comment on attachment 376823 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1556
>> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0
> 
> Oops. Somehow forgot to checkout this file.

LOl. r- to fix! :-)

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:74
> +        addResult = result.add(AuthenticatorTransport::Nfc);

Do we support NFC on Macs?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:44
> +// FIXME(200934)

"// FIXME(200934): Support NFCCTAP_GETRESPONSE"

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:84
> +    RunLoop::main().dispatch([callback = WTFMove(callback), response = WTFMove(response)] () mutable {

Please double-check that ResponseCallback (WTF::Function?) can be safely moved across threads without needing a cross-thread copy. I think if it's a WTF::Function and is constructed/destroyed on the same thread it's fine.

>> LayoutTests/http/wpt/webauthn/public-key-credential-create-success-nfc.https.html:12
>> +    function checkResult(credential)
> 
> Should probably move this to util and share it between NFC and HID.

Good idea!

[Jiewen Tan]

Comment on attachment 376823 [details]
Patch

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

Thanks Brent for reviewing the patch.

>>> Source/WTF/wtf/Platform.h:1556
>>> +#define HAVE_CTFONTCREATEFORCHARACTERSWITHLANGUAGEANDOPTION 0
>> 
>> Oops. Somehow forgot to checkout this file.
> 
> LOl. r- to fix! :-)

'webkit-patch upload ' tricks me every time for uploading uncommitted changes...

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:74
>> +        addResult = result.add(AuthenticatorTransport::Nfc);
> 
> Do we support NFC on Macs?

The framework is available but it doesn't do anything. I figure out it is better to have the NFHardwareManager to figure it out itself in runtime than a compile time flag .

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:44
>> +// FIXME(200934)
> 
> "// FIXME(200934): Support NFCCTAP_GETRESPONSE"

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:84
>> +    RunLoop::main().dispatch([callback = WTFMove(callback), response = WTFMove(response)] () mutable {
> 
> Please double-check that ResponseCallback (WTF::Function?) can be safely moved across threads without needing a cross-thread copy. I think if it's a WTF::Function and is constructed/destroyed on the same thread it's fine.

I didn't move the captured list objects across threads. I just make the response happen in the next runloop.

[Jiewen Tan]

Created attachment 376904 [details]
Patch

[Jiewen Tan]

Sadly, NearField doesn't present in iOS simulator. I will add a compile time flag for it.

Good news is my mocking tests work on macOS therefore the code is still covered by tests.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #12)
> Sadly, NearField doesn't present in iOS simulator. I will add a compile time
> flag for it.
> 
> Good news is my mocking tests work on macOS therefore the code is still
> covered by tests.

The easiest fix I guess is to skip those tests.

[Jiewen Tan]

Created attachment 376960 [details]
Patch

[Alex Christensen]

Comment on attachment 376960 [details]
Patch

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

> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:70
> +@property (nonatomic, readonly, copy) NSData *AppData NS_DEPRECATED(10_12, 10_15, 10_0, 13_0);

Do we need to declare this deprecated property?

> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:76
> +        auto addResult = result.add(AuthenticatorTransport::Nfc);

I'd put this outside the #if guard and call it something like nfcAddResult, or add it before Usb

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {

You could do session = retainPtr(session).
Also move weakThis into the lambda capture.
Just reducing a little ref churn.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.mm:53
> +    RunLoop::main().dispatch([connection = _connection, tags = retainTags] {

ditto about tags

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:52
> +        command.setCla(0x80);
> +        command.setIns(0x10);

Could these be given names and put in FidoConstants.h?

[Jiewen Tan]

Comment on attachment 376960 [details]
Patch

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

Thanks Alex for reviewing the patch.

>> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:70
>> +@property (nonatomic, readonly, copy) NSData *AppData NS_DEPRECATED(10_12, 10_15, 10_0, 13_0);
> 
> Do we need to declare this deprecated property?

I guess not. I have to conform the protocol in MockNfcService to do mock testing.

>> Source/WebKit/UIProcess/WebAuthentication/AuthenticatorManager.cpp:76
>> +        auto addResult = result.add(AuthenticatorTransport::Nfc);
> 
> I'd put this outside the #if guard and call it something like nfcAddResult, or add it before Usb

Fixed. I add it before USB.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {
> 
> You could do session = retainPtr(session).
> Also move weakThis into the lambda capture.
> Just reducing a little ref churn.

Fixed the retainPtr(session).

What do you mean by moving weakThis into the lambda capture which it is already in?

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.mm:53
>> +    RunLoop::main().dispatch([connection = _connection, tags = retainTags] {
> 
> ditto about tags

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:52
>> +        command.setIns(0x10);
> 
> Could these be given names and put in FidoConstants.h?

Definitely, yes.

Fixed.

[Jiewen Tan]

Comment on attachment 376960 [details]
Patch

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

>>> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:70
>>> +@property (nonatomic, readonly, copy) NSData *AppData NS_DEPRECATED(10_12, 10_15, 10_0, 13_0);
>> 
>> Do we need to declare this deprecated property?
> 
> I guess not. I have to conform the protocol in MockNfcService to do mock testing.

What I mean is we need to.

[Alex Christensen]

(In reply to Jiewen Tan from comment #16)
> What do you mean by moving weakThis into the lambda capture which it is
> already in?
weakThis = WTFMove(weakThis)
This kind of pattern prevents one ref and one deref of the WeakPtrImpl

[Jiewen Tan]

(In reply to Alex Christensen from comment #18)
> (In reply to Jiewen Tan from comment #16)
> > What do you mean by moving weakThis into the lambda capture which it is
> > already in?
> weakThis = WTFMove(weakThis)
> This kind of pattern prevents one ref and one deref of the WeakPtrImpl

I was not aware that WeakPtr uses a RefPtr to wrap the WeakPtrImpl. Thanks for the tips.

[Chris Dumez]

Comment on attachment 376960 [details]
Patch

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

r=me with changes

> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:85
> +- (void) endSession;

Why the space before endSession? Same comment for several other below.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:682
> +        WebKit::MockWebAuthenticationConfiguration::Nfc nfc;

Why is this local needed? Why not use configuration.nfc directly below?

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:687
> +        if (error == "no-tags")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:689
> +        if (error == "wrong-tag-type")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:691
> +        if (error == "no-connections")

else if

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:693
> +        if (error == "malicious-payload")

else if

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:79
> +    // FIXME(200932)

Please put in an actual comment, not just a number.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
> +        if (!m_service)

Why is this in the loop and not at the beginning of the method? Can m_service change while iterating?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.h:50
> +    std::unique_ptr<CtapNfcDriver> m_driver;

We normally try to avoid protected data members. We put data members private and have protected getters / setters for them if needed.

>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>>> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {
>> 
>> You could do session = retainPtr(session).
>> Also move weakThis into the lambda capture.
>> Just reducing a little ref churn.
> 
> Fixed the retainPtr(session).
> 
> What do you mean by moving weakThis into the lambda capture which it is already in?

weakThis = WTFMove(weakThis)

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.h:31
> +#import <wtf/WeakPtr.h>

Does not appear to be needed in the header.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:98
> +using Mock = MockWebAuthenticationConfiguration::Nfc;

Mock seems too generic a name, especially now that we have unified builds.

> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:146
> +    return result;

Should this autorelease ?

> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
> +        // FIXME: Somehow eliminate the need for the copy?

You could add the following getter to ApduResponse:
Vector<uint8_t>& data() { return m_data; }

and then WTFMove(apduResponse->data()) below.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:56
> +        weakThis->continueAfterGetInfo(ptr, WTFMove(response));

what guarantees that ptr is not stale here? Also please find a better name.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:62
> +void FidoService::continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& response)

ptr is not a good name.

> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.h:45
> +    void continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& info);

ptr should be omitted.

[Jiewen Tan]

Comment on attachment 376960 [details]
Patch

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

Thanks Chris for r+ this patch.

>> Source/WebKit/Platform/spi/Cocoa/NearFieldSPI.h:85
>> +- (void) endSession;
> 
> Why the space before endSession? Same comment for several other below.

Good catch. I copy & paste it from the actual header...

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:682
>> +        WebKit::MockWebAuthenticationConfiguration::Nfc nfc;
> 
> Why is this local needed? Why not use configuration.nfc directly below?

configuration.nfc is an Optional. I guess I cannot directly use it?

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:687
>> +        if (error == "no-tags")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:689
>> +        if (error == "wrong-tag-type")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:691
>> +        if (error == "no-connections")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:693
>> +        if (error == "malicious-payload")
> 
> else if

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:79
>> +    // FIXME(200932)
> 
> Please put in an actual comment, not just a number.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
>> +        if (!m_service)
> 
> Why is this in the loop and not at the beginning of the method? Can m_service change while iterating?

I should check it ahead.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.h:50
>> +    std::unique_ptr<CtapNfcDriver> m_driver;
> 
> We normally try to avoid protected data members. We put data members private and have protected getters / setters for them if needed.

Fixed.

>>>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcService.mm:78
>>>> +        RunLoop::main().dispatch([weakThis, this, session = retainSession] () mutable {
>>> 
>>> You could do session = retainPtr(session).
>>> Also move weakThis into the lambda capture.
>>> Just reducing a little ref churn.
>> 
>> Fixed the retainPtr(session).
>> 
>> What do you mean by moving weakThis into the lambda capture which it is already in?
> 
> weakThis = WTFMove(weakThis)

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/WKNFReaderSessionDelegate.h:31
>> +#import <wtf/WeakPtr.h>
> 
> Does not appear to be needed in the header.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:98
>> +using Mock = MockWebAuthenticationConfiguration::Nfc;
> 
> Mock seems too generic a name, especially now that we have unified builds.

Changed to MockNfc.

>> Source/WebKit/UIProcess/WebAuthentication/Mock/MockNfcService.mm:146
>> +    return result;
> 
> Should this autorelease ?

Maybe?

>> Source/WebKit/UIProcess/WebAuthentication/fido/CtapNfcDriver.cpp:71
>> +        // FIXME: Somehow eliminate the need for the copy?
> 
> You could add the following getter to ApduResponse:
> Vector<uint8_t>& data() { return m_data; }
> 
> and then WTFMove(apduResponse->data()) below.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:56
>> +        weakThis->continueAfterGetInfo(ptr, WTFMove(response));
> 
> what guarantees that ptr is not stale here? Also please find a better name.

The ptr is the key to the hashset. Therefore, it doesn't matter whether it is stale or not.

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.cpp:62
>> +void FidoService::continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& response)
> 
> ptr is not a good name.

id?

>> Source/WebKit/UIProcess/WebAuthentication/fido/FidoService.h:45
>> +    void continueAfterGetInfo(CtapDriver* ptr, Vector<uint8_t>&& info);
> 
> ptr should be omitted.

Fixed.

[Jiewen Tan]

Created attachment 377048 [details]
Patch for Landing

[EWS Watchlist]

Attachment 377048 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webauthn/apdu/ApduResponse.h:64:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 56 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jiewen Tan]

Committed r249059: <https://trac.webkit.org/changeset/249059>

[Jiewen Tan]

A build fix:
Committed r249068: <https://trac.webkit.org/changeset/249068>

[Truitt Savell]

It looks like two of the new tests added in https://trac.webkit.org/changeset/249059/webkit are failing 

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

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-create-success-nfc.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-get-success-nfc.https.html

Diff:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249096%20(6157)/http/wpt/webauthn/public-key-credential-create-success-nfc.https-diff.txt

https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249096%20(6157)/http/wpt/webauthn/public-key-credential-get-success-nfc.https-diff.txt

[Brent Fulgham]

(In reply to Truitt Savell from comment #26)
> It looks like two of the new tests added in
> https://trac.webkit.org/changeset/249059/webkit are failing 
> 
> http/wpt/webauthn/public-key-credential-create-success-nfc.https.html
> http/wpt/webauthn/public-key-credential-get-success-nfc.https.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> create-success-nfc.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-
> credential-get-success-nfc.https.html
> 
> Diff:
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249096%20(6157)/http/wpt/webauthn/public-key-credential-create-success-nfc.
> https-diff.txt
> 
> https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> r249096%20(6157)/http/wpt/webauthn/public-key-credential-get-success-nfc.
> https-diff.txt

They might need to be skipped on older OS's.

[Jiewen Tan]

(In reply to Brent Fulgham from comment #27)
> (In reply to Truitt Savell from comment #26)
> > It looks like two of the new tests added in
> > https://trac.webkit.org/changeset/249059/webkit are failing 
> > 
> > http/wpt/webauthn/public-key-credential-create-success-nfc.https.html
> > http/wpt/webauthn/public-key-credential-get-success-nfc.https.html
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=http%2Fwpt%2Fwebauthn%2Fpublic-key-credential-
> > create-success-nfc.https.html%20http%2Fwpt%2Fwebauthn%2Fpublic-key-
> > credential-get-success-nfc.https.html
> > 
> > Diff:
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r249096%20(6157)/http/wpt/webauthn/public-key-credential-create-success-nfc.
> > https-diff.txt
> > 
> > https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > r249096%20(6157)/http/wpt/webauthn/public-key-credential-get-success-nfc.
> > https-diff.txt
> 
> They might need to be skipped on older OS's.

Yes, here we go:
Committed r249107: <https://trac.webkit.org/changeset/249107>