Bug 182772

Summary: [WebAuthN] Enable LocalAuthenticator for macOS
Product: WebKit Reporter: Jiewen Tan <jiewen_tan>
Component: WebCore Misc.Assignee: Jiewen Tan <jiewen_tan>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.gaynor, benjamin, bfulgham, cdumez, chris, cmarcelo, commit-queue, dbates, ews-watchlist, james, jbedard, jiewen_tan, jonathan, jschoi, kieun.shin, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 181943    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jiewen Tan 2018-02-13 22:15:23 PST
Implement macOS corresponding authenticator operations.
Comment 1 Jiewen Tan 2018-03-12 12:28:09 PDT
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.
Comment 2 Radar WebKit Bug Importer 2018-08-15 13:41:23 PDT
<rdar://problem/43347920>
Comment 3 Jiewen Tan 2019-05-23 01:08:11 PDT
Created attachment 370494 [details]
Patch
Comment 4 Jiewen Tan 2019-05-23 14:53:52 PDT
Created attachment 370522 [details]
Patch
Comment 5 EWS Watchlist 2019-05-23 16:34:02 PDT Comment hidden (obsolete)
Comment 6 Jiewen Tan 2019-05-23 16:35:03 PDT
Created attachment 370531 [details]
Patch
Comment 7 Jiewen Tan 2019-08-02 14:47:59 PDT
Created attachment 375459 [details]
Patch
Comment 8 Brent Fulgham 2019-08-02 15:07:04 PDT
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?
Comment 9 Jonathan Bedard 2019-08-02 15:21:08 PDT
(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.
Comment 10 Jiewen Tan 2019-08-02 15:25:34 PDT
(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.
Comment 11 Jiewen Tan 2019-08-02 15:25:51 PDT
(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.
Comment 12 Jonathan Bedard 2019-08-02 17:01:51 PDT
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.
Comment 13 Jonathan Bedard 2019-08-06 09:12:29 PDT
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.
Comment 14 Jiewen Tan 2019-08-06 11:56:16 PDT
(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.
Comment 15 Jiewen Tan 2019-08-06 12:08:57 PDT
Created attachment 375641 [details]
Patch
Comment 16 Jiewen Tan 2019-08-06 12:09:50 PDT
Comment on attachment 375641 [details]
Patch

I meant to land-safely...
Comment 17 Jonathan Bedard 2019-08-06 12:52:32 PDT
(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)
Comment 18 WebKit Commit Bot 2019-08-06 12:54:50 PDT
Comment on attachment 375641 [details]
Patch

Clearing flags on attachment: 375641

Committed r248308: <https://trac.webkit.org/changeset/248308>
Comment 19 WebKit Commit Bot 2019-08-06 12:54:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Jiewen Tan 2019-08-06 13:03:55 PDT
(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.
Comment 21 Chris Dumez 2019-08-06 14:29:47 PDT
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.
Comment 22 Jiewen Tan 2019-08-06 14:40:27 PDT
(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.
Comment 23 Jiewen Tan 2019-08-06 14:43:01 PDT
(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.
Comment 24 Jiewen Tan 2019-08-06 15:13:07 PDT
Build fix:
Committed r248319: <https://trac.webkit.org/changeset/248319>
Comment 25 Jiewen Tan 2019-08-06 15:41:05 PDT
Committed r248324: <https://trac.webkit.org/changeset/248324>
Comment 26 Jiewen Tan 2019-08-06 16:21:06 PDT
Committed r248325: <https://trac.webkit.org/changeset/248325>
Comment 27 Ryan Haddad 2019-08-06 17:09:37 PDT
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
Comment 28 Ryan Haddad 2019-08-13 15:31:13 PDT
(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.