Rewrite X509Certificate::SupportsSSLClientAuth to be more accurate

Rewrite X509Certificate::SupportsSSLClientAuth to be more accurate

We take the intersection of key usage and extended key usage and treat as an
allow if missing. Also, support the "Any" purpose. Removed support for the
Netscape-specific ns-cert-type extension. Internet Explorer doesn't seem to
support it anyway.

BUG=45353
TEST=SSL client auth displays correct certificates on Mac (e.g. on foaf.me)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52189

[David Benjamin, 2010-07-08 19:21:58]

This probably wants unit tests of some sort. How do we normally deal with certs
for testing? Just sign up for a bunch of test certs or make our own...?

[agl, 2010-07-08 21:04:12]

LGTM

[wtc, 2010-07-08 22:17:08]

LGTM.

The description of the CL has a typo: the function you rewrote is
X509Certificate::SupportsSSLClientAuth, not
X509Certificate::SupportsSSLClientCertificate.

http://crbug.com/45353 is relevant to this CL.  You can add 45353 to
the BUG= field, and ask the people in that bug report to test the new
Dev channel release with this CL.

http://codereview.chromium.org/2899005/diff/1/2
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2899005/diff/1/2#newcode698
net/base/x509_certificate_mac.cc:698: namespace {
Our convention is to put this unnamed namespace at the top of a file, before
the net namespace.

http://codereview.chromium.org/2899005/diff/1/2#newcode702
net/base/x509_certificate_mac.cc:702: bool ExtendedKeyUsageAllows(const
CE_ExtendedKeyUsage* usage,
If a cert has a present but empty extended key usage extension,  this
function will return false.  I think that's the correct interpretation of an
empty extended key usage.

http://codereview.chromium.org/2899005/diff/1/2#newcode715
net/base/x509_certificate_mac.cc:715: bool
X509Certificate::SupportsSSLClientAuth() const {
You should document two things about this function:
1. It does not support CSSMOID_NetscapeCertType.
2. It does not use a cert with non-repudiation key usage for SSL client auth.

http://codereview.chromium.org/2899005/diff/1/2#newcode736
net/base/x509_certificate_mac.cc:736: // RFC5280 says to take the intersection
of the two checks.
Nit: checks => extensions

[David Benjamin, 2010-07-08 22:45:31]

Changed the description.

http://codereview.chromium.org/2899005/diff/1/2
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2899005/diff/1/2#newcode698
net/base/x509_certificate_mac.cc:698: namespace {
On 2010/07/08 22:17:09, wtc wrote:
> Our convention is to put this unnamed namespace at the top of a file, before
> the net namespace.

Done.

http://codereview.chromium.org/2899005/diff/1/2#newcode702
net/base/x509_certificate_mac.cc:702: bool ExtendedKeyUsageAllows(const
CE_ExtendedKeyUsage* usage,
On 2010/07/08 22:17:09, wtc wrote:
> If a cert has a present but empty extended key usage extension,  this
> function will return false.  I think that's the correct interpretation of an
> empty extended key usage.

Yeah. I don't see a note either way in the RFC, but that should be correct. It
looks like it's what NSS does. Do you want me to add a comment to that effect?

http://codereview.chromium.org/2899005/diff/1/2#newcode715
net/base/x509_certificate_mac.cc:715: bool
X509Certificate::SupportsSSLClientAuth() const {
On 2010/07/08 22:17:09, wtc wrote:
> You should document two things about this function:
> 1. It does not support CSSMOID_NetscapeCertType.

Done.

> 2. It does not use a cert with non-repudiation key usage for SSL client auth.

Is that actually true? I guess in practice, yes. But if someone has a cert with
the non-repudiation key usage bit as well as the relevant bits and OIDs for SSL
client auth, we'll use it. We probably should too in that case.

http://codereview.chromium.org/2899005/diff/1/2#newcode736
net/base/x509_certificate_mac.cc:736: // RFC5280 says to take the intersection
of the two checks.
On 2010/07/08 22:17:09, wtc wrote:
> Nit: checks => extensions

Done.

[wtc, 2010-07-08 22:57:49]

LGTM.

http://codereview.chromium.org/2899005/diff/1/2
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/2899005/diff/1/2#newcode702
net/base/x509_certificate_mac.cc:702: bool ExtendedKeyUsageAllows(const
CE_ExtendedKeyUsage* usage,
On 2010/07/08 22:45:31, David Benjamin wrote:
> Do you want me to add a comment to that effect?

No.  I just wanted you to doublecheck that issue.

http://codereview.chromium.org/2899005/diff/1/2#newcode715
net/base/x509_certificate_mac.cc:715: bool
X509Certificate::SupportsSSLClientAuth() const {
On 2010/07/08 22:45:31, David Benjamin wrote:
>
> But if someone has a cert with
> the non-repudiation key usage bit as well as the relevant bits and OIDs for
SSL
> client auth, we'll use it. We probably should too in that case.

That's correct.  But we need to point out that we won't use a cert whose
key usage extension has non-repudiation but not digital signature.  That's
the subject of issue 45353 and is therefore worth documenting.

http://codereview.chromium.org/2899005/diff/7001/8001#newcode720
net/base/x509_certificate_mac.cc:720: const CE_ExtendedKeyUsage* ext_key_usage =
NULL;
Nit: move this comment before line 717.