Move X509Certificate::Verify into CertVerifyProc

Move X509Certificate::Verify into CertVerifyProc 

With this split, CertVerifyProc is responsible for
interacting with the underlying PKIX path building and
verification library, while X509Certificate is responsible
for parsing certificates with the underlying crypto library
and exposing a common interface for higher-level code such
as UI.

BUG=114343
TEST=net_unittests 
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128172

[Ryan Sleevi, 2012-03-14 00:36:11]

This change moves all of the logic for certificate verification out of
X509Certificate.

This is largely a copy-paste, but I've highlighted a few changes with
annotations.

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

http://codereview.chromium.org/9691054/diff/2001/net/base/cert_verify_proc.h#...
net/base/cert_verify_proc.h:81: const std::vector<SHA1Fingerprint>&
public_key_hashes);
I don't feel strongly that these belong here. My personal take is that these
seem more appropriate as part of some static CRLSet (or CRLSet-like) class.

IsBlacklisted() checks only the serial number (rather than serial number &
issuer SPKI of CRLSet::CheckSerial), and IsPublicKeyBlacklisted is functionally
equivalent to CRLSet::CheckSPKI, except SHA-1 vs SHA-256.

I'd like to revisit that in a follow-up.

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_cert_types.h#n...
net/base/x509_cert_types.h:40: class NET_EXPORT SHA1FingerprintLessThan {
The only reasons these aren't breaking anything is because they're entirely
inlined. But that creates duplicate definitions in the various modules. So this
is a slight clean-up being slipped in here, but it's relevant for
IsSHA1HashInSortedArray

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_cert_types.h#n...
net/base/x509_cert_types.h:50: bool NET_EXPORT IsSHA1HashInSortedArray(const
SHA1Fingerprint& hash,
This doesn't need to be NET_EXPORT, and I feel like it might even be better
(read: safer) by cheating with a integer-array template

eg:

template<size_t N>
bool IsSHA1HashInSortedArray(const SHA1Fingerprint& hash,
                             const uint8[20] (&array)[N]) {
}

(Using template size deduction ala arraysize). But that's for a later point.

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (left):

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:599: CertVerifyResult* verify_result) const {
This is now CertVerifyProc::Verify().

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (left):

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:298: CFMutableArrayRef policies);
These have moved to x509_util_mac.h. The reason these were exposed was because
the UI needed access to these. If/when we switch to Chrome UI, instead of native
UI, these could be fully moved as internal methods in an unnamed namespace
within cert_verify_proc_mac.cc

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:513: static void ResetCertStore();
Right now the OpenSSL Init singletons are tightly coupled (originally, they were
loose). Subsequent work will remove this from OpenSSL, but since that involves
more non-trivial work, I've held off of that for now.

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:549: 
These were moved into CertVerifyProc (for now).

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:554: size_t array_byte_len);
This was moved to x509_cert_types.h, as it's really a utility function for
dealing with SHA1Fingerprints.

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (left):

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:137: class CSSMFieldValue {
This has been moved to net/base/x509_util_mac.h. This matches the behaviour of
x509_util_openssl.h of containing 'parse helpers' that may be shared between
code.

They are not NET_EXPORTed, as they're only internal to net/base code
(cert_verify_proc_mac & x509_certificate_mac)

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

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:441: TEST(X509CertificateTest,
DigiNotarCerts) {
If IsPublicKeyBlacklisted truly belongs on X509Certificate, this doesn't need to
move.

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_util_mac.cc
File net/base/x509_util_mac.cc (right):

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_util_mac.cc#ne...
net/base/x509_util_mac.cc:59: SecPolicyRef* policy) {
I debated whether to combine create SSLClientPolicy and CreateSSLServerPolicy as
CreateSSLPolicy(bool client, const std::string& hostname, ...), but considering
the trivial size of the function and how they'll be optimized (just struct
initialization and a jump), it seems like a minimal cost for improved
readibility.

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_util_mac.h
File net/base/x509_util_mac.h (right):

http://codereview.chromium.org/9691054/diff/2001/net/base/x509_util_mac.h#new...
net/base/x509_util_mac.h:51: 
The above are all exposed as NET_EXPORT because the UI code may need to create
these policies.

The below are all internal to net/base/

[wtc, 2012-03-16 00:48:25]

Patch set 6 LGTM.  Good refactoring, but it's too bad we'll
have a gap in SVN history.

http://codereview.chromium.org/9691054/diff/10009/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/9691054/diff/10009/net/base/x509_certificate.h...
net/base/x509_certificate.h:246: const std::string& serial_number() const {
return serial_number_; }

Nit: list serial_number() before subject() and issuer() to
roughly match their order in a certificate.

At least list serial_number() before fingerprint().

[Ryan Sleevi, 2012-03-16 00:52:02]

On 2012/03/16 00:48:25, wtc wrote:
> Patch set 6 LGTM.  Good refactoring, but it's too bad we'll
> have a gap in SVN history.

Great, thanks!

When I land these patchsets, I'll actually land through SVN, since I agree, it's
important to preserve SVN history. I've just been using git as the most
efficient way to track the other work going on (by you, palmer, and agl)

[Nico, 2012-03-22 04:37:40]

cocoa change lgtm