Bug 183527 - [WebAuthN] Implement authenticatorMakeCredential
Summary: [WebAuthN] Implement authenticatorMakeCredential
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on: 183587
Blocks: 181943
  Show dependency tree / graph
 
Reported: 2018-03-09 14:59 PST by Jiewen Tan
Modified: 2018-04-17 10:55 PDT (History)
9 users (show)

See Also:

Attachments
Patch (142.30 KB, patch)
2018-03-09 19:29 PST, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (123.97 KB, patch)
2018-03-11 03:36 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (122.65 KB, patch)
2018-03-11 12:47 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (122.66 KB, patch)
2018-03-11 15:24 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (122.69 KB, patch)
2018-03-11 15:44 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.59 MB, application/zip)
2018-03-11 16:53 PDT, EWS Watchlist 		no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.17 MB, application/zip)
2018-03-11 16:59 PDT, EWS Watchlist 		no flags Details
Patch (123.75 KB, patch)
2018-03-11 20:01 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (115.88 KB, patch)
2018-03-13 02:15 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (136.16 KB, patch)
2018-03-15 13:53 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (134.66 KB, patch)
2018-03-15 14:05 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (134.63 KB, patch)
2018-03-15 14:42 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (137.06 KB, patch)
2018-03-15 16:16 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (137.75 KB, patch)
2018-03-15 17:03 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Patch (134.36 KB, patch)
2018-03-16 17:18 PDT, Jiewen Tan 		bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (133.66 KB, patch)
2018-03-17 12:28 PDT, Jiewen Tan 		no flags Details | Formatted Diff | Diff
Show Obsolete (14) 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 2018-03-09 14:59:36 PST 
Implement authenticatorMakeCredential with self attestation. See: https://www.w3.org/TR/webauthn/#op-make-cred.
Comment 1 Jiewen Tan 2018-03-09 15:00:07 PST 
<rdar://problem/35275886>
Comment 2 Radar WebKit Bug Importer 2018-03-09 15:00:41 PST Comment hidden (obsolete)
Comment 3 Jiewen Tan 2018-03-09 18:18:39 PST 
Please refer to the radar for a more detailed explanation of the KeyChain Schema and the tentative Apple Attestation Format.
Comment 4 Jiewen Tan 2018-03-09 18:20:46 PST 
Noted, the coming patch could be a little bit large. Be sure to read the detailed explanation in the radar before reviewing. Sorry for the inconvenience.
Comment 5 Jiewen Tan 2018-03-09 18:22:42 PST 
This patch won’t fullfill all the Apple Attestation Format yet as it only implements a self-signed attestation in order to do auto test. WebKit testing enviroment restircts network access which is required by any Privacy CA attestation format.
Comment 6 Jiewen Tan 2018-03-09 19:29:32 PST 
Created attachment 335495 [details]
Patch
Comment 7 Brent Fulgham 2018-03-10 12:16:48 PST 
Comment on attachment 335495 [details]
Patch

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

It looks like this is a combination of two separate patches. Can you please correct the patch so we can do a clean review of each separately?

> Source/WebCore/ChangeLog:15
> +        1. An helper class called LocalAuthenticator is crafted to represent Apple platform attached authenticator, i.e.

"A helper class"

> Source/WebCore/ChangeLog:17
> +        supports, such as different Keychain API capabilities and different attestation frameworks.

"lacks some fundamental support, such as"

> Source/WebCore/ChangeLog:78
> +        <rdar://problem/36459988>

Looks like you got some extra ChangeLog data in the file!

> Source/WebKit/ChangeLog:30
> +        <rdar://problem/36459988>

Ditto!
Comment 8 Jiewen Tan 2018-03-11 01:10:08 PST 
Comment on attachment 335495 [details]
Patch

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

Not sure how the uploaded version of the patch is mixed with the previous one. Maybe I developed in a branch that bases on an unmerged version of the previous patch. Will upload a clear patch soon. Thanks Brent for pointing it out.

>> Source/WebCore/ChangeLog:15
>> +        1. An helper class called LocalAuthenticator is crafted to represent Apple platform attached authenticator, i.e.
> 
> "A helper class"

Fixed.

>> Source/WebCore/ChangeLog:17
>> +        supports, such as different Keychain API capabilities and different attestation frameworks.
> 
> "lacks some fundamental support, such as"

Fixed.

>> Source/WebCore/ChangeLog:78
>> +        <rdar://problem/36459988>
> 
> Looks like you got some extra ChangeLog data in the file!

Oops. Cleaned up.

>> Source/WebKit/ChangeLog:30
>> +        <rdar://problem/36459988>
> 
> Ditto!

Cleaned up.
Comment 9 Jiewen Tan 2018-03-11 03:36:27 PDT 
Created attachment 335534 [details]
Patch
Comment 10 EWS Watchlist 2018-03-11 03:38:16 PDT Comment hidden (obsolete)
Comment 11 Jiewen Tan 2018-03-11 12:47:41 PDT 
Created attachment 335541 [details]
Patch
Comment 12 EWS Watchlist 2018-03-11 12:50:44 PDT Comment hidden (obsolete)
Comment 13 Jiewen Tan 2018-03-11 15:24:42 PDT 
Created attachment 335549 [details]
Patch
Comment 14 EWS Watchlist 2018-03-11 15:27:42 PDT Comment hidden (obsolete)
Comment 15 Jiewen Tan 2018-03-11 15:44:48 PDT 
Created attachment 335551 [details]
Patch
Comment 16 EWS Watchlist 2018-03-11 15:47:12 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-03-11 16:53:41 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-03-11 16:53:42 PDT Comment hidden (obsolete)
Comment 19 EWS Watchlist 2018-03-11 16:59:29 PDT Comment hidden (obsolete)
Comment 20 EWS Watchlist 2018-03-11 16:59:30 PDT Comment hidden (obsolete)
Comment 21 Jiewen Tan 2018-03-11 20:01:18 PDT 
Created attachment 335569 [details]
Patch
Comment 22 EWS Watchlist 2018-03-11 20:03:44 PDT Comment hidden (obsolete)
Comment 23 Jiewen Tan 2018-03-12 18:49:50 PDT 
Will split this patch into two such that the soft linking part can be landed sooner.
Comment 24 Jiewen Tan 2018-03-13 02:15:51 PDT 
Created attachment 335684 [details]
Patch
Comment 25 EWS Watchlist 2018-03-13 02:19:02 PDT Comment hidden (obsolete)
Comment 26 lsykora 2018-03-14 03:01:29 PDT 
Comment on attachment 335684 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        support, such as different Keychain API capabilities and different attestation frameworks.

I believe that the iOS and macOS should be quite in sync. When you have a macOS with Biometry you should be able to use the same LocalAuthentication and SecKey frameworks.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:108
> +    auto context = adoptNS([allocLAContextInstance() init]);

In case that something else than the default UI will be needed then the caller will have to use a special entitlement. I'm not sure if it is a problem here as DeviceIdentity need entitlements too.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:150
> +        issueClientCertificate(options.rp.id, options.user.name, hash, ^(_Nullable SecKeyRef privateKey, NSArray * _Nullable, NSError * _Nullable error) {

In the real code the generated key would have to get also ACL using the SecAccessControlRef to protect access to the key by biometry. If only single type of ACL will be assigned it can be probably hidden in the issueClientCertificate call.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:305
> +                    CFDataRef signatureRef = SecKeyCreateSignature(privateKey, kSecKeyAlgorithmECDSASignatureMessageX962SHA256, (__bridge CFDataRef)[NSData dataWithBytes:authData.data() length:authData.size()], &errorRef);

As the key should be protected by the biometric ACL this call will produce a TouchID prompt. You can probably use it also as the consent instead of the AP controlled authentication using the LocalAuthentication API. You can use LA here too and pass wht LAContext but then you should use the evaluateACL based on the actual key ACL.
Comment 27 Jiewen Tan 2018-03-15 12:29:14 PDT 
Comment on attachment 335684 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +        support, such as different Keychain API capabilities and different attestation frameworks.
> 
> I believe that the iOS and macOS should be quite in sync. When you have a macOS with Biometry you should be able to use the same LocalAuthentication and SecKey frameworks.

I was told by Pavel after this patch. Will update the change log to only mention attestation framework.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:108
>> +    auto context = adoptNS([allocLAContextInstance() init]);
> 
> In case that something else than the default UI will be needed then the caller will have to use a special entitlement. I'm not sure if it is a problem here as DeviceIdentity need entitlements too.

We don't have problems with entitlements as those will be added to our clients.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:150
>> +        issueClientCertificate(options.rp.id, options.user.name, hash, ^(_Nullable SecKeyRef privateKey, NSArray * _Nullable, NSError * _Nullable error) {
> 
> In the real code the generated key would have to get also ACL using the SecAccessControlRef to protect access to the key by biometry. If only single type of ACL will be assigned it can be probably hidden in the issueClientCertificate call.

I figured out that it is better for me to upload a full patch that combines this one and real Apple attestation such that it is logically easier to review.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:305
>> +                    CFDataRef signatureRef = SecKeyCreateSignature(privateKey, kSecKeyAlgorithmECDSASignatureMessageX962SHA256, (__bridge CFDataRef)[NSData dataWithBytes:authData.data() length:authData.size()], &errorRef);
> 
> As the key should be protected by the biometric ACL this call will produce a TouchID prompt. You can probably use it also as the consent instead of the AP controlled authentication using the LocalAuthentication API. You can use LA here too and pass wht LAContext but then you should use the evaluateACL based on the actual key ACL.

How could I avoid the TouchID prompt here? I didn't see any documentation that SecKeyCreateSignature can accepts a LAContext.
Comment 28 Jiewen Tan 2018-03-15 13:53:54 PDT 
Created attachment 335877 [details]
Patch
Comment 29 Jiewen Tan 2018-03-15 14:05:47 PDT 
Created attachment 335880 [details]
Patch
Comment 30 EWS Watchlist 2018-03-15 14:08:37 PDT Comment hidden (obsolete)
Comment 31 Jiewen Tan 2018-03-15 14:33:42 PDT 
Comment on attachment 335880 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        1. A helper class called LocalAuthenticator is crafted to represent Apple platform attached authenticator, i.e.

This item is rephrased as:
A helper class called LocalAuthenticator is crafted to represent Apple platform attached authenticator, i.e. the devices themselves. All operations are currently restricted to iOS at this moment as macOS lacks attestation support.
Comment 32 Jiewen Tan 2018-03-15 14:42:08 PDT 
Created attachment 335888 [details]
Patch
Comment 33 EWS Watchlist 2018-03-15 14:45:04 PDT Comment hidden (obsolete)
Comment 34 Jiewen Tan 2018-03-15 16:16:53 PDT 
Created attachment 335905 [details]
Patch
Comment 35 EWS Watchlist 2018-03-15 16:20:33 PDT Comment hidden (obsolete)
Comment 36 mitz 2018-03-15 16:21:45 PDT 
Comment on attachment 335888 [details]
Patch

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

> Source/WebCore/Configurations/WebCore.xcconfig:141
> +WK_HAVE_DEVICE_IDENTITY[sdk=iphoneos*] = YES;

We shouldn’t be using [sdk=] conditionals anymore. See the recent work done to get rid of them in FeatureDefines.xcconfig.
Comment 37 Jiewen Tan 2018-03-15 16:56:14 PDT 
(In reply to mitz from comment #36)
> Comment on attachment 335888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335888&action=review
> 
> > Source/WebCore/Configurations/WebCore.xcconfig:141
> > +WK_HAVE_DEVICE_IDENTITY[sdk=iphoneos*] = YES;
> 
> We shouldn’t be using [sdk=] conditionals anymore. See the recent work done
> to get rid of them in FeatureDefines.xcconfig.

Fixed.
Comment 38 Jiewen Tan 2018-03-15 17:03:47 PDT 
Created attachment 335911 [details]
Patch
Comment 39 EWS Watchlist 2018-03-15 17:05:55 PDT Comment hidden (obsolete)
Comment 40 Jiewen Tan 2018-03-15 19:06:22 PDT 
Comment on attachment 335911 [details]
Patch

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

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:95
> +    for (auto& excludeCredential : options.excludeCredentials) {

Oops. The following algorithms is not very efficient. Since the following is basically finding an intersection between two unsorted arrays, I will rewrite it in the next patch with the O((m+n)min(logn, logm)) algorithm.

authenticatorGetAssertion needs to find the intersection as well. Therefore, I will encapsulate the algorithm into a function.
Comment 41 Jiewen Tan 2018-03-15 20:43:00 PDT 
(In reply to Jiewen Tan from comment #40)
> Comment on attachment 335911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335911&action=review
> 
> > Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:95
> > +    for (auto& excludeCredential : options.excludeCredentials) {
> 
> Oops. The following algorithms is not very efficient. Since the following is
> basically finding an intersection between two unsorted arrays, I will
> rewrite it in the next patch with the O((m+n)min(logn, logm)) algorithm.
> 
> authenticatorGetAssertion needs to find the intersection as well. Therefore,
> I will encapsulate the algorithm into a function.

Oops. A hash set should do.
Comment 42 Jiewen Tan 2018-03-16 01:21:31 PDT 
Comment on attachment 335911 [details]
Patch

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

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:44
> +struct WEBCORE_EXPORT CreationReturnBundle {

Removed WEBCORE_EXPORT as it is unnecessary.

> Source/WebCore/Modules/credentialmanagement/CredentialsMessenger.h:54
> +struct WEBCORE_EXPORT AssertionReturnBundle {

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:38
> +    struct WEBCORE_EXPORT Entity {

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:43
> +    struct WEBCORE_EXPORT RpEntity : public Entity {

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:49
> +        WEBCORE_EXPORT Vector<uint8_t> idVector;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:50
> +        WEBCORE_EXPORT String displayName;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:54
> +        WEBCORE_EXPORT PublicKeyCredentialType type;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:55
> +        WEBCORE_EXPORT int64_t alg;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:61
> +    WEBCORE_EXPORT RpEntity rp;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:62
> +    WEBCORE_EXPORT UserEntity user;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:65
> +    WEBCORE_EXPORT Vector<Parameters> pubKeyCredParams;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:68
> +    WEBCORE_EXPORT Vector<PublicKeyCredentialDescriptor> excludeCredentials;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialDescriptor.h:43
> +    WEBCORE_EXPORT PublicKeyCredentialType type;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialDescriptor.h:45
> +    WEBCORE_EXPORT Vector<uint8_t> idVector;

Ditto.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialDescriptor.h:46
> +    WEBCORE_EXPORT Vector<AuthenticatorTransport> transports;

Ditto.

> WebKitLibraries/WebKitPrivateFrameworkStubs/iOS/10/DeviceIdentity.framework/DeviceIdentity.tbd:1
> +--- !tapi-tbd-v2

Removed as we don't have to support iOS 10 anymore.
Comment 43 Brent Fulgham 2018-03-16 13:51:13 PDT 
Comment on attachment 335911 [details]
Patch

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

I think this looks pretty good, but there are a few things that need to be tweaked before it's ready. r- to fix the two crashes I mentioned, and to consider whether CrossThreadTask/CrossThreadCopy resolves your issues.

> Source/WebCore/ChangeLog:21
> +        the WebAuthN API, and thus it is moved to WebCore to perform unit tesing flavor API tests. This is not enoguh as it

... "This is not enoUGh as it" (you transposed two characters).

> Source/WebCore/ChangeLog:22
> +        can't test message exchange between the UI and Web processes. We will address this issue later.

By later here, do you mean in a follow-up patch? I think you should just say that: "We will address this in a subsequent patch."

> Source/WebCore/ChangeLog:23
> +        3. More on testing. The attestation process is abstracted into a protected method such that the testing enviroment can

I would use a colon here: "More on testing: The attestation ..."

> Source/WebCore/ChangeLog:24
> +        override it with self attestation as network access is restricted in WebKit testing enviroment. Also, swizzlers of

is restricted in THE WebKit testing environment.

> Source/WebCore/ChangeLog:26
> +        4. More on testing. The actual Apple attestation can only happen in real device and with network access, Therefore

Ditto on colon use. "More on testing: The actual Apple ...".

'Therefore' should not be capitalized in the middle of a sentence.

> Source/WebCore/ChangeLog:30
> +                This method is the one does all the magic.

I'd move this text to the same line as "makeCredential():"

> Source/WebCore/ChangeLog:38
> +        6. Even though files are of .mm format, they are written in a way that mix NS, CF and C++ types. Here is the rule:

in a way that MIXES NS, CF, ...

> Source/WebCore/ChangeLog:47
> +        (WebCore::getIdFromAttestationObject): Deleted.

Can you say why it's okay to delete this ? Do we now use DeviceIdentity framework to support the same task?

> Source/WebCore/Modules/webauthn/COSEConstants.h:29
> +

I suggest a comment:

// See RFC 8152 - CBOR Object Signing and Encryption <https://tools.ietf.org/html/rfc8152>

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:74
> +    // It only copies other's rp and user, which are used in another thread.

I don't understand this comment. It seems like you are warning people not to use it, perhaps because it is incomplete or lacking features you need for the future.

If so, I would put a FIXME() comment here indicating that it will be revised in a subsequent patch.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:48
> +const uint8_t authenticatorDataFlags = 69;

I don't understand what this is, or why it has the value 69, but I assume it's correct.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:134
> +    auto* localHash = new Vector<uint8_t>(hash);

Can you use CrossThreadTask for this? Or at least use CrossThreadCopy to get the data across?

Is this outer function operating on Main? I suggest you ASSERT(isMainThread()) outside the closure for 'evalutePolicy'.

Also: This is Objective C++, so you should do the asterisks backwards:

auto *localCallback
auto *localException...
etc.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:136
> +    [context evaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics localizedReason:reason reply:^(BOOL success, NSError *error) {

... and ASSERT(!isMainThread()) in here.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:163
> +                LOG_ERROR("Couldn't attestate: %@", error);

"Couldn't attest: %@"

> Source/WebKit/UIProcess/CredentialManagement/WebCredentialsMessengerProxy.cpp:56
> +        exceptionReply(messageId, { WebCore::NotAllowedError, ASCIILiteral("No avaliable authenticators.") });

Shouldn't you early return here? You go on and dereference m_authenticator later.

> Source/WebKit/UIProcess/CredentialManagement/WebCredentialsMessengerProxy.cpp:78
> +    isUserVerifyingPlatformAuthenticatorAvailableReply(messageId, m_authenticator->isAvailable());

Shouldn't you have an else here? If m_authenticator was just false, you are going to crash here!
Comment 44 Chris Dumez 2018-03-16 14:40:09 PDT 
Comment on attachment 335911 [details]
Patch

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

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:75
> +    PublicKeyCredentialCreationOptions* isolatedPartialCopyPtr() const

Why does this return a raw pointer? Does PublicKeyCredentialCreationOptions have a (implicit) move-constructor?

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.h:47
> +    explicit LocalAuthenticator() = default;

Why is this needed?

> Tools/ChangeLog:23
> +2018-03-11  Jiewen Tan  <jiewen_tan@apple.com>

Double changelog.
Comment 45 Jiewen Tan 2018-03-16 14:43:14 PDT 
Comment on attachment 335911 [details]
Patch

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

Thanks Brent for reviewing the patch.

>> Source/WebCore/ChangeLog:21
>> +        the WebAuthN API, and thus it is moved to WebCore to perform unit tesing flavor API tests. This is not enoguh as it
> 
> ... "This is not enoUGh as it" (you transposed two characters).

Fixed.

>> Source/WebCore/ChangeLog:22
>> +        can't test message exchange between the UI and Web processes. We will address this issue later.
> 
> By later here, do you mean in a follow-up patch? I think you should just say that: "We will address this in a subsequent patch."

Fixed.

>> Source/WebCore/ChangeLog:23
>> +        3. More on testing. The attestation process is abstracted into a protected method such that the testing enviroment can
> 
> I would use a colon here: "More on testing: The attestation ..."

Fixed.

>> Source/WebCore/ChangeLog:24
>> +        override it with self attestation as network access is restricted in WebKit testing enviroment. Also, swizzlers of
> 
> is restricted in THE WebKit testing environment.

Fixed.

>> Source/WebCore/ChangeLog:26
>> +        4. More on testing. The actual Apple attestation can only happen in real device and with network access, Therefore
> 
> Ditto on colon use. "More on testing: The actual Apple ...".
> 
> 'Therefore' should not be capitalized in the middle of a sentence.

Fixed.

>> Source/WebCore/ChangeLog:30
>> +                This method is the one does all the magic.
> 
> I'd move this text to the same line as "makeCredential():"

Fixed.

>> Source/WebCore/ChangeLog:38
>> +        6. Even though files are of .mm format, they are written in a way that mix NS, CF and C++ types. Here is the rule:
> 
> in a way that MIXES NS, CF, ...

Fixed.

>> Source/WebCore/ChangeLog:47
>> +        (WebCore::getIdFromAttestationObject): Deleted.
> 
> Can you say why it's okay to delete this ? Do we now use DeviceIdentity framework to support the same task?

It was a mistake before. I forgot the attestation object is encoded in CBOR binaries. Therefore, we can't rely on the old method. Decoding the attestation object on the other hand is tedious. The new way is to pass both the credential id and attestation object back.

>> Source/WebCore/Modules/webauthn/COSEConstants.h:29
>> +
> 
> I suggest a comment:
> 
> // See RFC 8152 - CBOR Object Signing and Encryption <https://tools.ietf.org/html/rfc8152>

Fixed.

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:74
>> +    // It only copies other's rp and user, which are used in another thread.
> 
> I don't understand this comment. It seems like you are warning people not to use it, perhaps because it is incomplete or lacking features you need for the future.
> 
> If so, I would put a FIXME() comment here indicating that it will be revised in a subsequent patch.

Will fix it with BlockPtr.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:48
>> +const uint8_t authenticatorDataFlags = 69;
> 
> I don't understand what this is, or why it has the value 69, but I assume it's correct.

See: https://www.w3.org/TR/webauthn/#flags. Added comments.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:134
>> +    auto* localHash = new Vector<uint8_t>(hash);
> 
> Can you use CrossThreadTask for this? Or at least use CrossThreadCopy to get the data across?
> 
> Is this outer function operating on Main? I suggest you ASSERT(isMainThread()) outside the closure for 'evalutePolicy'.
> 
> Also: This is Objective C++, so you should do the asterisks backwards:
> 
> auto *localCallback
> auto *localException...
> etc.

Will fix it with BlockPtr.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:136
>> +    [context evaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics localizedReason:reason reply:^(BOOL success, NSError *error) {
> 
> ... and ASSERT(!isMainThread()) in here.

Fixed.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:163
>> +                LOG_ERROR("Couldn't attestate: %@", error);
> 
> "Couldn't attest: %@"

Fixed.

>> Source/WebKit/UIProcess/CredentialManagement/WebCredentialsMessengerProxy.cpp:56
>> +        exceptionReply(messageId, { WebCore::NotAllowedError, ASCIILiteral("No avaliable authenticators.") });
> 
> Shouldn't you early return here? You go on and dereference m_authenticator later.

You are right!

>> Source/WebKit/UIProcess/CredentialManagement/WebCredentialsMessengerProxy.cpp:78
>> +    isUserVerifyingPlatformAuthenticatorAvailableReply(messageId, m_authenticator->isAvailable());
> 
> Shouldn't you have an else here? If m_authenticator was just false, you are going to crash here!

Should early return.
Comment 46 Jiewen Tan 2018-03-16 14:49:27 PDT 
Comment on attachment 335911 [details]
Patch

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

Thanks Chris for reviewing this patch.

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:75
>> +    PublicKeyCredentialCreationOptions* isolatedPartialCopyPtr() const
> 
> Why does this return a raw pointer? Does PublicKeyCredentialCreationOptions have a (implicit) move-constructor?

I was not aware there is a BlockPtr. Will rewrite all callbacks with BlockPtr.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.h:47
>> +    explicit LocalAuthenticator() = default;
> 
> Why is this needed?

It shouldn't be needed.

>> Tools/ChangeLog:23
>> +2018-03-11  Jiewen Tan  <jiewen_tan@apple.com>
> 
> Double changelog.

Fixed.
Comment 47 Jiewen Tan 2018-03-16 17:18:33 PDT 
Created attachment 335986 [details]
Patch
Comment 48 EWS Watchlist 2018-03-16 17:20:43 PDT 
Attachment 335986 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:41:  P_256 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.h:56:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:110:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:124:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:138:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:155:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Brent Fulgham 2018-03-16 23:34:45 PDT 
Comment on attachment 335986 [details]
Patch

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

Looks good, please clean up the comments I marked up.

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:74
> +    // Warning: Do not call this at least no other ways around.

This comment is hard to understand. Do you just mean no one should use this because it isn’t currently a complete copy?

> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:147
> +// Not every member are copied.

Not every member IS copied.

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:51
> +// FIXM(rdar://problem/38320512): Define Apple AAGUID.

FIXME

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:123
> +    [context evaluatePolicy:LAPolicyDeviceOwnerAuthenticationWithBiometrics localizedReason:reason reply:BlockPtr<void(BOOL, NSError *)>::fromCallable([weakThis = m_weakFactory.createWeakPtr(*this), callback = WTFMove(callback), exceptionCallback = WTFMove(exceptionCallback), options = crossThreadCopy(options), hash] (BOOL success, NSError *error) mutable {

Nice!!!

> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:243
> +                // L

Is this comment correct? What does L mean here?
Comment 50 Jiewen Tan 2018-03-17 12:21:28 PDT 
Comment on attachment 335986 [details]
Patch

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

Thanks Brent for r+ my patch.

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:74
>> +    // Warning: Do not call this at least no other ways around.
> 
> This comment is hard to understand. Do you just mean no one should use this because it isn’t currently a complete copy?

Oops. I should delete the following method.

>> Source/WebCore/Modules/webauthn/PublicKeyCredentialCreationOptions.h:147
>> +// Not every member are copied.
> 
> Not every member IS copied.

Fixed.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:51
>> +// FIXM(rdar://problem/38320512): Define Apple AAGUID.
> 
> FIXME

Fixed.

>> Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.mm:243
>> +                // L
> 
> Is this comment correct? What does L mean here?

It is literally "L" in the diagram: https://www.w3.org/TR/webauthn/#attestation-object. I added comment. What it means is the Length of the Credential ID.
Comment 51 Jiewen Tan 2018-03-17 12:28:04 PDT 
Created attachment 336009 [details]
Patch for landing
Comment 52 EWS Watchlist 2018-03-17 12:29:19 PDT 
Attachment 336009 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:41:  P_256 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/Modules/webauthn/cocoa/LocalAuthenticator.h:56:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:110:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:124:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:138:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:155:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 WebKit Commit Bot 2018-03-17 18:12:54 PDT 
Comment on attachment 336009 [details]
Patch for landing

Clearing flags on attachment: 336009

Committed r229699: <https://trac.webkit.org/changeset/229699>
Comment 54 Jiewen Tan 2018-03-19 17:04:47 PDT 
Committed r229708: <https://trac.webkit.org/changeset/229708>
Committed r229727: <https://trac.webkit.org/changeset/229727>

To fix some bot failures.