Create stubs for system certificate validation.

Create stubs for system certificate validation.

Future work will shift code from X509Certificate::Verify to these classes.

This is patch 2 of N for http://crbug.com/114343

BUG=114343
TEST=Existing unittests

[Ryan Sleevi, 2012-03-03 05:01:50]

wtc: PTAL.

I split this stub work out so that I can break up each OS-specific extraction
into its own CL, which should make it easier to review.

This abuses friend classes for now, but is temporary while code is moved around.
All the existing functionality (mod the comment below) is retained.

Note, the distinction between a CertVerifier and a
MultiThreadedCertVerifier::VerifyProc is that CertVerifier is non-thread-safe
(lives on the IO thread), while the VerifyProc MUST be thread safe (because it
runs on non-joinable worker threads).

This distinction is important not just for the MultiThreadedCertVerifier, but
also for the (future) NetworkDelegateCertVerifier, as NetworkDelegate's MUST
live on the IO thread as well. It's not important for CRLSets/Cert Pinning, but
those are small and trivial to begin with.

http://codereview.chromium.org/9584041/diff/1/net/base/single_request_cert_ve...
File net/base/single_request_cert_verifier.cc (left):

http://codereview.chromium.org/9584041/diff/1/net/base/single_request_cert_ve...
net/base/single_request_cert_verifier.cc:38: // Do a synchronous verification.
I checked every user of SingleRequestCertVerifier, and they all provide
callbacks. Plus, I believe SingleRequestCertVerifier should be ignorant about
sync vs async here.

[wtc, 2012-03-06 23:10:14]

Patch Set 3 LGTM.

http://codereview.chromium.org/9584041/diff/1/net/base/single_request_cert_ve...
File net/base/single_request_cert_verifier.cc (left):

http://codereview.chromium.org/9584041/diff/1/net/base/single_request_cert_ve...
net/base/single_request_cert_verifier.cc:38: // Do a synchronous verification.

I guess we can replace this with a DCHECK?  The code below
won't work if cert_verifier_->Verify() returns ERR_IO_PENDING
but callback is null.

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verifier.cc
File net/base/cert_verifier.cc (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verifier.cc#ne...
net/base/cert_verifier.cc:7: #include "build/build_config.h"

The "build/build_config.h" file can be listed in the standard
alphabetical order.  See the example in Chromium Coding Style:
http://www.chromium.org/developers/coding-style#TOC-Platform-defines

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.

If I click the "Side-by-side diffs" link for this file,
I see no changes.

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_mac.h
File net/base/cert_verify_proc_mac.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_ma...
net/base/cert_verify_proc_mac.h:14: // using OS X's Security.framework for path
building and verification.

Nit: path => certification path

This is for the benefit of people unfamiliar with
certificates.

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_win.h
File net/base/cert_verify_proc_win.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_wi...
net/base/cert_verify_proc_win.h:27: virtual ~CertVerifyProcWin();

Why do we need to make the destructor protected?

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
File net/base/multi_threaded_cert_verifier.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
net/base/multi_threaded_cert_verifier.h:31: NON_EXPORTED_BASE(public
base::NonThreadSafe),

Maybe NON_EXPORTED_BASE can be removed now?

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
net/base/multi_threaded_cert_verifier.h:41: class VerifyProc : public
base::RefCountedThreadSafe<VerifyProc> {

Why does VerifyProc need to be a class?  Isn't it a
function?  None of the concrete subclasses of VerifyProc
in this CL have data members.

Why does the VerifyProc class need to be ref-counted?

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
net/base/multi_threaded_cert_verifier.h:156: scoped_refptr<VerifyProc>
verifier_;

Please name this member verify_proc_.  verifier_ suggests
it's a CertVerifier.

Make the same change to the "VerifyProc* verifier" argument
to the constructor on line 60 above.

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
File net/base/multi_threaded_cert_verifier_unittest.cc (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
net/base/multi_threaded_cert_verifier_unittest.cc:31: virtual int
Verify(X509Certificate* certificate,

Nit: shorten 'certificate' to 'cert'?

http://codereview.chromium.org/9584041/diff/6001/net/base/transport_security_...
File net/base/transport_security_state_unittest.cc (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/transport_security_...
net/base/transport_security_state_unittest.cc:23: #include
"net/base/test_completion_callback.h"

Nit: ordering is wrong.

http://codereview.chromium.org/9584041/diff/6001/net/base/transport_security_...
net/base/transport_security_state_unittest.cc:303: rv =
callback.WaitForResult();

Nit: there is a method (GetResult?) that is equivalent to
these two lines.

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (left):

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:233: #endif

Why did you delete this #if TEST_EV block but keep the other
ones?

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:238: return cert->Verify(hostname, flags,
crl_set, verify_result);

Why is this fixture necessary?  Are you going to change
the Verify() method to call a CertVerifier or
VerifyProc?

http://codereview.chromium.org/9584041/diff/6001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/9584041/diff/6001/net/net.gyp#newcode936
net/net.gyp:936: [ 'use_nss!=1', {

Nit: the neighboring code uses spaces around == and !=,
so it would be nice to follow the style.

[Ryan Sleevi, 2012-03-10 03:09:11]

wtc: PTAL at Patchset 5. Since the interface significantly changed based on your
review feedback, I wanted to take the opportunity to make another pass with you.

The reference query for places using function pointers (excluding various bits
of third-party code)

http://code.google.com/p/chromium/source/search?q=typedef%5Cs%5Cw*%5Cs%5C%28%...


Note that many of these are system API wrapper functions or part of the content/
API.

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verifier.h
File net/base/cert_verifier.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2012/03/06 23:10:14, wtc wrote:
> 
> If I click the "Side-by-side diffs" link for this file,
> I see no changes.

Yeah, not sure why this was showing up. It's disappeared in subsequent
patchsets.

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
File net/base/multi_threaded_cert_verifier.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
net/base/multi_threaded_cert_verifier.h:31: NON_EXPORTED_BASE(public
base::NonThreadSafe),
On 2012/03/06 23:10:14, wtc wrote:
> 
> Maybe NON_EXPORTED_BASE can be removed now?

The removal of NET_EXPORT was a mistake. It should be NET_EXPORT_PRIVATE so that
it can be unit tested.

As a result, per
http://src.chromium.org/viewvc/chrome/trunk/src/base/threading/non_thread_saf...
, this is still needed.

http://codereview.chromium.org/9584041/diff/6001/net/base/multi_threaded_cert...
net/base/multi_threaded_cert_verifier.h:41: class VerifyProc : public
base::RefCountedThreadSafe<VerifyProc> {
On 2012/03/06 23:10:14, wtc wrote:
> 
> Why does VerifyProc need to be a class?  Isn't it a
> function?  None of the concrete subclasses of VerifyProc
> in this CL have data members.

Using function pointers outside of PPAPI and unittests seems to be exceedingly
rare in Chromium code. Further, by making something a function pointer, you
guarantee that no state can ever be used. It's one thing to say a function must
be thread safe and not touch MessageLoops - but quite another to say it can
never use any variables.

That said, considering the end goal is to have all the Mocking logic at the
CertVerifier layer, and not at the VerifyProc layer, I went ahead and split it
into function pointers.

> 
> Why does the VerifyProc class need to be ref-counted?

VerifyProc's class lifetime needs to be as long as any of the worker threads -
as the worker threads need to be able to call into it. The alternative to making
the VerifyProc thread-safe would have been to have an inner "Core" class that
could be detached from the main IO thread when the MTCV went out of scope. While
functionally equivalent, forcing VerifyProc to be RCTS is a way to ensure
clients' consider thread-safety as inherent to the class.

That said, in light of the above comments, it's a non-issue.

Note: See also HostResolverProc, which is a class that is RCTS.

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (left):

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:233: #endif
On 2012/03/06 23:10:14, wtc wrote:
> 
> Why did you delete this #if TEST_EV block but keep the other
> ones?

Missed the other ones, but Adam has already nuked them in master, so all good.

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:238: return cert->Verify(hostname, flags,
crl_set, verify_result);
On 2012/03/06 23:10:14, wtc wrote:
> 
> Why is this fixture necessary?  Are you going to change
> the Verify() method to call a CertVerifier or
> VerifyProc?

To make it easier to friend this in X509Certificate while making Verify()
private [to avoid new callers being introduced]. New callers during the
refactoring should use the CertVerifier interface directly.

[Ryan Sleevi, 2012-03-12 23:07:51]

wtc: PTAL. It's easiest to review by selecting the diff against Patchset 3,
which is what you previously reviewed.

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:554: scoped_refptr<CertVerifyProcNSS>
nss_verify_proc(new CertVerifyProcNSS());
This code directly uses CertVerifyProcNSS as it's directly tied to NSS
verification. Hence why it's not using CreateDefault or equivalent.

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.h
File net/base/cert_verify_proc.h (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.h...
net/base/cert_verify_proc.h:55: 
At some point, we'll want to verify CA/Root certificates for import
(application/x-x509-ca-cert / application/x-x509-root-cert), which will be
additional Verify() calls.

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.h...
net/base/cert_verify_proc.h:65: virtual int VerifyInternal(X509Certificate*
cert,
I debated about making CertVerifyProc a pure virtual and then having
CertVerifyProcBase, which implemented the Verify/VerifyInternal split, but that
seemed needlessly complex for what is ultimately a temporary solution.

The mapping is X509Certificate::Verify() -> CertVerifyProc::Verify() and
X509Certificate::VerifyInternal() -> CertVerifyProc::VerifyInternal().

Speaking longer term CertVerifyProc::Verify() (aka X509Certificate::Verify())
could/should be implemented as its own CertVerifier, since that conceptually
matches what it does (high level checks and blacklisting). However, to avoid
performance concerns (and I can talk more about them if needed), I wanted to
match the existing X509Certificate:: functionality first before implementing new
functionality.

[wtc, 2012-03-13 22:37:57]

Patch set 8 LGTM.

It took me a while to figure our what you meant by MTCV and
RCTS.

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_mac.h
File net/base/cert_verify_proc_mac.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_ma...
net/base/cert_verify_proc_mac.h:7: #pragma once

Why doesn't this header need #pragma once?  Same question for
the other cert_verify_proc_xxx.h headers.

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.cc
File net/base/cert_verify_proc.cc (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.c...
net/base/cert_verify_proc.cc:11: #include "net/base/cert_verify_proc_win.h"

Nit: it seems that these platform-specific headers should
be included inside ifdefs.

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.c...
net/base/cert_verify_proc.cc:39: return VerifyInternal(cert, hostname, flags,
crl_set, verify_result);

Nit: the purpose of the Verify/VerifyInternal split is not
clear to me.

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.h
File net/base/cert_verify_proc.h (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc.h...
net/base/cert_verify_proc.h:29: // Verifies the certificate against the given
hostname.  Returns OK if

Should we say "Verifies ... as an SSL server certificate"?

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc_o...
File net/base/cert_verify_proc_openssl.h (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/cert_verify_proc_o...
net/base/cert_verify_proc_openssl.h:13: // CryptoAPI.

Typo: Windows' CryptoAPI => OpenSSL

http://codereview.chromium.org/9584041/diff/28020/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.h (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.h:127: void
SetCertVerifyProc(CertVerifyProc* verify_proc);

Nit: this probably can be named set_cert_verify_proc().

[Ryan Sleevi, 2012-03-14 00:58:49]

Thanks for the feedback. A few explanations below, but no need to re-review.

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_mac.h
File net/base/cert_verify_proc_mac.h (right):

http://codereview.chromium.org/9584041/diff/6001/net/base/cert_verify_proc_ma...
net/base/cert_verify_proc_mac.h:7: #pragma once
On 2012/03/13 22:37:57, wtc wrote:
> 
> Why doesn't this header need #pragma once?  Same question for
> the other cert_verify_proc_xxx.h headers.

It does. This was an oversight. Thanks for catching.

http://codereview.chromium.org/9584041/diff/28020/net/base/multi_threaded_cer...
File net/base/multi_threaded_cert_verifier.h (right):

http://codereview.chromium.org/9584041/diff/28020/net/base/multi_threaded_cer...
net/base/multi_threaded_cert_verifier.h:127: void
SetCertVerifyProc(CertVerifyProc* verify_proc);
On 2012/03/13 22:37:57, wtc wrote:
> 
> Nit: this probably can be named set_cert_verify_proc().

Since this wasn't inlined, I followed the MethodNaming.

The reason why this wasn't inlined was because doing { verify_proc_ =
verify_proc; } requires a full definition of CertVerifyProc, but I've only
forward declared it here (line 30).

[wtc, 2012-03-14 21:25:58]

Patch set 9 LGTM!