bug 1529044 - intermediate certificate caching: import on a background thread to not block certificate verification r=mgoodwin
☠☠ backed out by b0b02f3c5203 ☠ ☠
authorDana Keeler <dkeeler@mozilla.com>
Mon, 25 Mar 2019 17:09:37 +0000
changeset 465988 d641ac81d9f07ef75cb3e13f9c33e13564d47fb1
parent 465987 87be6cf9c45bbb60dbabb19caee172b5141ef5ab
child 465989 c6b4e466e15887eed24f7017f8cc25d1b887d5d0
push id112550
push userrgurzau@mozilla.com
push dateTue, 26 Mar 2019 09:57:15 +0000
treeherdermozilla-inbound@b8be99473610 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgoodwin
bugs1529044
milestone68.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 1529044 - intermediate certificate caching: import on a background thread to not block certificate verification r=mgoodwin Apparently importing a certificate into the NSS certificate DB is slow enough to materially impact the time it takes to connect to a site. This patch addresses this by importing any intermediate certificates we want to cache from verified connections on a background thread (so the certificate verification thread can return faster). Differential Revision: https://phabricator.services.mozilla.com/D24384
security/certverifier/NSSCertDBTrustDomain.cpp
security/manager/ssl/tests/unit/test_missing_intermediate.js
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -3,30 +3,33 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "NSSCertDBTrustDomain.h"
 
 #include <stdint.h>
 
+#include "CryptoTask.h"
 #include "ExtendedValidation.h"
 #include "NSSErrorsService.h"
 #include "OCSPVerificationTrustDomain.h"
 #include "PublicKeyPinningService.h"
 #include "cert.h"
 #include "certdb.h"
 #include "cert_storage/src/cert_storage.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Casting.h"
 #include "mozilla/Move.h"
 #include "mozilla/PodOperations.h"
+#include "mozilla/Services.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/Unused.h"
 #include "nsCRTGlue.h"
+#include "nsIObserverService.h"
 #include "nsNSSCertHelper.h"
 #include "nsNSSCertValidity.h"
 #include "nsNSSCertificate.h"
 #include "nsServiceManagerUtils.h"
 #include "nsThreadUtils.h"
 #include "nss.h"
 #include "pk11pub.h"
 #include "mozpkix/Result.h"
@@ -1279,36 +1282,77 @@ nsresult BuildRevocationCheckStrings(con
   rv = Base64Encode(pubKeyString, encPubKey);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   return NS_OK;
 }
 
+// Helper for SaveIntermediateCerts. Does the actual importing work on a
+// background thread so certificate verification can return its result.
+class BackgroundSaveIntermediateCertsTask final : public CryptoTask {
+ public:
+  explicit BackgroundSaveIntermediateCertsTask(
+      UniqueCERTCertList&& intermediates)
+      : mIntermediates(std::move(intermediates)) {}
+
+ private:
+  virtual nsresult CalculateResult() override {
+    UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
+    if (!slot) {
+      return NS_ERROR_FAILURE;
+    }
+    for (CERTCertListNode* node = CERT_LIST_HEAD(mIntermediates);
+         !CERT_LIST_END(node, mIntermediates); node = CERT_LIST_NEXT(node)) {
+      // This is a best-effort attempt at avoiding unknown issuer errors in the
+      // future, so ignore failures here.
+      nsAutoCString nickname;
+      if (NS_FAILED(DefaultServerNicknameForCert(node->cert, nickname))) {
+        continue;
+      }
+      Unused << PK11_ImportCert(slot.get(), node->cert, CK_INVALID_HANDLE,
+                                nickname.get(), false);
+    }
+    return NS_OK;
+  }
+
+  virtual void CallCallback(nsresult rv) override {
+    nsCOMPtr<nsIObserverService> observerService =
+        mozilla::services::GetObserverService();
+    if (observerService) {
+      observerService->NotifyObservers(nullptr, "psm:intermediate-certs-cached",
+                                       nullptr);
+    }
+  }
+
+  UniqueCERTCertList mIntermediates;
+};
+
 /**
  * Given a list of certificates representing a verified certificate path from an
  * end-entity certificate to a trust anchor, imports the intermediate
  * certificates into the permanent certificate database. This is an attempt to
  * cope with misconfigured servers that don't include the appropriate
  * intermediate certificates in the TLS handshake.
  *
  * @param certList the verified certificate list
  */
 void SaveIntermediateCerts(const UniqueCERTCertList& certList) {
   if (!certList) {
     return;
   }
 
-  UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
-  if (!slot) {
+  UniqueCERTCertList intermediates(CERT_NewCertList());
+  if (!intermediates) {
     return;
   }
 
   bool isEndEntity = true;
+  size_t numIntermediates = 0;
   for (CERTCertListNode* node = CERT_LIST_HEAD(certList);
        !CERT_LIST_END(node, certList); node = CERT_LIST_NEXT(node)) {
     if (isEndEntity) {
       // Skip the end-entity; we only want to store intermediates
       isEndEntity = false;
       continue;
     }
 
@@ -1328,24 +1372,27 @@ void SaveIntermediateCerts(const UniqueC
     // root temporarily imported via the child mode or enterprise root features.
     // We don't want to import these because they're intended to be temporary
     // (and because importing them happens to reset their trust settings, which
     // breaks these features).
     if (node == CERT_LIST_TAIL(certList)) {
       continue;
     }
 
-    nsAutoCString nickname;
-    nsresult rv = DefaultServerNicknameForCert(node->cert, nickname);
-    if (NS_FAILED(rv)) {
-      continue;
+    UniqueCERTCertificate certHandle(CERT_DupCertificate(node->cert));
+    if (CERT_AddCertToListTail(intermediates.get(), certHandle.get()) !=
+        SECSuccess) {
+      // If this fails, we're probably out of memory. Just return.
+      return;
     }
+    certHandle.release();  // intermediates now owns the reference
+    numIntermediates++;
+  }
 
-    // As mentioned in the documentation of this function, we're importing only
-    // to cope with misconfigured servers. As such, we ignore the return value
-    // below, since it doesn't really matter if the import fails.
-    Unused << PK11_ImportCert(slot.get(), node->cert, CK_INVALID_HANDLE,
-                              nickname.get(), false);
+  if (numIntermediates > 0) {
+    RefPtr<BackgroundSaveIntermediateCertsTask> task =
+        new BackgroundSaveIntermediateCertsTask(std::move(intermediates));
+    Unused << task->Dispatch("ImportInts");
   }
 }
 
 }  // namespace psm
 }  // namespace mozilla
--- a/security/manager/ssl/tests/unit/test_missing_intermediate.js
+++ b/security/manager/ssl/tests/unit/test_missing_intermediate.js
@@ -3,28 +3,86 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 "use strict";
 
 // Tests that if a server does not send a complete certificate chain, we can
 // make use of cached intermediates to build a trust path.
 
+const {TestUtils} = ChromeUtils.import("resource://testing-common/TestUtils.jsm");
+
 do_get_profile(); // must be called before getting nsIX509CertDB
 const certdb  = Cc["@mozilla.org/security/x509certdb;1"]
                   .getService(Ci.nsIX509CertDB);
 
+function run_certutil_on_directory(directory, args) {
+  let envSvc = Cc["@mozilla.org/process/environment;1"]
+                 .getService(Ci.nsIEnvironment);
+  let greBinDir = Services.dirsvc.get("GreBinD", Ci.nsIFile);
+  envSvc.set("DYLD_LIBRARY_PATH", greBinDir.path);
+  // TODO(bug 1107794): Android libraries are in /data/local/xpcb, but "GreBinD"
+  // does not return this path on Android, so hard code it here.
+  envSvc.set("LD_LIBRARY_PATH", greBinDir.path + ":/data/local/xpcb");
+  let certutilBin = _getBinaryUtil("certutil");
+  let process = Cc["@mozilla.org/process/util;1"].createInstance(Ci.nsIProcess);
+  process.init(certutilBin);
+  args.push("-d");
+  args.push(`sql:${directory}`);
+  process.run(true, args, args.length);
+  Assert.equal(process.exitValue, 0, "certutil should succeed");
+}
+
+registerCleanupFunction(() => {
+  let certDir = Services.dirsvc.get("CurWorkD", Ci.nsIFile);
+  certDir.append("bad_certs");
+  Assert.ok(certDir.exists(), "bad_certs should exist");
+  let args = [ "-D", "-n", "manually-added-missing-intermediate" ];
+  run_certutil_on_directory(certDir.path, args);
+});
+
 function run_test() {
-  addCertFromFile(certdb, "bad_certs/test-ca.pem", "CTu,,");
   add_tls_server_setup("BadCertServer", "bad_certs");
   // If we don't know about the intermediate, we'll get an unknown issuer error.
   add_connection_test("ee-from-missing-intermediate.example.com",
                       SEC_ERROR_UNKNOWN_ISSUER);
+
+  // Make BadCertServer aware of the intermediate.
   add_test(() => {
-    addCertFromFile(certdb, "test_missing_intermediate/missing-intermediate.pem",
-                    ",,");
+    let args = [ "-A", "-n", "manually-added-missing-intermediate", "-a", "-i",
+                 "test_missing_intermediate/missing-intermediate.pem", "-t",
+                 ",," ];
+    let certDir = Services.dirsvc.get("CurWorkD", Ci.nsIFile);
+    certDir.append("bad_certs");
+    Assert.ok(certDir.exists(), "bad_certs should exist");
+    run_certutil_on_directory(certDir.path, args);
+    run_next_test();
+  });
+
+  // We have to start observing the topic before there's a chance it gets
+  // emitted.
+  add_test(() => {
+    TestUtils.topicObserved("psm:intermediate-certs-cached").then(run_next_test);
     run_next_test();
   });
-  // Now that we've cached the intermediate, the connection should succeed.
+  // Connect and cache the intermediate.
   add_connection_test("ee-from-missing-intermediate.example.com",
                       PRErrorCodeSuccess);
+
+  // Add a dummy test so that the only way we advance from here is by observing
+  // "psm:intermediate-certs-cached".
+  add_test(() => {});
+
+  // Confirm that we cached the intermediate by running a certutil command that
+  // will fail if we didn't.
+  add_test(() => {
+    // Here we have to use the name that gecko chooses to give it.
+    let args = [ "-D", "-n", "Missing Intermediate" ];
+    run_certutil_on_directory(do_get_profile().path, args);
+    run_next_test();
+  });
+
+  // Since we cached the intermediate, this should succeed.
+  add_connection_test("ee-from-missing-intermediate.example.com",
+                      PRErrorCodeSuccess);
+
   run_next_test();
 }