bug 1453741 - (2/2) remove nsIX509CertDB.findCertByEmailAddress r=fkiefer,jcj
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 12 Apr 2018 12:46:25 -0700
changeset 413925 00a9881a5ceb8e1d345a39c4c425f5fd88b4aa76
parent 413924 c76fdea6986887d84c1d252634f2a68228e375f6
child 413926 4012129c0b4023a46e345b8f99e76b31dcd3514f
push id33853
push usercbrindusan@mozilla.com
push dateTue, 17 Apr 2018 09:51:13 +0000
treeherdermozilla-central@8b0ba3f7d099 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfkiefer, jcj
bugs1453741
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
bug 1453741 - (2/2) remove nsIX509CertDB.findCertByEmailAddress r=fkiefer,jcj nsIX509CertDB.findCertByEmailAddress performs multiple certificate verifications on the main thread, which is bad because it blocks the main thread and can cause nested event loop spinning. Firefox doesn't even use this function. Other products that use this function will either have to re-implement it locally or find some other workaround. MozReview-Commit-ID: HShl0H8cgxs
security/manager/ssl/nsIX509CertDB.idl
security/manager/ssl/nsNSSCertificateDB.cpp
security/manager/ssl/tests/unit/test_certDB_import.js
--- a/security/manager/ssl/nsIX509CertDB.idl
+++ b/security/manager/ssl/nsIX509CertDB.idl
@@ -73,27 +73,16 @@ interface nsIX509CertDB : nsISupports {
    *
    *  @param aDBkey Database internal key, as obtained using
    *                attribute dbkey in nsIX509Cert.
    */
   [must_use]
   nsIX509Cert findCertByDBKey(in ACString aDBkey);
 
   /**
-   *  Find a certificate by email address.
-   *
-   *  @param aEmailAddress The email address to be used as the key
-   *                       to find the certificate.
-   *
-   *  @return The matching certificate if found.
-   */
-  [must_use]
-  nsIX509Cert findCertByEmailAddress(in ACString aEmailAddress);
-
-  /**
    *  Use this to import a stream sent down as a mime type into
    *  the certificate database on the default token.
    *  The stream may consist of one or more certificates.
    *
    *  @param data The raw data to be imported
    *  @param length The length of the data to be imported
    *  @param type The type of the certificate, see constants in nsIX509Cert
    *  @param ctx A UI context.
--- a/security/manager/ssl/nsNSSCertificateDB.cpp
+++ b/security/manager/ssl/nsNSSCertificateDB.cpp
@@ -884,72 +884,16 @@ nsNSSCertificateDB::ExportPKCS12File(nsI
   if (count == 0) {
     return NS_OK;
   }
   nsPKCS12Blob blob;
   return blob.ExportToFile(aFile, certs, count);
 }
 
 NS_IMETHODIMP
-nsNSSCertificateDB::FindCertByEmailAddress(const nsACString& aEmailAddress,
-                                           nsIX509Cert** _retval)
-{
-  nsresult rv = BlockUntilLoadableRootsLoaded();
-  if (NS_FAILED(rv)) {
-    return rv;
-  }
-
-  RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier());
-  NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED);
-
-  const nsCString& flatEmailAddress = PromiseFlatCString(aEmailAddress);
-  UniqueCERTCertList certlist(
-    PK11_FindCertsFromEmailAddress(flatEmailAddress.get(), nullptr));
-  if (!certlist)
-    return NS_ERROR_FAILURE;
-
-  // certlist now contains certificates with the right email address,
-  // but they might not have the correct usage or might even be invalid
-
-  if (CERT_LIST_END(CERT_LIST_HEAD(certlist), certlist))
-    return NS_ERROR_FAILURE; // no certs found
-
-  CERTCertListNode *node;
-  // search for a valid certificate
-  for (node = CERT_LIST_HEAD(certlist);
-       !CERT_LIST_END(node, certlist);
-       node = CERT_LIST_NEXT(node)) {
-
-    UniqueCERTCertList unusedCertChain;
-    mozilla::pkix::Result result =
-      certVerifier->VerifyCert(node->cert, certificateUsageEmailRecipient,
-                               mozilla::pkix::Now(),
-                               nullptr /*XXX pinarg*/,
-                               nullptr /*hostname*/,
-                               unusedCertChain);
-    if (result == mozilla::pkix::Success) {
-      break;
-    }
-  }
-
-  if (CERT_LIST_END(node, certlist)) {
-    // no valid cert found
-    return NS_ERROR_FAILURE;
-  }
-
-  // node now contains the first valid certificate with correct usage
-  RefPtr<nsNSSCertificate> nssCert = nsNSSCertificate::Create(node->cert);
-  if (!nssCert)
-    return NS_ERROR_OUT_OF_MEMORY;
-
-  nssCert.forget(_retval);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
 nsNSSCertificateDB::ConstructX509FromBase64(const nsACString& base64,
                                     /*out*/ nsIX509Cert** _retval)
 {
   if (!_retval) {
     return NS_ERROR_INVALID_POINTER;
   }
 
   // Base64Decode() doesn't consider a zero length input as an error, and just
--- a/security/manager/ssl/tests/unit/test_certDB_import.js
+++ b/security/manager/ssl/tests/unit/test_certDB_import.js
@@ -63,27 +63,35 @@ function getCertAsByteArray(certPath) {
   let byteArray = [];
   for (let i = 0; i < certBytes.length; i++) {
     byteArray.push(certBytes.charCodeAt(i));
   }
 
   return byteArray;
 }
 
-function findCertByCommonName(commonName) {
+function commonFindCertBy(propertyName, value) {
   let certEnumerator = gCertDB.getCerts().getEnumerator();
   while (certEnumerator.hasMoreElements()) {
     let cert = certEnumerator.getNext().QueryInterface(Ci.nsIX509Cert);
-    if (cert.commonName == commonName) {
+    if (cert[propertyName] == value) {
       return cert;
     }
   }
   return null;
 }
 
+function findCertByCommonName(commonName) {
+  return commonFindCertBy("commonName", commonName);
+}
+
+function findCertByEmailAddress(emailAddress) {
+  return commonFindCertBy("emailAddress", emailAddress);
+}
+
 function testImportCACert() {
   // Sanity check the CA cert is missing.
   equal(findCertByCommonName(CA_CERT_COMMON_NAME), null,
         "CA cert should not be in the database before import");
 
   // Import and check for success.
   let caArray = getCertAsByteArray("test_certDB_import/importedCA.pem");
   gCertDB.importCertificates(caArray, caArray.length, Ci.nsIX509Cert.CA_CERT,
@@ -102,22 +110,21 @@ function run_test() {
   let certificateDialogsCID =
     MockRegistrar.register("@mozilla.org/nsCertificateDialogs;1",
                            gCertificateDialogs);
   registerCleanupFunction(() => {
     MockRegistrar.unregister(certificateDialogsCID);
   });
 
   // Sanity check the e-mail cert is missing.
-  throws(() => gCertDB.findCertByEmailAddress(TEST_EMAIL_ADDRESS),
-         /NS_ERROR_FAILURE/,
+  equal(findCertByEmailAddress(TEST_EMAIL_ADDRESS), null,
          "E-mail cert should not be in the database before import");
 
   // Import the CA cert so that the e-mail import succeeds.
   testImportCACert();
 
   // Import the e-mail cert and check for success.
   let emailArray = getCertAsByteArray("test_certDB_import/emailEE.pem");
   gCertDB.importEmailCertificate(emailArray, emailArray.length,
                                  gInterfaceRequestor);
-  notEqual(gCertDB.findCertByEmailAddress(TEST_EMAIL_ADDRESS), null,
+  notEqual(findCertByEmailAddress(TEST_EMAIL_ADDRESS), null,
            "E-mail cert should now be found in the database");
 }