Adopt SecKeyProxy API in certificate based challenge response codes.
<rdar://problem/34586181>
Created attachment 340928 [details] Patch
Comment on attachment 340928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340928&action=review This is excellent! I'd like one other person to look at the XPC/IPC stuff (perhaps Ryosuke?) but this looks great. Please fix the GTK/WPE build failures. > Source/WebKit/ChangeLog:13 > + them to relay crypto operations to the server process while maintaining the same SecKey interfaces alike local operations. "maintaining the same SecKey interfaces alike local operations." should probably be: "maintaining the same SecKey interfaces as used for local operations." > Source/WebKit/ChangeLog:15 > + object alive long enough for the client process, i.e. Network Processes in our case, to finish all operations, and destroys "and destroys the proxy object afterwards." should be "and then destroy the proxy object afterward." > Source/WebKit/ChangeLog:18 > + 3) A new class called SecKeyProxyStore is then created to bind the lifetime of SecKeyProxy to the WebsiteDataStore while initializes "while initializes" should be "while initializing" > Source/WebKit/ChangeLog:19 > + it correctly. The time while the authentication process reaches WebPageProxy::didReceiveAuthenticationChallengeProxy where we have "The time while the authentication process reaches" should be "At the time the authentication process reaches" > Source/WebKit/ChangeLog:20 > + accesses to the WebsiteDataStore, we haven't yet be able to determine the Credential to authenticate the challenge. Therefore, we "we haven't yet be able to" should be "we haven't yet been able to" > Source/WebKit/ChangeLog:23 > + a strong reference of the SecKeyProxy, and move it to the WebsiteDataStore. And then we create a weak reference of SecKeyProxyStore "And then we create a weak reference of" should be "We also create a weak reference to" > Source/WebKit/ChangeLog:24 > + and move it to the AuthenticationChallenge. Such that we indirectly bind the lifetime of SecKeyProxy to the WebsiteDataStore through "Such that we indirectly" should be "In this way, we indirectly" > Source/WebKit/ChangeLog:29 > + so, it sends xpc messages at the palace where original IPC messages are sent and overwrites the boostrap listener of the xpc connection 'xpc messages at the palace" should be "xpc messages at the place" > Source/WebKit/ChangeLog:31 > + 5) Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog in macOS should say Safari instead of "Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog" should be "Tests are manually covered by tlstestwebkit.org. Note that the prompting Keychain dialog" Also: Is this really macOS-only? I would imagine it would apply to iOS as well. > Source/WebKit/UIProcess/WebPageProxy.cpp:78 > +#include "SecKeyProxyStore.h" This include should be wrapped in #if HAVE(SEC_KEY_PROXY)
Comment on attachment 340928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340928&action=review > Source/WebKit/Shared/AuthenticationManagerCocoa.mm:62 > + if (error) { I think the 'retainError' should probably be done inside the nil check, since there's no point in wrapping the nil in an NSRetainPtr<>. We should probably log this error condition, unless it's a common/expected occurrence. > Source/WebKit/Shared/AuthenticationManagerCocoa.mm:74 > + for (size_t i = 0; i < total; i++) { I think this whole loop could be done inside the "if (total)" test. > Source/WebKit/Shared/AuthenticationManagerCocoa.mm:78 > + [certificates addObject:(id)certificate]; I think this is a leak: SecCertificateCreateWIthData returns a +1 retain count, and NSMutableArray's "addObject" will retain the argument passed to it. You should do an "adoptNS" on the return value of SecCertificateCreateWithData,
Comment on attachment 340928 [details] Patch r- to correct the leak.
Comment on attachment 340928 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340928&action=review Thanks Brent for reviewing the patch. >> Source/WebKit/ChangeLog:13 >> + them to relay crypto operations to the server process while maintaining the same SecKey interfaces alike local operations. > > "maintaining the same SecKey interfaces alike local operations." > > should probably be: > > "maintaining the same SecKey interfaces as used for local operations." Fixed. >> Source/WebKit/ChangeLog:15 >> + object alive long enough for the client process, i.e. Network Processes in our case, to finish all operations, and destroys > > "and destroys the proxy object afterwards." should be "and then destroy the proxy object afterward." Fixed. >> Source/WebKit/ChangeLog:18 >> + 3) A new class called SecKeyProxyStore is then created to bind the lifetime of SecKeyProxy to the WebsiteDataStore while initializes > > "while initializes" should be "while initializing" Fixed. >> Source/WebKit/ChangeLog:19 >> + it correctly. The time while the authentication process reaches WebPageProxy::didReceiveAuthenticationChallengeProxy where we have > > "The time while the authentication process reaches" should be "At the time the authentication process reaches" Fixed. >> Source/WebKit/ChangeLog:20 >> + accesses to the WebsiteDataStore, we haven't yet be able to determine the Credential to authenticate the challenge. Therefore, we > > "we haven't yet be able to" should be "we haven't yet been able to" Fixed. >> Source/WebKit/ChangeLog:23 >> + a strong reference of the SecKeyProxy, and move it to the WebsiteDataStore. And then we create a weak reference of SecKeyProxyStore > > "And then we create a weak reference of" should be "We also create a weak reference to" Fixed. >> Source/WebKit/ChangeLog:24 >> + and move it to the AuthenticationChallenge. Such that we indirectly bind the lifetime of SecKeyProxy to the WebsiteDataStore through > > "Such that we indirectly" should be "In this way, we indirectly" Fixed. >> Source/WebKit/ChangeLog:29 >> + so, it sends xpc messages at the palace where original IPC messages are sent and overwrites the boostrap listener of the xpc connection > > 'xpc messages at the palace" should be "xpc messages at the place" Fixed. >> Source/WebKit/ChangeLog:31 >> + 5) Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog in macOS should say Safari instead of > > "Tests, again, are manually covered by tlstestwebkit.org. Noted, the prompting Keychain dialog" should be "Tests are manually covered by tlstestwebkit.org. Note that the prompting Keychain dialog" > > Also: Is this really macOS-only? I would imagine it would apply to iOS as well. This is for both iOS and macOS, but only macOS Keychain will prompt the dialog "XXX wants to access the private key stored in XXX". >> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:62 >> + if (error) { > > I think the 'retainError' should probably be done inside the nil check, since there's no point in wrapping the nil in an NSRetainPtr<>. We should probably log this error condition, unless it's a common/expected occurrence. Fixed. It looks like that we don't usually retain NSError. Hence, I removed the `adoptNS(error)` line. Also, I added the following error message: LOG_ERROR("Couldn't create identity from end point: %@", error); >> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:74 >> + for (size_t i = 0; i < total; i++) { > > I think this whole loop could be done inside the "if (total)" test. Fixed. >> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:78 >> + [certificates addObject:(id)certificate]; > > I think this is a leak: SecCertificateCreateWIthData returns a +1 retain count, and NSMutableArray's "addObject" will retain the argument passed to it. > > You should do an "adoptNS" on the return value of SecCertificateCreateWithData, Fixed. >> Source/WebKit/UIProcess/WebPageProxy.cpp:78 >> +#include "SecKeyProxyStore.h" > > This include should be wrapped in #if HAVE(SEC_KEY_PROXY) Fixed all the places that include SecKeyProxyStore.h. This should solve build error for other builds.
Created attachment 341009 [details] Patch
Comment on attachment 341009 [details] Patch I think this look good now. I'd like a WK2 owner to double-check the XPC code.
Comment on attachment 341009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341009&action=review Is there some kind of signal we should be listening to when the SecKeyProxy becomes invalid? > Source/WebKit/Shared/AuthenticationManagerCocoa.mm:50 > + ASSERT(type == XPC_TYPE_DICTIONARY); This seems a bit fragile. I think we should make it so if anything isn't as expected, we assert and fail gracefully. Can we trust the values coming from this xpc connection? > Source/WebKit/Shared/AuthenticationManagerCocoa.mm:53 > + auto challengeID = xpc_dictionary_get_uint64(event, "challenge-id"); It would be good to define these names in a header somewhere so we don't have magic strings in multiple places. That way we can see that the protocol is the same on the client and server sides. > Source/WebKit/Shared/AuthenticationManagerCocoa.mm:55 > + SecIdentityRef identity = NULL; nullptr, here and elsewhere. > Source/WebKit/UIProcess/WebPageProxy.cpp:6212 > + auto secKeyProxyStore = SecKeyProxyStore::create(); > + authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore); > + m_websiteDataStore->addSecKeyProxyStore(WTFMove(secKeyProxyStore)); This doesn't look very elegant. Couldn't we just create it when we're calling initialize in this patch? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:238 > + Vector<RefPtr<SecKeyProxyStore>> m_secKeyProxyStores; This could be a Vector<Ref<SecKeyProxyStore>>
(In reply to Alex Christensen from comment #9) > Comment on attachment 341009 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341009&action=review > > Is there some kind of signal we should be listening to when the SecKeyProxy > becomes invalid? > I don't think so. A finer grained design would be we remove those SecKeyProxy objects once their corresponding challenges are answered by the Networking Process. Since we usually don't expect any callbacks from CFNetwork after submitting the credential, we don't know when to terminate those proxies. We could potential terminate those proxies after receiving a resource response? Not sure if it is necessary though.
Comment on attachment 341009 [details] Patch Attachment 341009 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7769847 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 341047 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 341009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341009&action=review Thanks Alex for reviewing the patch. >> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:50 >> + ASSERT(type == XPC_TYPE_DICTIONARY); > > This seems a bit fragile. I think we should make it so if anything isn't as expected, we assert and fail gracefully. Can we trust the values coming from this xpc connection? like the following? if (type != XPC_TYPE_DICTIONARY) { ASSERT_NOT_REACHED(); // LOG ? return; } >> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:53 >> + auto challengeID = xpc_dictionary_get_uint64(event, "challenge-id"); > > It would be good to define these names in a header somewhere so we don't have magic strings in multiple places. That way we can see that the protocol is the same on the client and server sides. Fixed. Added XpcConstants.h >> Source/WebKit/Shared/AuthenticationManagerCocoa.mm:55 >> + SecIdentityRef identity = NULL; > > nullptr, here and elsewhere. Fixed. >> Source/WebKit/UIProcess/WebPageProxy.cpp:6212 >> + m_websiteDataStore->addSecKeyProxyStore(WTFMove(secKeyProxyStore)); > > This doesn't look very elegant. Couldn't we just create it when we're calling initialize in this patch? I don't think so. Please refer to the change log explanation. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:238 >> + Vector<RefPtr<SecKeyProxyStore>> m_secKeyProxyStores; > > This could be a Vector<Ref<SecKeyProxyStore>> Fixed.
The Win bot's tree is red.
(In reply to Jiewen Tan from comment #13) > like the following? > if (type != XPC_TYPE_DICTIONARY) { > ASSERT_NOT_REACHED(); > // LOG ? > return; > } And also if the persistence is out of range, if the certificates isn't an array or contains anything unexpected, etc.
Created attachment 341058 [details] Patch
Comment on attachment 341058 [details] Patch Attachment 341058 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/7772299 New failing tests: stress/destructuring-rest-element.js.ftl-eager
Comment on attachment 341058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341058&action=review > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:46 > + // The following xpc event handler overwrites the boostrap event handler and is only used > + // to capture client certificate credential. > + xpc_connection_set_event_handler(connection->xpcConnection(), ^(xpc_object_t event) { Will this intercept any other events? > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:50 > + if (type != XPC_TYPE_ERROR && weakThis) { This could be an early return instead of indenting everything. > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:64 > + { I don't think these additional scopes add much. > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68 > + identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error]; This needs to be adopted. It's a memory leak as it stands now. > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:69 > + if (error) { if (!identity || error) > Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:81 > + auto total = xpc_array_get_count(certificateDataArray); > + if (total) { You can declare total inside the if statement. if (auto total = ...) > Source/WebKit/UIProcess/WebPageProxy.cpp:6211 > + auto secKeyProxyStore = SecKeyProxyStore::create(); > + authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore); I still don't like this design. Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:182 > + void addSecKeyProxyStore(RefPtr<SecKeyProxyStore>&&); This should be a Ref.
Comment on attachment 341058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341058&action=review >> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:46 >> + xpc_connection_set_event_handler(connection->xpcConnection(), ^(xpc_object_t event) { > > Will this intercept any other events? I believe not. Even if so, the beneath assertion would fire. >> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:50 >> + if (type != XPC_TYPE_ERROR && weakThis) { > > This could be an early return instead of indenting everything. Got it. >> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:64 >> + { > > I don't think these additional scopes add much. The scope here is trying to make it clear that the following block is to create the identity. >> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68 >> + identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error]; > > This needs to be adopted. It's a memory leak as it stands now. I believe only NSObjects that are initialized with [[XXX alloc] init: XXX] needs to be adopted. Things like [XXX createXXX] should be managed by autorelease pool. >> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:69 >> + if (error) { > > if (!identity || error) Okay. >> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:81 >> + if (total) { > > You can declare total inside the if statement. > if (auto total = ...) Sure. >> Source/WebKit/UIProcess/WebPageProxy.cpp:6211 >> + authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore); > > I still don't like this design. Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet. The purpose here is to bind the secKeyProxyStore to the m_websiteDataStore. I don't understand what you proposed can achieve the goal that: 1) Bind the lifetime of secKeyProxyStore to websiteDataStore. 2) Initialize secKeyProxyStore when the Credential/SecIdentity is ready. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:182 >> + void addSecKeyProxyStore(RefPtr<SecKeyProxyStore>&&); > > This should be a Ref. Fixed.
Comment on attachment 341058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341058&action=review >>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68 >>> + identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error]; >> >> This needs to be adopted. It's a memory leak as it stands now. > > I believe only NSObjects that are initialized with [[XXX alloc] init: XXX] needs to be adopted. Things like [XXX createXXX] should be managed by autorelease pool. I don't think that you're correct. >>> Source/WebKit/UIProcess/WebPageProxy.cpp:6211 >>> + authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore); >> >> I still don't like this design. Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet. > > The purpose here is to bind the secKeyProxyStore to the m_websiteDataStore. I don't understand what you proposed can achieve the goal that: > 1) Bind the lifetime of secKeyProxyStore to websiteDataStore. > 2) Initialize secKeyProxyStore when the Credential/SecIdentity is ready. I guess you're right. The challenge doesn't know about the data store.
(In reply to Alex Christensen from comment #20) > Comment on attachment 341058 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341058&action=review > > >>> Source/WebKit/Shared/Authentication/cocoa/AuthenticationManagerCocoa.mm:68 > >>> + identity = [SecKeyProxy createIdentityFromEndpoint:endPoint.get() error:&error]; > >> > >> This needs to be adopted. It's a memory leak as it stands now. > > > > I believe only NSObjects that are initialized with [[XXX alloc] init: XXX] needs to be adopted. Things like [XXX createXXX] should be managed by autorelease pool. > > I don't think that you're correct. Okay. Then what's the rule of using adoptNS()? > > >>> Source/WebKit/UIProcess/WebPageProxy.cpp:6211 > >>> + authenticationChallenge->setSecKeyProxyStore(secKeyProxyStore); > >> > >> I still don't like this design. Let's make SecKeyProxyStore::create take a const WebCore::Credential& parameter and create it where we call m_secKeyProxyStore->initialize instead of creating it here without a purpose yet. > > > > The purpose here is to bind the secKeyProxyStore to the m_websiteDataStore. I don't understand what you proposed can achieve the goal that: > > 1) Bind the lifetime of secKeyProxyStore to websiteDataStore. > > 2) Initialize secKeyProxyStore when the Credential/SecIdentity is ready. > > I guess you're right. The challenge doesn't know about the data store. Yup. Maybe there are better ways around?
(In reply to Jiewen Tan from comment #21) > Okay. Then what's the rule of using adoptNS()? If a function returns a newly created object with a +1 retain count, we need to adopt it so we will free it when we're done. > Yup. Maybe there are better ways around? Probably.
Created attachment 341144 [details] Patch
(In reply to Alex Christensen from comment #22) > (In reply to Jiewen Tan from comment #21) > > Okay. Then what's the rule of using adoptNS()? > If a function returns a newly created object with a +1 retain count, we need > to adopt it so we will free it when we're done. > Fixed. > > Yup. Maybe there are better ways around? > Probably.
Comment on attachment 341144 [details] Patch Thanks Alex for r+ the patch.
Comment on attachment 341144 [details] Patch Clearing flags on attachment: 341144 Committed r232165: <https://trac.webkit.org/changeset/232165>
All reviewed patches have been landed. Closing bug.