Add a histogram for measuring the number of times we fall back to common name matching, when a cert…

Add a histogram for measuring the number of times we fall back to common name matching, when a certificate lacks a subjectAltName

BUG=308330
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230679

[Ryan Sleevi, 2013-10-17 00:50:10]

jar, since you own both net/ and tools/metrics

[jar (doing other things), 2013-10-17 16:44:44]

LGTM  % nit in xml

https://codereview.chromium.org/27624002/diff/1/tools/metrics/histograms/hist...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/27624002/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:6730: +    Whether certificate hostname
matching had to fall back to using the
nit: I'm not sure what "success" or "failed" would mean... which case it would
stand  for.

You should probably either illuminate that in the prose here (putting the words
in quotes for teh given case.... or probably better... declare you own enum with
tags like "fallback to Common Name"  vs "use subjectAltName"

[Ryan Sleevi, 2013-10-21 19:35:41]

jar: Requesting a re-review, because in discussions it was pointed out that we
may see different rates for publicly trusted vs internal CAs.

While the goal should be to fully deprecate the practice overall, I want to make
sure that hyper-inflated local CA #s don't prevent killing it from public CAs.
This will at least give a more accurate picture of how prevalent the bad
practice is.

wtc: Since this ends up plumbing a bit more through CertVerifyResult, I was
hoping you could take a quick look and make sure you're happy here.

[jar (doing other things), 2013-10-21 22:13:42]

Patch set 2 LGTM

tiny nit below.

https://codereview.chromium.org/27624002/diff/5001/net/cert/x509_certificate.cc
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/27624002/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.cc:532: *common_name_fallback_used =
common_name_fallback;
nit: indent

[wtc, 2013-10-23 22:39:26]

Patch set 3 LGTM.

https://codereview.chromium.org/27624002/diff/55001/net/cert/cert_verify_proc...
File net/cert/cert_verify_proc_nss.cc (right):

https://codereview.chromium.org/27624002/diff/55001/net/cert/cert_verify_proc...
net/cert/cert_verify_proc_nss.cc:766: // Make sure that the hostname matches
with the common name of the cert.

This comment should be updated or removed.

https://codereview.chromium.org/27624002/diff/55001/net/cert/cert_verify_resu...
File net/cert/cert_verify_result.h (right):

https://codereview.chromium.org/27624002/diff/55001/net/cert/cert_verify_resu...
net/cert/cert_verify_result.h:67: // name, rather than using the
subjectAlternativeName.

Nit: consider using "subjectAltName", the form used in RFC 5280.

https://codereview.chromium.org/27624002/diff/55001/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/27624002/diff/55001/net/cert/x509_certificate...
net/cert/x509_certificate.h:317: // if a fallback to the CN was used, rather
than subjectAlternativeName.

Nit: it should be clarified that |common_name_fallback_used| is set whether the
function returns true or false.

https://codereview.chromium.org/27624002/diff/55001/net/cert/x509_certificate...
net/cert/x509_certificate.h:429: // addresses, respectively.

Document the common_name_fallback_used output parameter.

https://codereview.chromium.org/27624002/diff/55001/net/cert/x509_certificate...
net/cert/x509_certificate.h:434: bool* common_name_fallback_used);

Having VerifyHostname return common_name_fallback_used is the most efficient
solution. However, a potentially more elegant API design could be a separate
method that returns whether the X509Certificate has a subject alternative name
extension. Then, it will be easy to remove when we are no longer interested in
this info.

[Ryan Sleevi, 2013-10-24 00:25:37]

https://codereview.chromium.org/27624002/diff/55001/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (right):

https://codereview.chromium.org/27624002/diff/55001/net/cert/x509_certificate...
net/cert/x509_certificate.h:434: bool* common_name_fallback_used);
On 2013/10/23 22:39:26, wtc wrote:
> 
> Having VerifyHostname return common_name_fallback_used is the most efficient
> solution. However, a potentially more elegant API design could be a separate
> method that returns whether the X509Certificate has a subject alternative name
> extension. Then, it will be easy to remove when we are no longer interested in
> this info.

We already expose GetSubjectAltName, which is a close approximation.

However, I wanted to keep this close to the actual verification code, so that
there's no risk of API drift. Having a separate method seems less than ideal,
short of making VerifyHostname also depend on it.

[commit-bot: I haz the power, 2013-10-24 00:30:42]

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

[commit-bot: I haz the power, 2013-10-24 00:49:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-10-24 00:56:47]

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

[commit-bot: I haz the power, 2013-10-24 08:14:17]

Change committed as 230679