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

net/base/x509_certificate_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/string_number_conversions.h"
@@ skipping 13 matching lines @@
 #if defined(USE_NSS)
 #include <cert.h>
 #endif
 
 #if defined(OS_WIN)
 #include "base/win/windows_version.h"
 #elif defined(OS_MACOSX)
 #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;
 
 namespace net {
 
 // Certificates for test data. They're obtained with:
 //
 // $ openssl s_client -connect [host]:443 -showcerts > /tmp/host.pem < /dev/null
 // $ openssl x509 -inform PEM -outform DER < /tmp/host.pem > /tmp/host.der
 //
@@ skipping 159 matching lines @@
   EXPECT_EQ(valid_to, valid_expiry.ToDoubleT());
 
   const SHA1Fingerprint& fingerprint = google_cert->fingerprint();
   for (size_t i = 0; i < 20; ++i)
     EXPECT_EQ(expected_fingerprint[i], fingerprint.data[i]);
 
   std::vector<std::string> dns_names;
   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) {
   scoped_refptr<X509Certificate> google_cert(
       X509Certificate::CreateFromBytes(
           reinterpret_cast<const char*>(google_der), sizeof(google_der)));
 
   CheckGoogleCert(google_cert, google_fingerprint,
                   1238192407,   // Mar 27 22:20:07 2009 GMT
                   1269728407);  // Mar 27 22:20:07 2010 GMT
@@ skipping 39 matching lines @@
   const SHA1Fingerprint& fingerprint = webkit_cert->fingerprint();
   for (size_t i = 0; i < 20; ++i)
     EXPECT_EQ(webkit_fingerprint[i], fingerprint.data[i]);
 
   std::vector<std::string> dns_names;
   webkit_cert->GetDNSNames(&dns_names);
   ASSERT_EQ(2U, dns_names.size());
   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"));
   EXPECT_TRUE(webkit_cert->VerifyNameMatch("webkit.org"));
   EXPECT_FALSE(webkit_cert->VerifyNameMatch("www.webkit.com"));
   EXPECT_FALSE(webkit_cert->VerifyNameMatch("www.foo.webkit.com"));
 }
 
 TEST(X509CertificateTest, WithoutRevocationChecking) {
   // Check that verification without revocation checking works.
@@ skipping 52 matching lines @@
   EXPECT_EQ(1263772799, valid_expiry.ToDoubleT());  // Jan 17 23:59:59 2010 GMT
 
   const SHA1Fingerprint& fingerprint = thawte_cert->fingerprint();
   for (size_t i = 0; i < 20; ++i)
     EXPECT_EQ(thawte_fingerprint[i], fingerprint.data[i]);
 
   std::vector<std::string> dns_names;
   thawte_cert->GetDNSNames(&dns_names);
   ASSERT_EQ(1U, dns_names.size());
   EXPECT_EQ("www.thawte.com", dns_names[0]);
+}
 
-#if TEST_EV
-  int flags = X509Certificate::VERIFY_REV_CHECKING_ENABLED |
-                X509Certificate::VERIFY_EV_CERT;
+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);
+
   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
 // a single RelativeDistinguishedName is present. "Normally" there is only
 // one AVA per RDN, but some CAs place all AVAs within a single RDN.
 // This is a regression test for http://crbug.com/101009
 TEST(X509CertificateTest, MultivalueRDN) {
   FilePath certs_dir = GetTestCertsDirectory();
 
   scoped_refptr<X509Certificate> multivalue_rdn_cert =
@@ skipping 1628 matching lines @@
 #define MAYBE_VerifyMixed DISABLED_VerifyMixed
 #else
 #define MAYBE_VerifyMixed VerifyMixed
 #endif
 WRAPPED_INSTANTIATE_TEST_CASE_P(
     MAYBE_VerifyMixed,
     X509CertificateWeakDigestTest,
     testing::ValuesIn(kVerifyMixedTestData));
 
 }  // namespace net