net: allow EV indication on Windows when the local OCSP/CRL cache is stale.

net/base/x509_certificate_unittest.cc

Index: net/base/x509_certificate_unittest.cc
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 25ddafaeb4817ab82aa552f071d18508b3fdf320..3ca6bc0e116b463825ed20569c2839fea49bcd55 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -31,17 +31,6 @@
 #include "base/mac/mac_util.h"
 #endif
 
-// Unit tests aren't allowed to access external resources. Unfortunately, to
-// properly verify the EV-ness of a cert, we need to check for its revocation
-// through online servers. If you're manually running unit tests, feel free to
-// turn this on to test EV certs. But leave it turned off for the automated
-// testing.
-#define ALLOW_EXTERNAL_ACCESS 0
-
-#if ALLOW_EXTERNAL_ACCESS && defined(OS_WIN)
-#define TEST_EV 1  // Test CERT_STATUS_IS_EV
-#endif
-
 using base::HexEncode;
 using base::Time;
 
@@ -221,16 +210,6 @@ void CheckGoogleCert(const scoped_refptr<X509Certificate>& google_cert,
   google_cert->GetDNSNames(&dns_names);
   ASSERT_EQ(1U, dns_names.size());
   EXPECT_EQ("www.google.com", dns_names[0]);
-
-#if TEST_EV

[agl, 2012/03/07 15:38:16]

These certificates have expired and so are no longer valid for testing in any
case.

[Ryan Sleevi, 2012/03/07 16:57:01]

Yeah, I noticed the same thing as well, and made the same changes in one of my
pending CLs. Fine to see these go.

-  // TODO(avi): turn this on for the Mac once EV checking is implemented.
-  CertVerifyResult verify_result;
-  int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
-                X509Certificate::VERIFY_EV_CERT;
-  EXPECT_EQ(OK, google_cert->Verify("www.google.com", flags, NULL,
-                                    &verify_result);
-  EXPECT_FALSE(verify_result.cert_status & CERT_STATUS_IS_EV);
-#endif
 }
 
 TEST(X509CertificateTest, GoogleCertParsing) {
@@ -290,14 +269,6 @@ TEST(X509CertificateTest, WebkitCertParsing) {
   EXPECT_EQ("*.webkit.org", dns_names[0]);
   EXPECT_EQ("webkit.org", dns_names[1]);
 
-#if TEST_EV
-  int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
-                X509Certificate::VERIFY_EV_CERT;
-  CertVerifyResult verify_result;
-  EXPECT_EQ(OK, webkit_cert->Verify("webkit.org", flags, NULL, &verify_result));
-  EXPECT_FALSE(verify_result.cert_status & CERT_STATUS_IS_EV);
-#endif
-
   // Test that the wildcard cert matches properly.
   EXPECT_TRUE(webkit_cert->VerifyNameMatch("www.webkit.org"));
   EXPECT_TRUE(webkit_cert->VerifyNameMatch("foo.webkit.org"));
@@ -370,22 +341,29 @@ TEST(X509CertificateTest, ThawteCertParsing) {
   thawte_cert->GetDNSNames(&dns_names);
   ASSERT_EQ(1U, dns_names.size());
   EXPECT_EQ("www.thawte.com", dns_names[0]);
+}
+
+TEST(X509CertificateTest, EVVerification) {
+  // This certificate will expire Jun 21, 2013.
+  CertificateList certs = CreateCertificateListFromFile(
+      GetTestCertsDirectory(),
+      "comodo.chain.pem",
+      X509Certificate::FORMAT_PEM_CERT_SEQUENCE);
+

[Ryan Sleevi, 2012/03/07 16:57:01]

ASSERT_EQ(3U, certs.size()) ? (I noticed this yesterday with some tests)

[agl, 2012/03/07 17:31:24]

Done.

+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(certs[1]->os_cert_handle());
+  intermediates.push_back(certs[2]->os_cert_handle());
+
+  scoped_refptr<X509Certificate> comodo_chain =
+      X509Certificate::CreateFromHandle(certs[0]->os_cert_handle(),
+                                        intermediates);
 
-#if TEST_EV
-  int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
-                X509Certificate::VERIFY_EV_CERT;
   CertVerifyResult verify_result;
-  // EV cert verification requires revocation checking.
-  EXPECT_EQ(OK, thawte_cert->Verify("www.thawte.com", flags, NULL,
-                                    &verify_result);
+  int flags = X509Certificate::VERIFY_EV_CERT;
+  int error = comodo_chain->Verify(
+      "comodo.com", flags, NULL, &verify_result);

[Ryan Sleevi, 2012/03/07 16:57:01]

In the past we didn't turn these on for the bots because they 'shouldn't' go out
to the network (the existing tests subtly fetch AIA, and that already can mask
issues)

With the changes you've made lately to revocation checking, do we know for sure
this won't hit the network on any platform? If so, great, otherwise I'd be a
little concerned.

[agl, 2012/03/07 17:31:24]

I don't believe that this will cause any network fetches. The chain is complete
and online revocation checking is off by default. If this does end up being
flaky because of an unexpected network fetch, then that's the sort of thing that
I'd like a test to highlight.

[wtc, 2012/03/08 01:02:20]

Hmm... if we report CERT_STATUS_IS_EV without getting a
fresh "good" status from any source -- CRL/OCSP (cached
or online) or CRL sets, then this is a step backward.

Does this unit test use the CRL set?

+  EXPECT_EQ(OK, error);
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_IS_EV);
-  // Consequently, if we don't have revocation checking enabled, we can't claim
-  // any cert is EV.
-  flags = X509Certificate::VERIFY_EV_CERT;
-  EXPECT_EQ(OK, thawte_cert->Verify("www.thawte.com", flags, NULL,
-                                    &verify_result));
-  EXPECT_FALSE(verify_result.cert_status & CERT_STATUS_IS_EV);
-#endif
 }
 
 // Test that all desired AttributeAndValue pairs can be extracted when only