Bug 182772 – [WebAuthN] Enable LocalAuthenticator for macOS

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

[Jiewen Tan]

Implement macOS corresponding authenticator operations.

[Jiewen Tan]

1. We could use kSecAttrNoLegacy=@YES SPI to enforce macOS Keychain to use the iOS Keychain. Such that we can unify the Keychain behaviors between iOS and macOS.

[Radar WebKit Bug Importer]

<rdar://problem/43347920>

[Jiewen Tan]

Created attachment 370494 [details]
Patch

[Jiewen Tan]

Created attachment 370522 [details]
Patch

[EWS Watchlist]

Comment on attachment 370522 [details]
Patch

Attachment 370522 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12272517

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-baseline
apiTests

[Jiewen Tan]

Created attachment 370531 [details]
Patch

[Jiewen Tan]

Created attachment 375459 [details]
Patch

[Brent Fulgham]

Comment on attachment 375459 [details]
Patch

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

> LayoutTests/platform/mac-wk2/TestExpectations:914
> +http/wpt/webauthn/idl.https.html [ Failure ]

Will these ever be Successes in the open source test environment? Maybe we should wait to un-skip then until they have the ability to use the modern keychain?

[Jonathan Bedard]

(In reply to Brent Fulgham from comment #8)
> Comment on attachment 375459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375459&action=review
> 
> > LayoutTests/platform/mac-wk2/TestExpectations:914
> > +http/wpt/webauthn/idl.https.html [ Failure ]
> 
> Will these ever be Successes in the open source test environment? Maybe we
> should wait to un-skip then until they have the ability to use the modern
> keychain?

I have a Catalina seed install...I'd like to see what this patch does on that before landing it. I know especially that Jiewen was having trouble getting the keychain entitlements working, I'd like to see if we can sort that out.

[Jiewen Tan]

(In reply to Brent Fulgham from comment #8)
> Comment on attachment 375459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375459&action=review
> 
> > LayoutTests/platform/mac-wk2/TestExpectations:914
> > +http/wpt/webauthn/idl.https.html [ Failure ]
> 
> Will these ever be Successes in the open source test environment? Maybe we
> should wait to un-skip then until they have the ability to use the modern
> keychain?

Thanks Brent for r+ this patch.

[Jiewen Tan]

(In reply to Jonathan Bedard from comment #9)
> (In reply to Brent Fulgham from comment #8)
> > Comment on attachment 375459 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=375459&action=review
> > 
> > > LayoutTests/platform/mac-wk2/TestExpectations:914
> > > +http/wpt/webauthn/idl.https.html [ Failure ]
> > 
> > Will these ever be Successes in the open source test environment? Maybe we
> > should wait to un-skip then until they have the ability to use the modern
> > keychain?
> 
> I have a Catalina seed install...I'd like to see what this patch does on
> that before landing it. I know especially that Jiewen was having trouble
> getting the keychain entitlements working, I'd like to see if we can sort
> that out.

Thanks Jonathan.

[Jonathan Bedard]

Someone broke Catalina builds on the seeds...I will get to this first thing Monday morning. It's not super urgent because we don't have any Catalina seed automation yet.

[Jonathan Bedard]

Comment on attachment 375459 [details]
Patch

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

>>>>> LayoutTests/platform/mac-wk2/TestExpectations:914
>>>>> +http/wpt/webauthn/idl.https.html [ Failure ]
>>>> 
>>>> Will these ever be Successes in the open source test environment? Maybe we should wait to un-skip then until they have the ability to use the modern keychain?
>>> 
>>> I have a Catalina seed install...I'd like to see what this patch does on that before landing it. I know especially that Jiewen was having trouble getting the keychain entitlements working, I'd like to see if we can sort that out.
>> 
>> Thanks Brent for r+ this patch.
> 
> Thanks Jonathan.

I tried adopting the same code we use JSC tools, but no luck. 

I think we should change this comment to: 'Local authenticator tests require keychain entitlement' or something to that effect....I don't think we will be able to give that entitlement with adhoc code signing.

[Jiewen Tan]

(In reply to Jonathan Bedard from comment #13)
> Comment on attachment 375459 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375459&action=review
> 
> >>>>> LayoutTests/platform/mac-wk2/TestExpectations:914
> >>>>> +http/wpt/webauthn/idl.https.html [ Failure ]
> >>>> 
> >>>> Will these ever be Successes in the open source test environment? Maybe we should wait to un-skip then until they have the ability to use the modern keychain?
> >>> 
> >>> I have a Catalina seed install...I'd like to see what this patch does on that before landing it. I know especially that Jiewen was having trouble getting the keychain entitlements working, I'd like to see if we can sort that out.
> >> 
> >> Thanks Brent for r+ this patch.
> > 
> > Thanks Jonathan.
> 
> I tried adopting the same code we use JSC tools, but no luck. 
> 
> I think we should change this comment to: 'Local authenticator tests require
> keychain entitlement' or something to that effect....I don't think we will
> be able to give that entitlement with adhoc code signing.

Sure, I will change the comment to something more clear.

[Jiewen Tan]

Created attachment 375641 [details]
Patch

[Jiewen Tan]

Comment on attachment 375641 [details]
Patch

I meant to land-safely...

[Jonathan Bedard]

(In reply to Jiewen Tan from comment #16)
> Comment on attachment 375641 [details]
> Patch
> 
> I meant to land-safely...

For future reference, commit queue should be smart enough to understand the 'Reviewed by' line in your changelog, you don't need to r+ through the bugzilla UI (and in fact, I'm actually not sure what that will do)

[WebKit Commit Bot]

Comment on attachment 375641 [details]
Patch

Clearing flags on attachment: 375641

Committed r248308: <https://trac.webkit.org/changeset/248308>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jiewen Tan]

(In reply to Jonathan Bedard from comment #17)
> (In reply to Jiewen Tan from comment #16)
> > Comment on attachment 375641 [details]
> > Patch
> > 
> > I meant to land-safely...
> 
> For future reference, commit queue should be smart enough to understand the
> 'Reviewed by' line in your changelog, you don't need to r+ through the
> bugzilla UI (and in fact, I'm actually not sure what that will do)

Thanks. I didn't know that.

[Chris Dumez]

Comment on attachment 375641 [details]
Patch

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

> Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:122
> +            (id)kSecAttrNoLegacy: @YES

This is deprecated and should probably not be used in new code. This broke the build for me.

[Jiewen Tan]

(In reply to Chris Dumez from comment #21)
> Comment on attachment 375641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375641&action=review
> 
> > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:122
> > +            (id)kSecAttrNoLegacy: @YES
> 
> This is deprecated and should probably not be used in new code. This broke
> the build for me.

But the new API is only available for iOS 13 and macOS Catalina. I guess we should use ALLOW_DEPRECATED_DECLARATIONS_BEGIN/ALLOW_DEPRECATED_DECLARATIONS_END to guard it for now.

[Jiewen Tan]

(In reply to Jiewen Tan from comment #22)
> (In reply to Chris Dumez from comment #21)
> > Comment on attachment 375641 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=375641&action=review
> > 
> > > Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:122
> > > +            (id)kSecAttrNoLegacy: @YES
> > 
> > This is deprecated and should probably not be used in new code. This broke
> > the build for me.
> 
> But the new API is only available for iOS 13 and macOS Catalina. I guess we
> should use
> ALLOW_DEPRECATED_DECLARATIONS_BEGIN/ALLOW_DEPRECATED_DECLARATIONS_END to
> guard it for now.

Sorry, I should probably guard it with OS Versions for the new API as a proper fix.

[Jiewen Tan]

Build fix:
Committed r248319: <https://trac.webkit.org/changeset/248319>

[Jiewen Tan]

Committed r248324: <https://trac.webkit.org/changeset/248324>

[Jiewen Tan]

Committed r248325: <https://trac.webkit.org/changeset/248325>

[Ryan Haddad]

There are 8 webauthn tests failing an assertion on debug bots after r248308:

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r248311%20(8877)/results.html

[Ryan Haddad]

(In reply to Ryan Haddad from comment #27)
> There are 8 webauthn tests failing an assertion on debug bots after r248308:
> 
> https://build.webkit.org/results/
> Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r248311%20(8877)/results.html
Skipped these tests in https://trac.webkit.org/changeset/248639/webkit since they aren't expected to work in open source.