Move certificate verification off the IO thread....

Move certificate verification off the IO thread.

Move the MapNetErrorToCertStatus and MapCertStatusToNetError
functions to cert_status_flags.h so they can be shared with
Mac and Linux code.

Move the certificate verification function to the
X509Certificate class.  Right now X509Certificate::Verify is
only implemented on Windows.

R=eroman
BUG=3592
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9084

[wtc, 2009-01-29 23:55:36]

The responsibility of each reviewer is:

 eroman: Please review every file.

 avi,dank: FYI.  Please review cert_status_flags.* and
 ssl_client_socket_mac.cc and ssl_client_socket_nss.cc.
 Also note that I didn't move certificate verification off
 the IO thread for Mac and Linux because we're still letting
 the system SSL libraries verify the certificates.

[wtc, 2009-01-30 00:03:18]

Notes to eroman:

Start the review with ssl_client_socket_win.h and
ssl_client_socket_win.cc.  This CL depends on the
cert_verifier.{h,cc} files that I added in December.
I added them first to make try server work.  (They
were created with "svn cp host_resolver.{h,cc}".)

The changes to the other files are just to move large
blocks of code to their new homes in cert_status_flags
and x509_certificate.

[Avi (use Gerrit), 2009-01-30 00:05:43]

On 2009/01/29 23:55:36, wtc wrote:
>  avi,dank: FYI.  Please review cert_status_flags.* and
>  ssl_client_socket_mac.cc

Those look good to me.

[eroman, 2009-01-30 00:25:40]

nice cleanups!

Will look at cert_verifier.{cc,h} shortly

(I want to check the behavior of what happens if you delete the SSLClientSocket
while verification is in progress).

The rest looks good.

http://codereview.chromium.org/14915/diff/617/623
File net/base/cert_status_flags.cc (right):

http://codereview.chromium.org/14915/diff/617/623#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
2009?

http://codereview.chromium.org/14915/diff/617/623#newcode27
Line 27: NOTREACHED();
just curious, why is this NOTREACHED?
(I realize this is unchanged from original).

http://codereview.chromium.org/14915/diff/617/624
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/14915/diff/617/624#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
do we need to update the year range when modifying files? 
I seem to remember we don't need to, but just wanted to check.

http://codereview.chromium.org/14915/diff/617/622
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/14915/diff/617/622#newcode195
Line 195: // If rev_checking_enabled is true, certificate revocation checking is
|rev_checking_enabled|

http://codereview.chromium.org/14915/diff/617/626
File net/net_lib.scons (right):

http://codereview.chromium.org/14915/diff/617/626#newcode29
Line 29: 'base/cert_status_flags.cc',
I reckon we need "cert_verifier.cc" here too, for building scons on windows.

Probably doesn't hurt to add it in the platform agnostic list even though the
other platforms aren't using it.

[dank, 2009-01-30 01:27:41]

Linux part LGTM.

[eroman, 2009-01-30 01:52:43]

Reviewed cert_verifier.{cc,h}, looks good.

http://codereview.chromium.org/14915/diff/617/621
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/14915/diff/617/621#newcode703
Line 703: X509Certificate::CreateFromHandle(dup_cert,
I would recommend moving the duplication into CertVerifier rather than doing it
here. That way it is harder for consumers to get this wrong (and a bit easier to
read CertVerifier without wandering if the accesses to cert_ are safe).

http://codereview.chromium.org/14915/diff/617/621#newcode704
Line 704: X509Certificate::SOURCE_FROM_NETWORK);
minor nit: I guess this parameter doesn't really matter, do you think SOURCE_
UNUSED is better? Either way works for me.

http://codereview.chromium.org/14915/diff/617/620
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/14915/diff/617/620#newcode480
Line 480: LogConnectionTypeMetrics(chain_context);
UpdateConnectionTypeHistograms() may now get called from two threads: {IO
thread, verify thread} -- is it threadsafe?

[wtc, 2009-01-30 23:47:35]

I'll send you a new Patch Set later.  Let me respond to
your review comments first.

http://codereview.chromium.org/14915/diff/617/623
File net/base/cert_status_flags.cc (right):

http://codereview.chromium.org/14915/diff/617/623#newcode27
Line 27: NOTREACHED();
On 2009/01/30 00:25:40, eroman wrote:
> just curious, why is this NOTREACHED?
> (I realize this is unchanged from original).

I added a comment to explain why we should not use
ERR_CERT_CONTAINS_ERRORS.

http://codereview.chromium.org/14915/diff/617/621
File net/base/ssl_client_socket_win.cc (right):

http://codereview.chromium.org/14915/diff/617/621#newcode703
Line 703: X509Certificate::CreateFromHandle(dup_cert,
On 2009/01/30 01:52:43, eroman wrote:
> I would recommend moving the duplication into CertVerifier rather than doing
it
> here. That way it is harder for consumers to get this wrong (and a bit easier
to
> read CertVerifier without wandering if the accesses to cert_ are safe).

Actually CertVerifier already duplicates the cert so that
it can use the cert safely.  The cert_ member of
CertVerifier::Request is a scoped_refptr, so it duplicates
the cert when it is initialized.

But, CertVerifier duplicates a X509Certificate, whereas here
we duplicate a PCCERT_CONTEXT.  We need to duplicate
PCCERT_CONTEXT for X509Certificate::CreateFromHandle.

Is this clear?  I can explain this to you in person if you
have questions.

http://codereview.chromium.org/14915/diff/617/621#newcode704
Line 704: X509Certificate::SOURCE_FROM_NETWORK);
On 2009/01/30 01:52:43, eroman wrote:
> minor nit: I guess this parameter doesn't really matter, do you think SOURCE_
> UNUSED is better? Either way works for me.

This parameter matters here.  It has to be SOURCE_FROM_NETWORK because we get
the certificate from
the network (SSL).

http://codereview.chromium.org/14915/diff/617/620
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/14915/diff/617/620#newcode480
Line 480: LogConnectionTypeMetrics(chain_context);
On 2009/01/30 01:52:43, eroman wrote:
> UpdateConnectionTypeHistograms() may now get called from two threads: {IO
> thread, verify thread} -- is it threadsafe?

No.  It's good that you noticed this issue.  I was planning
to address it later, but since you brought it up, I will
address it in a new Patch Set.

[wtc, 2009-01-31 02:38:22]

avi,dank: You don't need to review this CL again.

eroman: Please review Patch Set 4.

The Patch Set you reviewed last time was Patch Set 3.
Note that Patch Set 4 has three more files:
  cert_verifier.cc
  cert_verifier.h
  cert_verify_result.h

To make LogConnectionTypeMetrics thread-safe, I had to split
it into two functions.  The portions that call
UpdateConnectionTypeHistograms have to remain in
SSLClientSocketWin so that we call
UpdateConnectionTypeHistograms on the IO thread.  The
rest becomes the PopulateCertChainInfo function in
x509_certificate_win.cc.  This requires me to add a new
type CertVerifyResult to hold both the original
X509Certificate::Verify result (cert_status) and all these
had_xxx boolean flags I need for
UpdateConnectionTypeHistograms.  This int* cert_status =>
CertVerifyResult* verify_result change has to be propagated
through several classes.

[wtc, 2009-01-31 03:15:24]

eroman: I just uploaded Patch Set 5.  Just some minor
comment fixes.

[eroman, 2009-02-02 20:42:29]

lgtm