The PCCERT_CONTEXT for the client certificate we create from the message from the UI process doesn't contain enough information to actually use it in a request, we need to get the real certificate from the certificate store (which is typically the "MY" store).
Created attachment 87980 [details] Patch
Comment on attachment 87980 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87980&action=review > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:114 > + ASSERT(!certificateSystemStoreName.isEmpty()); You can use ASSERT_ARG here. > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:122 > + ASSERT(certStore); I don’t know this Windows API, but I’m not sure it’s correct to assert this. At the very least it asserts that the bundle code, which is not part of WebKit, isn’t sending a bogus store name. But even if the store name was correct at some point, isn’t it possible that by the time we reach this statement, the store has been deleted, renamed, or otherwise made un-openable? Maybe LOG_ERROR() is more appropriate than ASSERT(). > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:127 > + ASSERT(realCert); Similar comment here. > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:134 > + // We can't close certStore here, since the certificate is still in use. So we never close it. Is this OK?
(In reply to comment #2) > (From update of attachment 87980 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87980&action=review > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:114 > > + ASSERT(!certificateSystemStoreName.isEmpty()); > > You can use ASSERT_ARG here. Will do. > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:122 > > + ASSERT(certStore); > > I don’t know this Windows API, but I’m not sure it’s correct to assert this. At the very least it asserts that the bundle code, which is not part of WebKit, isn’t sending a bogus store name. But even if the store name was correct at some point, isn’t it possible that by the time we reach this statement, the store has been deleted, renamed, or otherwise made un-openable? Maybe LOG_ERROR() is more appropriate than ASSERT(). In theory, a system certificate store should always be available, but you're right, I should use LOG_ERROR(). > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:127 > > + ASSERT(realCert); > > Similar comment here. Yes, I'll change this to LOG_ERROR() as well. > > > Source/WebKit2/WebProcess/InjectedBundle/win/InjectedBundleWin.cpp:134 > > + // We can't close certStore here, since the certificate is still in use. > > So we never close it. Is this OK? Yes, this is what we do in Safari as well.
Committed r82776: <http://trac.webkit.org/changeset/82776>