Make chromeos ONC certificate code use NSS types directly.

Bug: 671420
Change-Id: I7c21271715b467f352c059e6648841adf2713d21
Reviewed-on: https://chromium-review.googlesource.com/622078
Commit-Queue: Matt Mueller <mattm@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499321}

chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc

@@ -38,6 +38,7 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/test_data_directory.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -117,8 +118,8 @@ class FakeCertificateImporter : public chromeos::onc::CertificateImporter {
   ~FakeCertificateImporter() override {}
 
   void SetTrustedCertificatesResult(
-      net::CertificateList onc_trusted_certificates) {
-    onc_trusted_certificates_ = onc_trusted_certificates;
+      net::ScopedCERTCertificateList onc_trusted_certificates) {
+    onc_trusted_certificates_ = std::move(onc_trusted_certificates);
   }
 
   void SetExpectedONCCertificates(const base::ListValue& certificates) {
@@ -145,13 +146,14 @@ class FakeCertificateImporter : public chromeos::onc::CertificateImporter {
           expected_onc_certificates_.get(), &certificates));
     }
     ++call_count_;
-    done_callback.Run(true, onc_trusted_certificates_);
+    done_callback.Run(true, net::x509_util::DupCERTCertificateList(
+                                onc_trusted_certificates_));
   }
 
  private:
   ::onc::ONCSource expected_onc_source_;
   std::unique_ptr<base::ListValue> expected_onc_certificates_;
-  net::CertificateList onc_trusted_certificates_;
+  net::ScopedCERTCertificateList onc_trusted_certificates_;
   unsigned int call_count_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeCertificateImporter);
@@ -377,16 +379,15 @@ TEST_F(NetworkConfigurationUpdaterTest, PolicyIsValidatedAndRepaired) {
 
 TEST_F(NetworkConfigurationUpdaterTest,
        DoNotAllowTrustedCertificatesFromPolicy) {
-  net::CertificateList cert_list;
-  cert_list =
-      net::CreateCertificateListFromFile(net::GetTestCertsDirectory(),
-                                         "ok_cert.pem",
-                                         net::X509Certificate::FORMAT_AUTO);
+  net::ScopedCERTCertificateList cert_list =
+      net::CreateCERTCertificateListFromFile(net::GetTestCertsDirectory(),
+                                             "ok_cert.pem",
+                                             net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
 
   EXPECT_CALL(network_config_handler_,
               SetPolicy(onc::ONC_SOURCE_USER_POLICY, _, _, _));
-  certificate_importer_->SetTrustedCertificatesResult(cert_list);
+  certificate_importer_->SetTrustedCertificatesResult(std::move(cert_list));
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -415,15 +416,14 @@ TEST_F(NetworkConfigurationUpdaterTest,
   EXPECT_CALL(network_config_handler_, SetPolicy(_, _, _, _))
       .Times(AnyNumber());
 
-  net::CertificateList cert_list;
-  cert_list =
-      net::CreateCertificateListFromFile(net::GetTestCertsDirectory(),
-                                         "ok_cert.pem",
-                                         net::X509Certificate::FORMAT_AUTO);
+  net::ScopedCERTCertificateList cert_list =
+      net::CreateCERTCertificateListFromFile(net::GetTestCertsDirectory(),
+                                             "ok_cert.pem",
+                                             net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
 
   certificate_importer_->SetExpectedONCSource(onc::ONC_SOURCE_USER_POLICY);
-  certificate_importer_->SetTrustedCertificatesResult(cert_list);
+  certificate_importer_->SetTrustedCertificatesResult(std::move(cert_list));
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -466,13 +466,12 @@ TEST_F(NetworkConfigurationUpdaterTest,
   EXPECT_TRUE(observer.trust_anchors_.empty());
 
   // Now use a non-empty certificate list to test the observer notification.
-  net::CertificateList cert_list;
-  cert_list =
-      net::CreateCertificateListFromFile(net::GetTestCertsDirectory(),
-                                         "ok_cert.pem",
-                                         net::X509Certificate::FORMAT_AUTO);
+  net::ScopedCERTCertificateList cert_list =
+      net::CreateCERTCertificateListFromFile(net::GetTestCertsDirectory(),
+                                             "ok_cert.pem",
+                                             net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
-  certificate_importer_->SetTrustedCertificatesResult(cert_list);
+  certificate_importer_->SetTrustedCertificatesResult(std::move(cert_list));
 
   // Change to any non-empty policy, so that updates are triggered. The actual
   // content of the policy is irrelevant.

chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc

@@ -22,6 +22,7 @@
 #include "net/cert/cert_verify_result.h"
 #include "net/cert/nss_cert_database_chromeos.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
 #include "net/log/net_log_with_source.h"
 #include "net/test/cert_test_util.h"
 #include "net/test/test_data_directory.h"
@@ -54,10 +55,17 @@ class PolicyCertVerifierTest : public testing::Test {
         crypto::GetPublicSlotForChromeOSUser(test_nss_user_.username_hash())));
 
     test_ca_cert_ = LoadCertificate("root_ca_cert.pem", net::CA_CERT);
-    ASSERT_TRUE(test_ca_cert_.get());
-    test_server_cert_ = LoadCertificate("ok_cert.pem", net::SERVER_CERT);
-    ASSERT_TRUE(test_server_cert_.get());
-    test_ca_cert_list_.push_back(test_ca_cert_);
+    ASSERT_TRUE(test_ca_cert_);
+    test_ca_cert_list_.push_back(
+        net::x509_util::DupCERTCertificate(test_ca_cert_.get()));
+
+    net::ScopedCERTCertificate test_server_cert =
+        LoadCertificate("ok_cert.pem", net::SERVER_CERT);
+    ASSERT_TRUE(test_server_cert);
+    test_server_cert_ =
+        net::x509_util::CreateX509CertificateFromCERTCertificate(
+            test_server_cert.get());
+    ASSERT_TRUE(test_server_cert_);
   }
 
   void TearDown() override {
@@ -96,9 +104,9 @@ class PolicyCertVerifierTest : public testing::Test {
   }
 
   // |test_ca_cert_| is the issuer of |test_server_cert_|.
-  scoped_refptr<net::X509Certificate> test_ca_cert_;
+  net::ScopedCERTCertificate test_ca_cert_;
   scoped_refptr<net::X509Certificate> test_server_cert_;
-  net::CertificateList test_ca_cert_list_;
+  net::ScopedCERTCertificateList test_ca_cert_list_;
   std::unique_ptr<net::NSSCertDatabaseChromeOS> test_cert_db_;
   std::unique_ptr<PolicyCertVerifier> cert_verifier_;
 
@@ -107,10 +115,12 @@ class PolicyCertVerifierTest : public testing::Test {
     trust_anchor_used_ = true;
   }
 
-  scoped_refptr<net::X509Certificate> LoadCertificate(const std::string& name,
-                                                      net::CertType type) {
-    scoped_refptr<net::X509Certificate> cert =
-        net::ImportCertFromFile(net::GetTestCertsDirectory(), name);
+  net::ScopedCERTCertificate LoadCertificate(const std::string& name,
+                                             net::CertType type) {
+    net::ScopedCERTCertificate cert =
+        net::ImportCERTCertificateFromFile(net::GetTestCertsDirectory(), name);
+    if (!cert)
+      return cert;
 
     // No certificate is trusted right after it's loaded.
     net::NSSCertDatabase::TrustBits trust =
@@ -194,8 +204,13 @@ TEST_F(PolicyCertVerifierTest, VerifyUsingAdditionalTrustAnchor) {
   }
   EXPECT_FALSE(WasTrustAnchorUsedAndReset());
 
+  net::CertificateList test_ca_x509cert_list =
+      net::x509_util::CreateX509CertificateListFromCERTCertificates(
+          test_ca_cert_list_);
+  ASSERT_FALSE(test_ca_x509cert_list.empty());
+
   // Verify() again with the additional trust anchors.
-  cert_verifier_->SetTrustAnchors(test_ca_cert_list_);
+  cert_verifier_->SetTrustAnchors(test_ca_x509cert_list);
   {
     net::CertVerifyResult verify_result;
     net::TestCompletionCallback callback;
@@ -209,7 +224,7 @@ TEST_F(PolicyCertVerifierTest, VerifyUsingAdditionalTrustAnchor) {
   EXPECT_TRUE(WasTrustAnchorUsedAndReset());
 
   // Verify() again with the additional trust anchors will hit the cache.
-  cert_verifier_->SetTrustAnchors(test_ca_cert_list_);
+  cert_verifier_->SetTrustAnchors(test_ca_x509cert_list);
   {
     net::CertVerifyResult verify_result;
     net::TestCompletionCallback callback;

chrome/browser/chromeos/policy/user_network_configuration_updater.cc

@@ -21,6 +21,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
 
 namespace policy {
 
@@ -87,10 +88,13 @@ void UserNetworkConfigurationUpdater::GetWebTrustedCertificates(
 
 void UserNetworkConfigurationUpdater::OnCertificatesImported(
     bool /* unused success */,
-    const net::CertificateList& onc_trusted_certificates) {
+    net::ScopedCERTCertificateList onc_trusted_certificates) {
   web_trust_certs_.clear();
-  if (allow_trusted_certificates_from_policy_)
-    web_trust_certs_ = onc_trusted_certificates;
+  if (allow_trusted_certificates_from_policy_) {
+    web_trust_certs_ =
+        net::x509_util::CreateX509CertificateListFromCERTCertificates(
+            onc_trusted_certificates);
+  }
   NotifyTrustAnchorsChanged();
 }
 

chrome/browser/chromeos/policy/user_network_configuration_updater.h

@@ -17,6 +17,7 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "net/cert/scoped_nss_types.h"
 
 class Profile;
 
@@ -98,7 +99,7 @@ class UserNetworkConfigurationUpdater : public NetworkConfigurationUpdater,
   // Called by the CertificateImporter when an import finished.
   void OnCertificatesImported(
       bool success,
-      const net::CertificateList& onc_trusted_certificates);
+      net::ScopedCERTCertificateList onc_trusted_certificates);
 
   // NetworkConfigurationUpdater:
   void ImportCertificates(const base::ListValue& certificates_onc) override;

chrome/browser/ui/webui/net_internals/net_internals_ui.cc

@@ -213,7 +213,7 @@ class NetInternalsMessageHandler
   void OnCertificatesImported(
       const std::string& previous_error,
       bool success,
-      const net::CertificateList& onc_trusted_certificates);
+      net::ScopedCERTCertificateList onc_trusted_certificates);
 #endif
 
  private:
@@ -955,7 +955,7 @@ void NetInternalsMessageHandler::ImportONCFileToNSSDB(
 void NetInternalsMessageHandler::OnCertificatesImported(
     const std::string& previous_error,
     bool success,
-    const net::CertificateList& /* unused onc_trusted_certificates */) {
+    net::ScopedCERTCertificateList /* unused onc_trusted_certificates */) {
   std::string error = previous_error;
   if (!success)
     error += "Some certificates couldn't be imported. ";

chromeos/network/onc/onc_certificate_importer.h

@@ -9,7 +9,7 @@
 #include "base/macros.h"
 #include "chromeos/chromeos_export.h"
 #include "components/onc/onc_constants.h"
-#include "net/cert/x509_certificate.h"
+#include "net/cert/scoped_nss_types.h"
 
 namespace base {
 class ListValue;
@@ -20,8 +20,9 @@ namespace onc {
 
 class CHROMEOS_EXPORT CertificateImporter {
  public:
-  typedef base::Callback<
-      void(bool success, const net::CertificateList& onc_trusted_certificates)>
+  typedef base::Callback<void(
+      bool success,
+      net::ScopedCERTCertificateList onc_trusted_certificates)>
       DoneCallback;
 
   CertificateImporter() {}

chromeos/network/onc/onc_certificate_importer_impl.cc

@@ -24,7 +24,7 @@
 #include "crypto/scoped_nss_types.h"
 #include "net/base/net_errors.h"
 #include "net/cert/nss_cert_database.h"
-#include "net/cert/x509_certificate.h"
+#include "net/cert/x509_util_nss.h"
 
 namespace chromeos {
 namespace onc {
@@ -35,9 +35,10 @@ void CallBackOnOriginLoop(
     const scoped_refptr<base::SingleThreadTaskRunner>& origin_loop,
     const CertificateImporter::DoneCallback& callback,
     bool success,
-    const net::CertificateList& onc_trusted_certificates) {
+    net::ScopedCERTCertificateList onc_trusted_certificates) {
   origin_loop->PostTask(
-      FROM_HERE, base::Bind(callback, success, onc_trusted_certificates));
+      FROM_HERE,
+      base::BindOnce(callback, success, std::move(onc_trusted_certificates)));
 }
 
 }  // namespace
@@ -93,7 +94,7 @@ void CertificateImporterImpl::ParseAndStoreCertificates(
     net::NSSCertDatabase* nssdb) {
   // Web trust is only granted to certificates imported by the user.
   bool allow_trust_imports = source == ::onc::ONC_SOURCE_USER_IMPORT;
-  net::CertificateList onc_trusted_certificates;
+  net::ScopedCERTCertificateList onc_trusted_certificates;
   bool success = true;
   for (size_t i = 0; i < certificates->GetSize(); ++i) {
     const base::DictionaryValue* certificate = NULL;
@@ -111,24 +112,24 @@ void CertificateImporterImpl::ParseAndStoreCertificates(
     }
   }
 
-  done_callback.Run(success, onc_trusted_certificates);
+  done_callback.Run(success, std::move(onc_trusted_certificates));
 }
 
 void CertificateImporterImpl::RunDoneCallback(
     const CertificateImporter::DoneCallback& callback,
     bool success,
-    const net::CertificateList& onc_trusted_certificates) {
+    net::ScopedCERTCertificateList onc_trusted_certificates) {
   if (!success)
     NET_LOG_ERROR("ONC Certificate Import Error", "");
-  callback.Run(success, onc_trusted_certificates);
+  callback.Run(success, std::move(onc_trusted_certificates));
 }
 
 bool CertificateImporterImpl::ParseAndStoreCertificate(
     ::onc::ONCSource source,
     bool allow_trust_imports,
     const base::DictionaryValue& certificate,
     net::NSSCertDatabase* nssdb,
-    net::CertificateList* onc_trusted_certificates) {
+    net::ScopedCERTCertificateList* onc_trusted_certificates) {
   std::string guid;
   certificate.GetStringWithoutPathExpansion(::onc::certificate::kGUID, &guid);
   DCHECK(!guid.empty());
@@ -156,7 +157,7 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
     const std::string& guid,
     const base::DictionaryValue& certificate,
     net::NSSCertDatabase* nssdb,
-    net::CertificateList* onc_trusted_certificates) {
+    net::ScopedCERTCertificateList* onc_trusted_certificates) {
   // Device policy can't import certificates.
   if (source == ::onc::ONC_SOURCE_DEVICE_POLICY) {
     // This isn't a parsing error.
@@ -205,8 +206,7 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
     return false;
   }
 
-  scoped_refptr<net::X509Certificate> x509_cert =
-      DecodePEMCertificate(x509_data);
+  net::ScopedCERTCertificate x509_cert = DecodePEMCertificate(x509_data);
   if (!x509_cert.get()) {
     LOG(ERROR) << "Unable to create certificate from PEM encoding, type: "
                << cert_type;
@@ -217,7 +217,7 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
                                            net::NSSCertDatabase::TRUSTED_SSL :
                                            net::NSSCertDatabase::TRUST_DEFAULT);
 
-  if (x509_cert->os_cert_handle()->isperm) {
+  if (x509_cert.get()->isperm) {
     net::CertType net_cert_type =
         cert_type == ::onc::certificate::kServer ? net::SERVER_CERT
                                                  : net::CA_CERT;
@@ -239,8 +239,8 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
       }
     }
   } else {
-    net::CertificateList cert_list;
-    cert_list.push_back(x509_cert);
+    net::ScopedCERTCertificateList cert_list;
+    cert_list.push_back(net::x509_util::DupCERTCertificate(x509_cert.get()));
     net::NSSCertDatabase::ImportCertFailureList failures;
     bool success = false;
     if (cert_type == ::onc::certificate::kServer)
@@ -262,7 +262,7 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
   }
 
   if (web_trust_flag && onc_trusted_certificates)
-    onc_trusted_certificates->push_back(x509_cert);
+    onc_trusted_certificates->push_back(std::move(x509_cert));
 
   return true;
 }
@@ -291,7 +291,7 @@ bool CertificateImporterImpl::ParseClientCertificate(
   if (!private_slot)
     return false;
 
-  net::CertificateList imported_certs;
+  net::ScopedCERTCertificateList imported_certs;
 
   int import_result =
       nssdb->ImportFromPKCS12(private_slot.get(), decoded_pkcs12,
@@ -313,14 +313,12 @@ bool CertificateImporterImpl::ParseClientCertificate(
                     "Only the first one will be imported.";
   }
 
-  scoped_refptr<net::X509Certificate> cert_result = imported_certs[0];
+  CERTCertificate* cert_result = imported_certs[0].get();
 
   // Find the private key associated with this certificate, and set the
   // nickname on it.
   SECKEYPrivateKey* private_key = PK11_FindPrivateKeyFromCert(
-      cert_result->os_cert_handle()->slot,
-      cert_result->os_cert_handle(),
-      NULL);  // wincx
+      cert_result->slot, cert_result, nullptr /* wincx */);
   if (private_key) {
     PK11_SetPrivateKeyNickname(private_key, const_cast<char*>(guid.c_str()));
     SECKEY_DestroyPrivateKey(private_key);