Ignore a missing issuer certificate for certificate matching.

BUG=506185

Review URL: https://codereview.chromium.org/1212433006

Cr-Commit-Position: refs/heads/master@{#351579}

2 files changed, 110 insertions, 43 deletions

diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc
index e5e4dd9..a0545c9 100644
--- a/chromeos/network/client_cert_resolver.cc
+++ b/chromeos/network/client_cert_resolver.cc
@@ -71,6 +71,7 @@ bool HasPrivateKey(const net::X509Certificate& cert) {
 }
 
 // Describes a certificate which is issued by |issuer| (encoded as PEM).
+// |issuer| can be empty if no issuer certificate is found in the database.
 struct CertAndIssuer {
   CertAndIssuer(const scoped_refptr<net::X509Certificate>& certificate,
                 const std::string& issuer)
@@ -127,6 +128,33 @@ struct MatchCertWithPattern {
   const CertificatePattern pattern;
 };
 
+// Lookup the issuer certificate of |cert|. If it is available, return the PEM
+// encoding of that certificate. Otherwise return the empty string.
+std::string GetPEMEncodedIssuer(const net::X509Certificate& cert) {
+  net::ScopedCERTCertificate issuer_handle(
+      CERT_FindCertIssuer(cert.os_cert_handle(), PR_Now(), certUsageAnyCA));
+  if (!issuer_handle) {
+    VLOG(1) << "Couldn't find an issuer.";
+    return std::string();
+  }
+
+  scoped_refptr<net::X509Certificate> issuer =
+      net::X509Certificate::CreateFromHandle(
+          issuer_handle.get(),
+          net::X509Certificate::OSCertHandles() /* no intermediate certs */);
+  if (!issuer.get()) {
+    LOG(ERROR) << "Couldn't create issuer cert.";
+    return std::string();
+  }
+  std::string pem_encoded_issuer;
+  if (!net::X509Certificate::GetPEMEncoded(issuer->os_cert_handle(),
+                                           &pem_encoded_issuer)) {
+    LOG(ERROR) << "Couldn't PEM-encode certificate.";
+    return std::string();
+  }
+  return pem_encoded_issuer;
+}
+
 std::vector<CertAndIssuer> CreateSortedCertAndIssuerList(
     const net::CertificateList& certs) {
   // Filter all client certs and determines each certificate's issuer, which is
@@ -140,27 +168,7 @@ std::vector<CertAndIssuer> CreateSortedCertAndIssuerList(
         !CertLoader::IsCertificateHardwareBacked(&cert)) {
       continue;
     }
-    net::ScopedCERTCertificate issuer_handle(
-        CERT_FindCertIssuer(cert.os_cert_handle(), PR_Now(), certUsageAnyCA));
-    if (!issuer_handle) {
-      LOG(ERROR) << "Couldn't find an issuer.";
-      continue;
-    }
-    scoped_refptr<net::X509Certificate> issuer =
-        net::X509Certificate::CreateFromHandle(
-            issuer_handle.get(),
-            net::X509Certificate::OSCertHandles() /* no intermediate certs */);
-    if (!issuer.get()) {
-      LOG(ERROR) << "Couldn't create issuer cert.";
-      continue;
-    }
-    std::string pem_encoded_issuer;
-    if (!net::X509Certificate::GetPEMEncoded(issuer->os_cert_handle(),
-                                             &pem_encoded_issuer)) {
-      LOG(ERROR) << "Couldn't PEM-encode certificate.";
-      continue;
-    }
-    client_certs.push_back(CertAndIssuer(*it, pem_encoded_issuer));
+    client_certs.push_back(CertAndIssuer(*it, GetPEMEncodedIssuer(cert)));
   }
 
   std::sort(client_certs.begin(), client_certs.end(), &CompareCertExpiration);
diff --git a/chromeos/network/client_cert_resolver_unittest.cc b/chromeos/network/client_cert_resolver_unittest.cc
index 5032b63..0109b8c 100644
--- a/chromeos/network/client_cert_resolver_unittest.cc
+++ b/chromeos/network/client_cert_resolver_unittest.cc
@@ -101,25 +101,27 @@ class ClientCertResolverTest : public testing::Test,
     }
   }
 
-  // Imports a CA cert (stored as PEM in test_ca_cert_pem_) and a client
-  // certificate signed by that CA. Its PKCS#11 ID is stored in
-  // |test_cert_id_|.
-  void SetupTestCerts() {
-    // Import a CA cert.
-    net::CertificateList ca_cert_list =
-        net::CreateCertificateListFromFile(net::GetTestCertsDirectory(),
-                                           "client_1_ca.pem",
-                                           net::X509Certificate::FORMAT_AUTO);
+  // Imports a client certificate. Its PKCS#11 ID is stored in |test_cert_id_|.
+  // If |import_issuer| is true, also imports the CA cert (stored as PEM in
+  // test_ca_cert_pem_) that issued the client certificate.
+  void SetupTestCerts(bool import_issuer) {
+    // Load a CA cert.
+    net::CertificateList ca_cert_list = net::CreateCertificateListFromFile(
+        net::GetTestCertsDirectory(), "client_1_ca.pem",
+        net::X509Certificate::FORMAT_AUTO);
     ASSERT_TRUE(!ca_cert_list.empty());
-    net::NSSCertDatabase::ImportCertFailureList failures;
-    EXPECT_TRUE(test_nsscertdb_->ImportCACerts(
-        ca_cert_list, net::NSSCertDatabase::TRUST_DEFAULT, &failures));
-    ASSERT_TRUE(failures.empty()) << net::ErrorToString(failures[0].net_error);
-
     net::X509Certificate::GetPEMEncoded(ca_cert_list[0]->os_cert_handle(),
                                         &test_ca_cert_pem_);
     ASSERT_TRUE(!test_ca_cert_pem_.empty());
 
+    if (import_issuer) {
+      net::NSSCertDatabase::ImportCertFailureList failures;
+      EXPECT_TRUE(test_nsscertdb_->ImportCACerts(
+          ca_cert_list, net::NSSCertDatabase::TRUST_DEFAULT, &failures));
+      ASSERT_TRUE(failures.empty())
+          << net::ErrorToString(failures[0].net_error);
+    }
+
     // Import a client cert signed by that CA.
     test_client_cert_ =
         net::ImportClientCertAndKeyFromFile(net::GetTestCertsDirectory(),
@@ -172,10 +174,45 @@ class ClientCertResolverTest : public testing::Test,
         ->AddManagerService(kWifiStub, true);
   }
 
-  // Setup a policy with a certificate pattern that matches any client cert that
-  // is signed by the test CA cert (stored in |test_ca_cert_pem_|). In
+  // Sets up a policy with a certificate pattern that matches any client cert
+  // with a certain Issuer CN. It will match the test client cert.
+  void SetupPolicyMatchingIssuerCN() {
+    const char* kTestPolicy =
+        "[ { \"GUID\": \"wifi_stub\","
+        "    \"Name\": \"wifi_stub\","
+        "    \"Type\": \"WiFi\","
+        "    \"WiFi\": {"
+        "      \"Security\": \"WPA-EAP\","
+        "      \"SSID\": \"wifi_ssid\","
+        "      \"EAP\": {"
+        "        \"Outer\": \"EAP-TLS\","
+        "        \"ClientCertType\": \"Pattern\","
+        "        \"ClientCertPattern\": {"
+        "          \"Issuer\": {"
+        "            \"CommonName\": \"B CA\""
+        "          }"
+        "        }"
+        "      }"
+        "    }"
+        "} ]";
+
+    std::string error;
+    scoped_ptr<base::Value> policy_value = base::JSONReader::ReadAndReturnError(
+        kTestPolicy, base::JSON_ALLOW_TRAILING_COMMAS, NULL, &error);
+    ASSERT_TRUE(policy_value) << error;
+
+    base::ListValue* policy = NULL;
+    ASSERT_TRUE(policy_value->GetAsList(&policy));
+
+    managed_config_handler_->SetPolicy(
+        onc::ONC_SOURCE_USER_POLICY, kUserHash, *policy,
+        base::DictionaryValue() /* no global network config */);
+  }
+
+  // Sets up a policy with a certificate pattern that matches any client cert
+  // that is signed by the test CA cert (stored in |test_ca_cert_pem_|). In
   // particular it will match the test client cert.
-  void SetupPolicy() {
+  void SetupPolicyMatchingIssuerPEM() {
     const char* kTestPolicyTemplate =
         "[ { \"GUID\": \"wifi_stub\","
         "    \"Name\": \"wifi_stub\","
@@ -248,12 +285,13 @@ class ClientCertResolverTest : public testing::Test,
 };
 
 TEST_F(ClientCertResolverTest, NoMatchingCertificates) {
+  SetupTestCerts(false /* do not import the issuer */);
   StartCertLoader();
   SetupWifi();
   base::RunLoop().RunUntilIdle();
   network_properties_changed_count_ = 0;
   SetupNetworkHandlers();
-  SetupPolicy();
+  SetupPolicyMatchingIssuerPEM();
   base::RunLoop().RunUntilIdle();
 
   // Verify that no client certificate was configured.
@@ -264,13 +302,34 @@ TEST_F(ClientCertResolverTest, NoMatchingCertificates) {
   EXPECT_FALSE(client_cert_resolver_->IsAnyResolveTaskRunning());
 }
 
+TEST_F(ClientCertResolverTest, MatchIssuerCNWithoutIssuerInstalled) {
+  SetupTestCerts(false /* do not import the issuer */);
+  SetupWifi();
+  base::RunLoop().RunUntilIdle();
+
+  SetupNetworkHandlers();
+  SetupPolicyMatchingIssuerCN();
+  base::RunLoop().RunUntilIdle();
+
+  network_properties_changed_count_ = 0;
+  StartCertLoader();
+  base::RunLoop().RunUntilIdle();
+
+  // Verify that the resolver positively matched the pattern in the policy with
+  // the test client cert and configured the network.
+  std::string pkcs11_id;
+  GetClientCertProperties(&pkcs11_id);
+  EXPECT_EQ(test_cert_id_, pkcs11_id);
+  EXPECT_EQ(1, network_properties_changed_count_);
+}
+
 TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) {
-  SetupTestCerts();
+  SetupTestCerts(true /* import issuer */);
   SetupWifi();
   base::RunLoop().RunUntilIdle();
 
   SetupNetworkHandlers();
-  SetupPolicy();
+  SetupPolicyMatchingIssuerPEM();
   base::RunLoop().RunUntilIdle();
 
   network_properties_changed_count_ = 0;
@@ -286,7 +345,7 @@ TEST_F(ClientCertResolverTest, ResolveOnCertificatesLoaded) {
 }
 
 TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) {
-  SetupTestCerts();
+  SetupTestCerts(true /* import issuer */);
   SetupWifi();
   base::RunLoop().RunUntilIdle();
   StartCertLoader();
@@ -295,7 +354,7 @@ TEST_F(ClientCertResolverTest, ResolveAfterPolicyApplication) {
 
   // Policy application will trigger the ClientCertResolver.
   network_properties_changed_count_ = 0;
-  SetupPolicy();
+  SetupPolicyMatchingIssuerPEM();
   base::RunLoop().RunUntilIdle();
 
   // Verify that the resolver positively matched the pattern in the policy with