Bug 57707

Summary: WebKit2: Specify the certificate store in WKBundleSetClientCertificate()
Product: WebKit Reporter: Jeff Miller <jeffm>
Component: WebKit2Assignee: Jeff Miller <jeffm>
Status: RESOLVED FIXED    
Severity: Normal CC: mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Attachments:
Description Flags
Patch mitz: review+

Description Jeff Miller 2011-04-02 11:42:53 PDT
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).
Comment 1 Jeff Miller 2011-04-02 11:47:54 PDT
Created attachment 87980 [details]
Patch
Comment 2 mitz 2011-04-02 12:03:27 PDT
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?
Comment 3 Jeff Miller 2011-04-02 13:54:17 PDT
(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.
Comment 4 Jeff Miller 2011-04-02 15:00:24 PDT
Committed r82776: <http://trac.webkit.org/changeset/82776>