Bug 105467 - [WK2 NetworkProcess] Client doesn't receive SSL certificates
Summary: [WK2 NetworkProcess] Client doesn't receive SSL certificates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-12-19 14:54 PST by Alexey Proskuryakov
Modified: 2012-12-19 16:59 PST (History)
4 users (show)

See Also:


Attachments
proposed fix (14.30 KB, patch)
2012-12-19 15:02 PST, Alexey Proskuryakov
beidson: review-
Details | Formatted Diff | Diff
updated patch (14.53 KB, patch)
2012-12-19 16:53 PST, Alexey Proskuryakov
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2012-12-19 14:54:20 PST
SSL certificate chain is not serialized with NSURLResponse.

<rdar://problem/12890242>
Comment 1 Alexey Proskuryakov 2012-12-19 15:02:51 PST
Created attachment 180233 [details]
proposed fix
Comment 2 WebKit Review Bot 2012-12-19 15:04:16 PST
Attachment 180233 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/Network/WebResourceLoader.h:81:  The parameter name "certificateInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2012-12-19 15:11:38 PST
Comment on attachment 180233 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=180233&action=review

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:155
> -    send(Messages::WebResourceLoader::DidReceiveResponse(response));
> +    send(Messages::WebResourceLoader::DidReceiveResponse(response, PlatformCertificateInfo(response)));

I would much rather see us keep the cert chain encapsulated by the response, and just encode it in the ResourceResponse encoder when it exists.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:84
> -void WebResourceLoader::didReceiveResponse(const WebCore::ResourceResponse& response)
> +void WebResourceLoader::didReceiveResponse(const ResourceResponse& response, const PlatformCertificateInfo& certificateInfo)
>  {
>      LOG(Network, "(WebProcess) WebResourceLoader::didReceiveResponse for '%s'. Status %d.", m_coreLoader->url().string().utf8().data(), response.httpStatusCode());
> -    m_coreLoader->didReceiveResponse(response);
> +    ResourceResponse responseCopy(response);
> +    responseCopy.setCertificateChain(certificateInfo.certificateChain());
> +    m_coreLoader->didReceiveResponse(responseCopy);

Then none of this would be required.
Comment 4 Alexey Proskuryakov 2012-12-19 16:53:20 PST
Created attachment 180249 [details]
updated patch

Discussed this with Brady in person.

I think that the right design is for ResourceRequest and ResourceResponse serializers to only preserve essential lightweight data. Parts that we rarely need can be sent over IPC as separate arguments. Another example of this is how we shouldn't serialize form data with each ResourceRequest.

Another consideration is that we should really think about only passing certificate chain data on demand, not with ResourceResponse.

As a result of this discussion, I renamed didReceiveResponse to didReceiveResponseWithCertificateInfo, so that it doesn't look confusingly similar to other didReceiveResponse functions.
Comment 5 WebKit Review Bot 2012-12-19 16:55:45 PST
Attachment 180249 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/Network/WebResourceLoader.h:81:  The parameter name "certificateInfo" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Brady Eidson 2012-12-19 16:57:06 PST
Yes, please fix the stylebot complaint.  :)
Comment 7 Alexey Proskuryakov 2012-12-19 16:59:37 PST
Fixed <http://trac.webkit.org/changeset/138206>.