Bug 183527 – [WebAuthN] Implement authenticatorMakeCredential

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

[Jiewen Tan]

Implement authenticatorMakeCredential with self attestation. See: https://www.w3.org/TR/webauthn/#op-make-cred.

[Jiewen Tan]

<rdar://problem/35275886>

[Radar WebKit Bug Importer]

<rdar://problem/38318968>

[Jiewen Tan]

Please refer to the radar for a more detailed explanation of the KeyChain Schema and the tentative Apple Attestation Format.

[Jiewen Tan]

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 335495 [details]
Patch

[Brent Fulgham]

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!

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 335534 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:84:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:98:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:112:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:126:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 29 files


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

[Jiewen Tan]

Created attachment 335541 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:84:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:98:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:112:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:126:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 29 files


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

[Jiewen Tan]

Created attachment 335549 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:84:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:98:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:112:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:126:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 29 files


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

[Jiewen Tan]

Created attachment 335551 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:84:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:98:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:112:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:126:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 29 files


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

[EWS Watchlist]

Comment on attachment 335551 [details]
Patch

Attachment 335551 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6906915

New failing tests:
http/wpt/credential-management/credentialscontainer-store-basics.https.html

[EWS Watchlist]

Created attachment 335554 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335551 [details]
Patch

Attachment 335551 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/6906841

New failing tests:
http/wpt/credential-management/credentialscontainer-store-basics.https.html

[EWS Watchlist]

Created attachment 335555 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Jiewen Tan]

Created attachment 335569 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  P_256 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
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:84:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:98:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:112:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:126:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 7 in 30 files


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

[Jiewen Tan]

Will split this patch into two such that the soft linking part can be landed sooner.

[Jiewen Tan]

Created attachment 335684 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  P_256 is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
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:84:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:98:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:112:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:126:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 7 in 29 files


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

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.

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 335877 [details]
Patch

[Jiewen Tan]

Created attachment 335880 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:109:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:123:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:137:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:151:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 33 files


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

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 335888 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:109:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:123:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:137:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:151:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 33 files


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

[Jiewen Tan]

Created attachment 335905 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:109:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:123:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:137:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:151:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 36 files


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

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.

[Jiewen Tan]

(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.

[Jiewen Tan]

Created attachment 335911 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/webauthn/COSEConstants.h:40:  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:55:  _Nonnull is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:109:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:123:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:137:  Missing spaces around :  [whitespace/init] [4]
ERROR: Tools/TestWebKitAPI/Tests/ios/LocalAuthenticator.mm:151:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 6 in 36 files


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

[Jiewen Tan]

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.

[Jiewen Tan]

(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.

[Jiewen Tan]

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.

[Brent Fulgham]

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!

[Chris Dumez]

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
> [phone]  Jiewen Tan  <[e-mail]>

Double changelog.

[Jiewen Tan]

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.

[Jiewen Tan]

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
>> [phone]  Jiewen Tan  <[e-mail]>
> 
> Double changelog.

Fixed.

[Jiewen Tan]

Created attachment 335986 [details]
Patch

[EWS Watchlist]

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.

[Brent Fulgham]

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?

[Jiewen Tan]

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.

[Jiewen Tan]

Created attachment 336009 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 336009 [details]
Patch for landing

Clearing flags on attachment: 336009

Committed r229699: <https://trac.webkit.org/changeset/229699>

[Jiewen Tan]

Committed r229708: <https://trac.webkit.org/changeset/229708>
Committed r229727: <https://trac.webkit.org/changeset/229727>

To fix some bot failures.