Include both certificate chains in invalid cert reporting

Include both certificate chains in invalid cert reporting

The report now includes the chain as received by the client from the
server as well as the verified chain built by the client.

This is done by adding an |unverified_server_cert| field to SSLInfo
(which replaces SSLClientSocket::GetUnverifiedServerCertificateChain(),
which was only used by unit tests). My understanding is that the |cert|
field contains the chain as verified by the client, or as received by
the client if a verified chain can't be built. So in some cases the
|cert| and |unverified_server_cert| fields might be the same.

This CL is based on https://codereview.chromium.org/935663004/

BUG=461588

[estark, 2015-02-21 06:42:22]

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

[estark, 2015-02-21 06:42:23]

felt@, could you please take a look? This was the CL that I was talking about
that could be way way off. :-)

https://codereview.chromium.org/949633002/diff/1/chrome/browser/net/chrome_fr...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.h (right):

https://codereview.chromium.org/949633002/diff/1/chrome/browser/net/chrome_fr...
chrome/browser/net/chrome_fraudulent_certificate_reporter.h:49: static
std::string BuildReport(
(I just made this a static method on the class so that I could write unit tests
for it.)

https://codereview.chromium.org/949633002/diff/1/chrome/browser/net/chrome_fr...
File chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc
(right):

https://codereview.chromium.org/949633002/diff/1/chrome/browser/net/chrome_fr...
chrome/browser/net/chrome_fraudulent_certificate_reporter_unittest.cc:157:
static std::string BuildReportPublic(ReportType type,
Is this a weird pattern and/or weird name? My goal here is to expose
ChromeFraudulentCertificateReporter::BuildReport to tests without making it
public.

[estark, 2015-02-23 19:36:59]

Patchset #3 (id:40001) has been deleted

[felt, 2015-02-25 16:41:32]

felt@chromium.org changed reviewers:
+ rsleevi@chromium.org

[felt, 2015-02-25 16:41:33]

Looks right to me % a few nits. Adding Sleevi who might have more opinions.

https://codereview.chromium.org/949633002/diff/60001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_fraudulent_certificate_reporter.cc (right):

https://codereview.chromium.org/949633002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:48: if (!cert ||
!cert->GetPEMEncodedChain(&pem_encoded_chain)) {
is it possible and expected that this can be called with !cert? (maybe this can
happen for INVALID_CERT errors, but I am not sure; have you checked?) should
this be a DCHECK making sure that the cert has a value instead?

https://codereview.chromium.org/949633002/diff/60001/chrome/browser/net/chrom...
chrome/browser/net/chrome_fraudulent_certificate_reporter.cc:49: LOG(ERROR) <<
"Could not get PEM encoded chain.";
should there also be a return here?

[Ryan Sleevi, 2015-02-25 23:15:41]

Not LGTM, unfortunately. I don't have a good suggestion (yet) on how to wire
this up, I still need to noodle about it.

But sending feedback early to start the dialog.

https://codereview.chromium.org/949633002/diff/60001/net/cert/cert_verify_res...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/949633002/diff/60001/net/cert/cert_verify_res...
net/cert/cert_verify_result.h:41: scoped_refptr<X509Certificate>
unverified_server_cert;
This doesn't seem like it belongs in the CertVerifyResult, as it's a function of
the socket, not verification.

https://codereview.chromium.org/949633002/diff/60001/net/ssl/ssl_info.h
File net/ssl/ssl_info.h (right):

https://codereview.chromium.org/949633002/diff/60001/net/ssl/ssl_info.h#newco...
net/ssl/ssl_info.h:53: scoped_refptr<X509Certificate> unverified_server_cert;
the _server_ seems redundant here.

https://codereview.chromium.org/949633002/diff/60001/net/ssl/ssl_info.h#newco...
net/ssl/ssl_info.h:53: scoped_refptr<X509Certificate> unverified_server_cert;
There's a problem here, which is that HttpResponseInfo contains an SSLInfo

HttpResponseInfo is our entry point into a responses information, and it's also
a stable object persisted to disk. That is, when synthesizing responses from the
cache, all of an HttpResponseInfo must be filled "as if" it was received over
the network.

This creates a problem, in that
1) Existing cached resources won't contain this information
2) We'll now have to start caching these unverified certs to disk (in addition
to the verified cert) in order to meet the //net API contracts that disk-served
resources are 'as good as' their from-the-network counterparts.

Thus, |unverified_server_cert| can be NULL, and a fair bit of the rest of the CL
falls over on the assumption that it must never be null.

[Ryan Sleevi, 2015-05-07 01:14:33]

Hey Emily, with my Not LG, it sounds like there's going to be some other work to
do first before getting to this.

Any concerns with closing out this CL for now until we resolve some of the
transient issues?

[estark, 2015-05-07 01:16:36]

On 2015/05/07 01:14:33, Ryan Sleevi wrote:
> Hey Emily, with my Not LG, it sounds like there's going to be some other work
to
> do first before getting to this.
> 
> Any concerns with closing out this CL for now until we resolve some of the
> transient issues?

Sure, that's fine by me.