Work around our not caching the intermediate CA...

Work around our not caching the intermediate CA
certificates by passing the source of each OSCertHandle to
CreateFromHandle and the X509Certificate constructor.  If
the OSCertHandle comes from the network layer, we know it
has a complete certificate chain and therefore prefer it to
an OSCertHandle that comes from the HTTP cache, which
doesn't have the intermediate CA certificates.  A
certificate from the network layer can kick out a
certificate from the HTTP cache in our certificate cache.
This workaround seems good enough to fix all the known
symptoms of not caching the intermediate CA certificates.

Move the common code in x509_certificate_<os>.cc to
x509_certificate.cc.

R=eroman
BUG=3154, 7065
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8864

[wtc, 2009-01-27 02:16:14]

I will add a unit test, but please start reviewing the CL
now and ask questions if anything about the problem or the
solution is not clear.  Thanks.

[eroman, 2009-01-27 04:35:37]

lgtm once unit tested.

http://codereview.chromium.org/18836/diff/204/206
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/18836/diff/204/206#newcode133
Line 133: X509Certificate* cert =
cache->Find(CalculateFingerprint(cert_handle));
style-nit: I would suggest re-naming this to |cached_cert|.

http://codereview.chromium.org/18836/diff/204/206#newcode154
Line 154: X509Certificate::X509Certificate(std::string subject,
Could you change these to be |const std::string&| ?

http://codereview.chromium.org/18836/diff/204/206#newcode163
Line 163: source_(FROM_CACHE) {
It doesn't seem source_(FROM_CACHE) has any real meaning for this caller -- can
this be turned into a required parameter or left un-initialized?

http://codereview.chromium.org/18836/diff/204/210
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/18836/diff/204/210#newcode125
Line 125: static X509Certificate* CreateFromHandle(OSCertHandle cert_handle,
do you think it is worth describing |source| in the documentation of this
function?

http://codereview.chromium.org/18836/diff/204/212
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/18836/diff/204/212#newcode238
Line 238: return CreateFromHandle(cert_handle, SOURCE_CACHE);
Can Source be added as a parameter of CreateFromBytes() and CreateFromPickle()
instead?

Since their names don't imply anything about where the data came from, the
defaulting is a bit awkward. I think it will be clearer to see for example
http_cache.cc writing:
  X509Certificate::CreateFromPickle(pickle, &iter, SOURCE_CACHE);

[wtc, 2009-01-28 02:51:47]

Thanks for the review.  I addressed all of your review
comments in Patch Set 3.  You can wait for the new Patch
Set with the unit test to review again.

Note that I added the Source enum FROM_UNUSED for the
dummy certificates (which don't use their source_ field),
and renamed FROM_CACHE to FROM_LONE_CERT_IMPORT.  Please
let me know if you like these changes.

[eroman, 2009-01-28 03:20:44]

looks good

[wtc, 2009-01-28 22:17:26]

I added a unit test in x509_certificate_unittest.cc.
Please review the new Patch Set.

The unit test requires creating an OS certificate handle
from bytes.  So I extracted a new static method
CreateOSCertHandleFromBytes from CreateFromBytes.

To make FRIEND_TEST work in x509_certificate.h, I had to
put the new unit test in the net namespace.  So I just put
all the unit tests in x509_certificate_unittest.cc in the
net namespace.  I also removed the unused "test fixture"
X509CertificateTest.

[wtc, 2009-01-28 22:37:31]

Do you think the Source enum constants would look nicer
if they all begin with SOURCE_ instead of FROM_?  I.e.,

FROM_UNUSED => SOURCE_UNUSED
FROM_LONE_CERT_IMPORT => SOURCE_LONE_CERT_IMPORT
FROM_NETWORK => SOURCE_FROM_NETWORK

[eroman, 2009-01-28 22:42:34]

> Do you think the Source enum constants would look nicer
> if they all begin with SOURCE_ instead of FROM_?  I.e.,
> 
> FROM_UNUSED => SOURCE_UNUSED
> FROM_LONE_CERT_IMPORT => SOURCE_LONE_CERT_IMPORT
> FROM_NETWORK => SOURCE_FROM_NETWORK

Yes.

[wtc, 2009-01-28 23:07:40]

OK, please review the new Patch Set which has the renamed
Source enum constants.

[eroman, 2009-01-28 23:20:05]

unit test looks good.

http://codereview.chromium.org/18836/diff/419/631
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/18836/diff/419/631#newcode191
Line 191: friend class base::RefCountedThreadSafe<X509Certificate>;
why is this needed?

Is it because of the lines ?
EXPECT_EQ(cert1, cert2);

If that is the case, how about:
EXPECT_EQ(cert1.get(), cert2.get())

http://codereview.chromium.org/18836/diff/419/631#newcode230
Line 230: // Returns NULL on failure.
Is OSCertHandle always a pointer type on all platforms? (If yes cool; if not
NULL be a bit awkward).

http://codereview.chromium.org/18836/diff/419/630
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/18836/diff/419/630#newcode208
Line 208: der_cert.data = reinterpret_cast<unsigned char *>(const_cast<char
*>(data));
style nit: I think the * should be flush against "char".

[wtc, 2009-01-28 23:39:17]

http://codereview.chromium.org/18836/diff/419/631
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/18836/diff/419/631#newcode191
Line 191: friend class base::RefCountedThreadSafe<X509Certificate>;
The line
  friend class base::RefCountedThreadSafe<X509Certificate>;
is pre-existing.  I merely moved it up to follow the order
of declaration in the Style Guide.  It used to be right
above the destructor at line 205, so I guess we need it so
that the RefCountedThreadSafe template can invoke the
private destructor.

http://codereview.chromium.org/18836/diff/419/631#newcode230
Line 230: // Returns NULL on failure.
Yes, OSCertHandle is a pointer type on all platforms.

Want me to add a comment to explicitly state this assumption?

http://codereview.chromium.org/18836/diff/419/630
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/18836/diff/419/630#newcode208
Line 208: der_cert.data = reinterpret_cast<unsigned char *>(const_cast<char
*>(data));
On 2009/01/28 23:20:05, eroman wrote:
> style nit: I think the * should be flush against "char".

Fixed in the new Patch Set.