Bug 57707 – WebKit2: Specify the certificate store in WKBundleSetClientCertificate()

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeff Miller]

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).

[Jeff Miller]

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?

[Jeff Miller]

(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.

[Jeff Miller]

Committed r82776: <http://trac.webkit.org/changeset/82776>