Bug 204024 - [WebAuthn] Add quirk needed to support legacy Google NFC Titan security keys
Summary: [WebAuthn] Add quirk needed to support legacy Google NFC Titan security keys
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 treegraph
 
Reported: 2019-11-08 14:37 PST by Jiewen Tan
Modified: 2019-11-19 16:39 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.58 KB, patch)
2019-11-08 14:46 PST, Jiewen Tan
bfulgham: review+
Details | Formatted Diff | Diff
Patch for landing (6.64 KB, patch)
2019-11-08 16:24 PST, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-11-08 14:37:48 PST
Add quirk needed to support legacy Google NFC Titan security keys.
Comment 1 Jiewen Tan 2019-11-08 14:38:19 PST
<rdar://problem/56962320>
Comment 2 Jiewen Tan 2019-11-08 14:46:17 PST
Created attachment 383167 [details]
Patch
Comment 3 Brent Fulgham 2019-11-08 15:07:43 PST
Comment on attachment 383167 [details]
Patch

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

r=me, but please run the autoreleasepool stuff by David before committing.

> Source/WebKit/ChangeLog:12
> +        of those legacy key.

actually speak U2F, indicating we are interacting with one of these legacy keys.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:52
> +{

Should the code in here be wrapped in @autoreleasepool?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:53
> +    auto versionData = [session transceive:adoptNS([[NSData alloc] initWithBytes:kCtapNfcAppletSelectionCommand length:sizeof(kCtapNfcAppletSelectionCommand)]).get()];

auto *versionData maybe?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:61
> +    // of those legacy key.

Same rephrasing I suggest above.

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:-71
> -    @autoreleasepool {

Why don't you need the @autoreleasepool block anymore? Did you check with David Kilzer to see if this is good to do?

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
> +    auto responseData = [m_session transceive:adoptNS([[NSData alloc] initWithBytes:data.data() length:data.size()]).get()];

auto *responseData =
Comment 4 Brent Fulgham 2019-11-08 15:08:01 PST
CCing David Kilzer to look at the @autoreleasepool stuff.
Comment 5 Jiewen Tan 2019-11-08 15:38:17 PST
Comment on attachment 383167 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/ChangeLog:12
>> +        of those legacy key.
> 
> actually speak U2F, indicating we are interacting with one of these legacy keys.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:52
>> +{
> 
> Should the code in here be wrapped in @autoreleasepool?

Before, I used [NSData dataWithBytes:data.data() length:data.size()] which is not wrapped with adoptNS. Now, I switched to adoptNS([[NSData alloc] initWithBytes:kCtapNfcAppletSelectionCommand length:sizeof(kCtapNfcAppletSelectionCommand)]).get(). It should be fine.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:53
>> +    auto versionData = [session transceive:adoptNS([[NSData alloc] initWithBytes:kCtapNfcAppletSelectionCommand length:sizeof(kCtapNfcAppletSelectionCommand)]).get()];
> 
> auto *versionData maybe?

Yes. Right.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:61
>> +    // of those legacy key.
> 
> Same rephrasing I suggest above.

Fixed.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:-71
>> -    @autoreleasepool {
> 
> Why don't you need the @autoreleasepool block anymore? Did you check with David Kilzer to see if this is good to do?

Same as above.

>> Source/WebKit/UIProcess/WebAuthentication/Cocoa/NfcConnection.mm:94
>> +    auto responseData = [m_session transceive:adoptNS([[NSData alloc] initWithBytes:data.data() length:data.size()]).get()];
> 
> auto *responseData =

Fixed.
Comment 6 Jiewen Tan 2019-11-08 16:24:46 PST
Created attachment 383177 [details]
Patch for landing
Comment 7 Jiewen Tan 2019-11-08 17:09:27 PST
Committed r252297: <https://trac.webkit.org/changeset/252297>