Remove GetPkcs11Id from x509_certificate_model

We have the same function in chromeos::CertLoader.
CertLibrary, which is the only user of x509_certificate_model::GetPkcs11Id,
already depends on the CertLoader, so there is no real reson not to use
CertLoader::GetPkcs11IdForCert.

Also, update CertLibrary::GetCertIndexByPEM and
CertLibrary::GetCertIndexByPkcs11 to work only for a single certificate
type (server ca and client certificates respectively), instead of taking
the certificate type as param.

BUG=236978

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@262257 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 40 insertions, 72 deletions

diff --git a/chrome/browser/chromeos/options/cert_library.cc b/chrome/browser/chromeos/options/cert_library.cc
index d905186..7eeea75 100644
--- a/chrome/browser/chromeos/options/cert_library.cc
+++ b/chrome/browser/chromeos/options/cert_library.cc
@@ -161,13 +161,13 @@ base::string16 CertLibrary::GetCertDisplayStringAt(CertType type,
   return GetDisplayString(cert, hardware_backed);
 }
 
-std::string CertLibrary::GetCertPEMAt(CertType type, int index) const {
-  return CertToPEM(*GetCertificateAt(type, index));
+std::string CertLibrary::GetServerCACertPEMAt(int index) const {
+  return CertToPEM(*GetCertificateAt(CERT_TYPE_SERVER_CA, index));
 }
 
-std::string CertLibrary::GetCertPkcs11IdAt(CertType type, int index) const {
-  net::X509Certificate* cert = GetCertificateAt(type, index);
-  return x509_certificate_model::GetPkcs11Id(cert->os_cert_handle());
+std::string CertLibrary::GetUserCertPkcs11IdAt(int index) const {
+  net::X509Certificate* cert = GetCertificateAt(CERT_TYPE_USER, index);
+  return CertLoader::GetPkcs11IdForCert(*cert);
 }
 
 bool CertLibrary::IsCertHardwareBackedAt(CertType type, int index) const {
@@ -175,11 +175,11 @@ bool CertLibrary::IsCertHardwareBackedAt(CertType type, int index) const {
   return CertLoader::Get()->IsCertificateHardwareBacked(cert);
 }
 
-int CertLibrary::GetCertIndexByPEM(CertType type,
-                                   const std::string& pem_encoded) const {
-  int num_certs = NumCertificates(type);
+int CertLibrary::GetServerCACertIndexByPEM(
+    const std::string& pem_encoded) const {
+  int num_certs = NumCertificates(CERT_TYPE_SERVER_CA);
   for (int index = 0; index < num_certs; ++index) {
-    net::X509Certificate* cert = GetCertificateAt(type, index);
+    net::X509Certificate* cert = GetCertificateAt(CERT_TYPE_SERVER_CA, index);
     if (CertToPEM(*cert) != pem_encoded)
       continue;
     return index;
@@ -187,13 +187,12 @@ int CertLibrary::GetCertIndexByPEM(CertType type,
   return -1;
 }
 
-int CertLibrary::GetCertIndexByPkcs11Id(CertType type,
-                                        const std::string& pkcs11_id) const {
-  int num_certs = NumCertificates(type);
+int CertLibrary::GetUserCertIndexByPkcs11Id(
+    const std::string& pkcs11_id) const {
+  int num_certs = NumCertificates(CERT_TYPE_USER);
   for (int index = 0; index < num_certs; ++index) {
-    net::X509Certificate* cert = GetCertificateAt(type, index);
-    net::X509Certificate::OSCertHandle cert_handle = cert->os_cert_handle();
-    std::string id = x509_certificate_model::GetPkcs11Id(cert_handle);
+    net::X509Certificate* cert = GetCertificateAt(CERT_TYPE_USER, index);
+    std::string id = CertLoader::GetPkcs11IdForCert(*cert);
     if (id == pkcs11_id)
       return index;
   }
diff --git a/chrome/browser/chromeos/options/cert_library.h b/chrome/browser/chromeos/options/cert_library.h
index 9bbfb77..eb9b7e5 100644
--- a/chrome/browser/chromeos/options/cert_library.h
+++ b/chrome/browser/chromeos/options/cert_library.h
@@ -68,8 +68,8 @@ class CertLibrary : public CertLoader::Observer {
 
   // Retreives the certificate property for |type| at |index|.
   base::string16 GetCertDisplayStringAt(CertType type, int index) const;
-  std::string GetCertPEMAt(CertType type, int index) const;
-  std::string GetCertPkcs11IdAt(CertType type, int index) const;
+  std::string GetServerCACertPEMAt(int index) const;
+  std::string GetUserCertPkcs11IdAt(int index) const;
   bool IsCertHardwareBackedAt(CertType type, int index) const;
 
   // Returns the index of a Certificate matching |pem_encoded| or -1 if none
@@ -77,10 +77,9 @@ class CertLibrary : public CertLoader::Observer {
   // certificates.
   // TOOD(pneubeck): Either make this more efficient, asynchronous or get rid of
   // it.
-  int GetCertIndexByPEM(CertType type, const std::string& pem_encoded) const;
-  // Same as above but for a PKCS#11 id. TODO(stevenjb): Replace this with a
-  // better mechanism for uniquely idientifying certificates, crbug.com/236978.
-  int GetCertIndexByPkcs11Id(CertType type, const std::string& pkcs11_id) const;
+  int GetServerCACertIndexByPEM(const std::string& pem_encoded) const;
+  // Same as above but for a PKCS#11 id.
+  int GetUserCertIndexByPkcs11Id(const std::string& pkcs11_id) const;
 
   // CertLoader::Observer
   virtual void OnCertificatesLoaded(const net::CertificateList&,
diff --git a/chrome/browser/chromeos/options/vpn_config_view.cc b/chrome/browser/chromeos/options/vpn_config_view.cc
index 1daa1411..5dbb5e4 100644
--- a/chrome/browser/chromeos/options/vpn_config_view.cc
+++ b/chrome/browser/chromeos/options/vpn_config_view.cc
@@ -447,8 +447,7 @@ const std::string VPNConfigView::GetServerCACertPEM() const {
     return std::string();
   } else {
     int cert_index = index - 1;
-    return CertLibrary::Get()->GetCertPEMAt(
-        CertLibrary::CERT_TYPE_SERVER_CA, cert_index);
+    return CertLibrary::Get()->GetServerCACertPEMAt(cert_index);
   }
 }
 
@@ -458,8 +457,7 @@ const std::string VPNConfigView::GetUserCertID() const {
   } else {
     // Certificates are listed in the order they appear in the model.
     int index = user_cert_combobox_ ? user_cert_combobox_->selected_index() : 0;
-    return CertLibrary::Get()->GetCertPkcs11IdAt(
-        CertLibrary::CERT_TYPE_USER, index);
+    return CertLibrary::Get()->GetUserCertPkcs11IdAt(index);
   }
 }
 
@@ -886,8 +884,8 @@ void VPNConfigView::Refresh() {
     server_ca_cert_combobox_->ModelChanged();
     if (enable_server_ca_cert_ && !ca_cert_pem_.empty()) {
       // Select the current server CA certificate in the combobox.
-      int cert_index = CertLibrary::Get()->GetCertIndexByPEM(
-          CertLibrary::CERT_TYPE_SERVER_CA, ca_cert_pem_);
+      int cert_index =
+          CertLibrary::Get()->GetServerCACertIndexByPEM(ca_cert_pem_);
       if (cert_index >= 0) {
         // Skip item for "Default"
         server_ca_cert_combobox_->SetSelectedIndex(1 + cert_index);
@@ -902,8 +900,8 @@ void VPNConfigView::Refresh() {
   if (user_cert_combobox_) {
     user_cert_combobox_->ModelChanged();
     if (enable_user_cert_ && !client_cert_id_.empty()) {
-      int cert_index = CertLibrary::Get()->GetCertIndexByPkcs11Id(
-          CertLibrary::CERT_TYPE_USER, client_cert_id_);
+      int cert_index =
+          CertLibrary::Get()->GetUserCertIndexByPkcs11Id(client_cert_id_);
       if (cert_index >= 0)
         user_cert_combobox_->SetSelectedIndex(cert_index);
       else
diff --git a/chrome/browser/chromeos/options/wifi_config_view.cc b/chrome/browser/chromeos/options/wifi_config_view.cc
index 675caa5..f88b502 100644
--- a/chrome/browser/chromeos/options/wifi_config_view.cc
+++ b/chrome/browser/chromeos/options/wifi_config_view.cc
@@ -824,8 +824,7 @@ std::string WifiConfigView::GetEapServerCaCertPEM() const {
     return std::string();
   } else {
     int cert_index = index - 1;
-    return CertLibrary::Get()->GetCertPEMAt(
-        CertLibrary::CERT_TYPE_SERVER_CA, cert_index);
+    return CertLibrary::Get()->GetServerCACertPEMAt(cert_index);
   }
 }
 
@@ -847,8 +846,7 @@ std::string WifiConfigView::GetEapClientCertPkcs11Id() const {
   } else {
     // Certificates are listed in the order they appear in the model.
     int index = user_cert_combobox_->selected_index();
-    return CertLibrary::Get()->GetCertPkcs11IdAt(
-        CertLibrary::CERT_TYPE_USER, index);
+    return CertLibrary::Get()->GetUserCertPkcs11IdAt(index);
   }
 }
 
@@ -1279,8 +1277,8 @@ void WifiConfigView::InitFromProperties(
       }
     } else {
       // Select the certificate if available.
-      int cert_index = CertLibrary::Get()->GetCertIndexByPEM(
-          CertLibrary::CERT_TYPE_SERVER_CA, eap_ca_cert_pem);
+      int cert_index =
+          CertLibrary::Get()->GetServerCACertIndexByPEM(eap_ca_cert_pem);
       if (cert_index >= 0) {
         // Skip item for "Default".
         server_ca_cert_combobox_->SetSelectedIndex(1 + cert_index);
@@ -1297,8 +1295,8 @@ void WifiConfigView::InitFromProperties(
     properties.GetStringWithoutPathExpansion(
         shill::kEapCertIdProperty, &eap_cert_id);
     if (!eap_cert_id.empty()) {
-      int cert_index = CertLibrary::Get()->GetCertIndexByPkcs11Id(
-          CertLibrary::CERT_TYPE_USER, eap_cert_id);
+      int cert_index =
+          CertLibrary::Get()->GetUserCertIndexByPkcs11Id(eap_cert_id);
       if (cert_index >= 0)
         user_cert_combobox_->SetSelectedIndex(cert_index);
     }
diff --git a/chrome/common/net/x509_certificate_model.h b/chrome/common/net/x509_certificate_model.h
index 16cae48..fe7f06c 100644
--- a/chrome/common/net/x509_certificate_model.h
+++ b/chrome/common/net/x509_certificate_model.h
@@ -77,10 +77,6 @@ void GetNicknameStringsFromCertList(const net::CertificateList& certs,
                                     const std::string& cert_not_yet_valid,
                                     std::vector<std::string>* nick_names);
 
-// Returns the PKCS#11 attribute CKA_ID for a certificate as an upper-case
-// hex string, or the empty string if none is found.
-std::string GetPkcs11Id(net::X509Certificate::OSCertHandle cert_handle);
-
 struct Extension {
   std::string name;
   std::string value;
diff --git a/chrome/common/net/x509_certificate_model_nss.cc b/chrome/common/net/x509_certificate_model_nss.cc
index bc0785c..3fccdc0 100644
--- a/chrome/common/net/x509_certificate_model_nss.cc
+++ b/chrome/common/net/x509_certificate_model_nss.cc
@@ -258,29 +258,6 @@ void GetNicknameStringsFromCertList(
   CERT_DestroyCertList(cert_list);
 }
 
-// For background see this discussion on dev-tech-crypto.lists.mozilla.org:
-// http://web.archiveorange.com/archive/v/6JJW7E40sypfZGtbkzxX
-//
-// NOTE: This function relies on the convention that the same PKCS#11 ID
-// is shared between a certificate and its associated private and public
-// keys.  I tried to implement this with PK11_GetLowLevelKeyIDForCert(),
-// but that always returns NULL on Chrome OS for me.
-std::string GetPkcs11Id(net::X509Certificate::OSCertHandle cert_handle) {
-  std::string pkcs11_id;
-  SECKEYPrivateKey *priv_key = PK11_FindKeyByAnyCert(cert_handle,
-                                                     NULL /* wincx */);
-  if (priv_key) {
-    // Get the CKA_ID attribute for a key.
-    SECItem* sec_item = PK11_GetLowLevelKeyIDForPrivateKey(priv_key);
-    if (sec_item) {
-      pkcs11_id = base::HexEncode(sec_item->data, sec_item->len);
-      SECITEM_FreeItem(sec_item, PR_TRUE);
-    }
-    SECKEY_DestroyPrivateKey(priv_key);
-  }
-  return pkcs11_id;
-}
-
 void GetExtensions(
     const string& critical_label,
     const string& non_critical_label,
diff --git a/chrome/common/net/x509_certificate_model_openssl.cc b/chrome/common/net/x509_certificate_model_openssl.cc
index 72dd28d..ad5d67c 100644
--- a/chrome/common/net/x509_certificate_model_openssl.cc
+++ b/chrome/common/net/x509_certificate_model_openssl.cc
@@ -180,11 +180,6 @@ void GetNicknameStringsFromCertList(
   // TODO(bulach): implement me.
 }
 
-std::string GetPkcs11Id(net::X509Certificate::OSCertHandle cert_handle) {
-  // TODO(jamescook): implement me.
-  return "";
-}
-
 void GetExtensions(
     const std::string& critical_label,
     const std::string& non_critical_label,
diff --git a/chromeos/cert_loader.cc b/chromeos/cert_loader.cc
index d34b6d1..b72d43a 100644
--- a/chromeos/cert_loader.cc
+++ b/chromeos/cert_loader.cc
@@ -115,7 +115,8 @@ bool CertLoader::CertificatesLoading() const {
   return database_ && !certificates_loaded_;
 }
 
-// This is copied from chrome/common/net/x509_certificate_model_nss.cc.
+// static
+//
 // For background see this discussion on dev-tech-crypto.lists.mozilla.org:
 // http://web.archiveorange.com/archive/v/6JJW7E40sypfZGtbkzxX
 //
@@ -123,8 +124,6 @@ bool CertLoader::CertificatesLoading() const {
 // is shared between a certificate and its associated private and public
 // keys.  I tried to implement this with PK11_GetLowLevelKeyIDForCert(),
 // but that always returns NULL on Chrome OS for me.
-
-// static
 std::string CertLoader::GetPkcs11IdForCert(const net::X509Certificate& cert) {
   CERTCertificateStr* cert_handle = cert.os_cert_handle();
   SECKEYPrivateKey *priv_key =
diff --git a/chromeos/cert_loader.h b/chromeos/cert_loader.h
index 848a2c8..8968b3c 100644
--- a/chromeos/cert_loader.h
+++ b/chromeos/cert_loader.h
@@ -57,6 +57,13 @@ class CHROMEOS_EXPORT CertLoader : public net::CertDatabase::Observer {
   // Returns true if the global instance has been initialized.
   static bool IsInitialized();
 
+  // Returns the PKCS#11 attribute CKA_ID for a certificate as an upper-case
+  // hex string, or the empty string if none is found. Note that the returned ID
+  // should be used only to identify the cert in its slot.
+  // This should be used only for user certificates, assuming that only one
+  // private slot is loaded for a user.
+  // TODO(tbarzic): Make this check cert slot id if we start loading
+  // certificates for secondary users.
   static std::string GetPkcs11IdForCert(const net::X509Certificate& cert);
 
   // Starts the CertLoader with the NSS cert database.