Include unverified server-sent cert chain in reports

Include unverified server-sent cert chain in reports

As of https://codereview.chromium.org/1174073003/, net::SSLInfo contains
the unverified certificate chain as received by the client (which might
be different than the chain as built by the client during
verification). This CL adds the unverified chain to invalid certificate
reports.

BUG=498945
Committed: https://crrev.com/fcac4d5822ebb8b831a2125b1800e4cc65fe6b3d
Cr-Commit-Position: refs/heads/master@{#334472}

Committed: https://crrev.com/2910d62cffd1c2fa949cf0b5e8417fa114f58491
Cr-Commit-Position: refs/heads/master@{#334975}

[estark, 2015-06-15 18:58:33]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2015-06-15 18:58:33]

meacer: could you please take a look?

For more context, https://codereview.chromium.org/1174073003/ is the CL that
adds |unverified_cert| to SSLInfo.

[meacer, 2015-06-15 19:57:20]

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/cert_log...
File chrome/browser/ssl/cert_logger.proto (right):

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/cert_log...
chrome/browser/ssl/cert_logger.proto:91: optional string unverified_cert_chain =
8;
Just to clarify: Should we always expect this not to be empty? Does caching etc.
change whether |unverified_cert_chain| will be empty?

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
File chrome/browser/ssl/certificate_error_report.cc (right):

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
chrome/browser/ssl/certificate_error_report.cc:58: for (size_t i = 0; i <
pem_encoded_chain.size(); ++i)
You can use C++11 style for loop here:

for (const string& pem: pem_encoded_chain)
  result->append(pem);

Or directly use JoinString from string_util.

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
chrome/browser/ssl/certificate_error_report.cc:79: LOG(ERROR) << "Could not get
PEM encoded chain.";
Wrap this statement with braces since |if| spans two lines.

[estark, 2015-06-15 20:47:37]

Thanks meacer.

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/cert_log...
File chrome/browser/ssl/cert_logger.proto (right):

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/cert_log...
chrome/browser/ssl/cert_logger.proto:91: optional string unverified_cert_chain =
8;
On 2015/06/15 19:57:20, Mustafa Emre Acer wrote:
> Just to clarify: Should we always expect this not to be empty? Does caching
etc.
> change whether |unverified_cert_chain| will be empty?

I believe we should always expect it to be non-empty. SSLInfo::unverified_cert
will be empty for resources that were loaded from cache, but since we are
sending reports for pages that have certificate errors and we don't cache
resources with certificate errors, this should always be filled. (That is, if we
are showing an interstitial page, I don't think the page could have been loaded
from cache.) Should I add a comment explaining that, or is too much detail?

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
File chrome/browser/ssl/certificate_error_report.cc (right):

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
chrome/browser/ssl/certificate_error_report.cc:58: for (size_t i = 0; i <
pem_encoded_chain.size(); ++i)
On 2015/06/15 19:57:20, Mustafa Emre Acer wrote:
> You can use C++11 style for loop here:
> 
> for (const string& pem: pem_encoded_chain)
>   result->append(pem);
> 
> Or directly use JoinString from string_util.

Done. (JoinString)

https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
chrome/browser/ssl/certificate_error_report.cc:79: LOG(ERROR) << "Could not get
PEM encoded chain.";
On 2015/06/15 19:57:20, Mustafa Emre Acer wrote:
> Wrap this statement with braces since |if| spans two lines.

Done.

[meacer, 2015-06-15 20:55:50]

On 2015/06/15 20:47:37, estark wrote:
> Thanks meacer.
> 
>
https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/cert_log...
> File chrome/browser/ssl/cert_logger.proto (right):
> 
>
https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/cert_log...
> chrome/browser/ssl/cert_logger.proto:91: optional string unverified_cert_chain
=
> 8;
> On 2015/06/15 19:57:20, Mustafa Emre Acer wrote:
> > Just to clarify: Should we always expect this not to be empty? Does caching
> etc.
> > change whether |unverified_cert_chain| will be empty?
> 
> I believe we should always expect it to be non-empty. SSLInfo::unverified_cert
> will be empty for resources that were loaded from cache, but since we are
> sending reports for pages that have certificate errors and we don't cache
> resources with certificate errors, this should always be filled. (That is, if
we
> are showing an interstitial page, I don't think the page could have been
loaded
> from cache.) Should I add a comment explaining that, or is too much detail?

Thanks for the explanation, I think it's okay as it is.
If this turns out to be the other way around, we'll eventually find it out :)

> 
>
https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
> File chrome/browser/ssl/certificate_error_report.cc (right):
> 
>
https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
> chrome/browser/ssl/certificate_error_report.cc:58: for (size_t i = 0; i <
> pem_encoded_chain.size(); ++i)
> On 2015/06/15 19:57:20, Mustafa Emre Acer wrote:
> > You can use C++11 style for loop here:
> > 
> > for (const string& pem: pem_encoded_chain)
> >   result->append(pem);
> > 
> > Or directly use JoinString from string_util.
> 
> Done. (JoinString)
> 
>
https://codereview.chromium.org/1180313006/diff/1/chrome/browser/ssl/certific...
> chrome/browser/ssl/certificate_error_report.cc:79: LOG(ERROR) << "Could not
get
> PEM encoded chain.";
> On 2015/06/15 19:57:20, Mustafa Emre Acer wrote:
> > Wrap this statement with braces since |if| spans two lines.
> 
> Done.

LGTM.

[estark, 2015-06-15 20:56:49]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-06-15 20:57:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180313006/20001

[commit-bot: I haz the power, 2015-06-15 22:15:48]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-15 22:17:47]

Patchset 2 (id:??) landed as
https://crrev.com/fcac4d5822ebb8b831a2125b1800e4cc65fe6b3d
Cr-Commit-Position: refs/heads/master@{#334472}

[phoglund_chromium, 2015-06-16 07:52:46]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1191743002/ by phoglund@chromium.org.

The reason for reverting is: Appears this breaks
CertificateErrorReportTest.SerializedReportAsProtobuf..

[estark, 2015-06-17 17:26:46]

Waiting on fix to SSLInfo before re-landing this: crrev.com/1188503004

[estark, 2015-06-17 23:50:39]

The CQ bit was checked by estark@chromium.org

[estark, 2015-06-17 23:50:39]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1180313006/#ps100001
(title: "revert testing changes")

[commit-bot: I haz the power, 2015-06-17 23:52:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1180313006/100001

[commit-bot: I haz the power, 2015-06-18 01:02:35]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-18 01:04:50]

Patchset 6 (id:??) landed as
https://crrev.com/2910d62cffd1c2fa949cf0b5e8417fa114f58491
Cr-Commit-Position: refs/heads/master@{#334975}