Make CertVerifier a pure virtual interface.

Make CertVerifier a pure virtual interface.

The existing CertVerifier implementation has been renamed to
MultiThreadedCertVerifier, consistent with ProxyResolver naming.

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

BUG=114343
TEST=Compiles and existing unittests pass.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128090

[Ryan Sleevi, 2012-02-28 04:43:12]

wtc: PTAL. This is in support of having runtime-defined CertVerifiers (eg:
libpkix on OS X, extensions).

Please review in the following order:
1) net/base/cert_verifier.h - This defines the minimal interface for a
CertVerifier. In a future CL, CRLSets will be removed from the core interface
and made a custom CertVerifier (eg: CRLSetCertVerifier)
2) net/base/multi_threaded_cert_verifier[.h, .cc, _unittest.cc] - This is a
simple rename of the existing CertVerifier class and logic, with the exception
that SingleRequestCertVerifier has been extracted into its own file.
3) net/base/single_request_cert_verifier[.h/cc] - This is a simple extraction
that was previously in cert_verifier.h

For steps 2 and 3, these are renames, so feel free to rubberstamp.

If this is suitable, I'll pursue getting the rubberstamps from the other OWNERS.
A future CL will seek to replace the explicit dependencies on
MultiThreadedCertVerifier with either a MockCertVerifier or a call to
GetDefaultCertVerifier(), as appropriate.

[wtc, 2012-02-29 20:27:49]

Review comments on Patch Set 1:

I only reviewed the cert_verifier related files in net/base.
This approach seems good.

Could you use "svn copy" to fork cert_verifier.h into
multi_threaded_cert_verifier.h and
single_request_cert_verifier.h and fork cert_verifier.cc
into multi_threaded_cert_verifier.cc and
single_request_cert_verifier.cc?

I have some suggested changes and questions below.

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h
File net/base/cert_verifier.h (left):

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#oldcode48
net/base/cert_verifier.h:48: // canceled, and their completion callbacks will
not be called.

I think this behavior is required for the destructor of
CertVerifier subclasses, so we should keep this comment,
right?

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

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#newcode30
net/base/cert_verifier.h:30: // Opaque pointer type, to return a handle to
cancel outstanding requests.

Nit: delete "to return"?  It sounds like the description of
a function, but this is a type.

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#newcode64
net/base/cert_verifier.h:64: // TODO(rsleevi): Move CRLSet* out of the
CertVerifier signature.

CertVerifier is also responsible for building the certification
path, but CRLSet is a certificate revocation checker.

If CRLSet is used by a separate CertVerifier, will that
CertVerifier have to repeat the certification path building?

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#newcode79
net/base/cert_verifier.h:79: CertVerifier() {}

Is it necessary to define the default constructor?

http://codereview.chromium.org/9476035/diff/1/net/base/single_request_cert_ve...
File net/base/single_request_cert_verifier.h (right):

http://codereview.chromium.org/9476035/diff/1/net/base/single_request_cert_ve...
net/base/single_request_cert_verifier.h:28: int Verify(X509Certificate* cert,

If the corresponding CertVerifier method is named VerifySslServer,
this method should have the same name.

But CertVerifier::VerifySslServer() seems like a typo or
a future change.

[Ryan Sleevi, 2012-02-29 21:51:33]

> Could you use "svn copy" to fork cert_verifier.h into
> multi_threaded_cert_verifier.h and
> single_request_cert_verifier.h and fork cert_verifier.cc
> into multi_threaded_cert_verifier.cc and
> single_request_cert_verifier.cc?

I'm currently using the NewGit workflow (
http://code.google.com/p/chromium/wiki/UsingNewGit ), which creates problems due
to the fact that git does not treat renames as part of the repository metadata
(it instead uses a fuzzy heuristic).

Even with --find-copies-harder / -M10%, it does not pick up that
multi_threaded_cert_verifier.h or single_request_cert_verifier.h originally came
from cert_verifier.h.

If you feel this is important metadata to preserve and is non-obvious, I would
be happy to re-create this patch on an SVN repository. I just wanted to confirm
with you before setting up an SVN checkout (I haven't yet used naked SVN it on
my Google machine)

Note that, as indicated in this patchset by the M+ (and viewable in the
"Download Raw Diff"), multi_threaded_cert_verifier.cc and _unittest.cc ARE
matched as renames from cert_verifier/_unittest.cc. It's only the headers and
single_request_cert_verifier.cc that are potentially problematic.

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h
File net/base/cert_verifier.h (left):

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#oldcode48
net/base/cert_verifier.h:48: // canceled, and their completion callbacks will
not be called.
On 2012/02/29 20:27:50, wtc wrote:
> 
> I think this behavior is required for the destructor of
> CertVerifier subclasses, so we should keep this comment,
> right?

Yes, done.

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

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#newcode64
net/base/cert_verifier.h:64: // TODO(rsleevi): Move CRLSet* out of the
CertVerifier signature.
On 2012/02/29 20:27:50, wtc wrote:
> 
> CertVerifier is also responsible for building the certification
> path, but CRLSet is a certificate revocation checker.
> 
> If CRLSet is used by a separate CertVerifier, will that
> CertVerifier have to repeat the certification path building?

No, that's something that I would want to avoid.

I think of every CertVerifier wanting two inputs - |cert| and the previous/input
|verify_result|. |cert| always represents the server chain, |verify_result| is
the constructed chain (from the previous verifier).

A future CL would seek to clarify and formalize this in the API. But since "path
building" is so directly tied to "path verification", it doesn't make sense to
have every CertVerifier repeat the same logic.

http://codereview.chromium.org/9476035/diff/1/net/base/cert_verifier.h#newcode79
net/base/cert_verifier.h:79: CertVerifier() {}
On 2012/02/29 20:27:50, wtc wrote:
> 
> Is it necessary to define the default constructor?

Since there are pure virtual methods, no. Removed.

http://codereview.chromium.org/9476035/diff/1/net/base/single_request_cert_ve...
File net/base/single_request_cert_verifier.h (right):

http://codereview.chromium.org/9476035/diff/1/net/base/single_request_cert_ve...
net/base/single_request_cert_verifier.h:28: int Verify(X509Certificate* cert,
On 2012/02/29 20:27:50, wtc wrote:
> 
> If the corresponding CertVerifier method is named VerifySslServer,
> this method should have the same name.
> 
> But CertVerifier::VerifySslServer() seems like a typo or
> a future change.

Yes, a typo of a (possible, undecided) future change. Fixed.

[Ryan Sleevi, 2012-03-02 18:39:07]

Stamp collecting:
sergeyu: jingle/, remoting/
darin: webkit/, content/


wtc: I rolled in the CreateDefaultCertVerifier() call to this CL, to avoid a
need for a second rubber-stamping. I wanted to avoid calling it
"CreateSystemCertVerifier()", since that may be confusing if/when we switch OS X
to use libpkix from NSS rather than Security.framework.

[wtc, 2012-03-02 23:51:52]

Patch set 3 LGTM.

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

http://codereview.chromium.org/9476035/diff/8001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:30: // Opaque pointer type used cancel outstanding
requests.

Nit: add "to" after "used".

http://codereview.chromium.org/9476035/diff/8001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:80: // Creates a CertVerifier implementation that
verifies certificates against

Nit: against => "using" or "with".

You told me CertVerifiers will be chained.  Is the default
CertVerifier always the first one in a chain?

http://codereview.chromium.org/9476035/diff/8001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:82: static NET_EXPORT CertVerifier*
CreateDefaultCertVerifier();

Nit: It seems self-contradictory for CertVerifier to be
NET_EXPORT_PRIVATE but CreateDefaultCertVerifier to be
NET_EXPORT.  It seems that they should have the same
export level.

Nit: since this method will be invoked with the
CertVerifier:: prefix, can the method name be shorted to
CreateDefault?

http://codereview.chromium.org/9476035/diff/8001/net/base/cert_verifier.h#new...
net/base/cert_verifier.h:85: CertVerifier() { }

The default constructor should be removed, right?  I don't
see it in other abstract classes in our source tree.

[Ryan Sleevi, 2012-03-06 02:06:13]

sergeyu, darin: ping

[Sergey Ulanov, 2012-03-06 02:16:06]

Didn't look through the whole CL, but the remoting changes LGTM! Thank you Ryan
for working on it!

[Ryan Sleevi, 2012-03-07 20:24:40]

darin: Any chance for a rubberstamp for webkit/ and content/ ?

[darin (slow to review), 2012-03-07 20:40:31]

LGTM for content/ and webkit/

[commit-bot: I haz the power, 2012-03-07 21:21:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/9476035/23013

[commit-bot: I haz the power, 2012-03-07 21:49:04]

Try job failure for 9476035-23013 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-20 19:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/9476035/50001