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
Jiewen Tan
2018-02-13 22:15:23 PST
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. Created attachment 370494 [details]
Patch
Created attachment 370522 [details]
Patch
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 Created attachment 370531 [details]
Patch
Created attachment 375459 [details]
Patch
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? (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. (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. (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. 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 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. (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. Created attachment 375641 [details]
Patch
Comment on attachment 375641 [details]
Patch
I meant to land-safely...
(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 on attachment 375641 [details] Patch Clearing flags on attachment: 375641 Committed r248308: <https://trac.webkit.org/changeset/248308> All reviewed patches have been landed. Closing bug. (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 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. (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. (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. Build fix: Committed r248319: <https://trac.webkit.org/changeset/248319> Committed r248324: <https://trac.webkit.org/changeset/248324> Committed r248325: <https://trac.webkit.org/changeset/248325> 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 (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. |