bug 1529044 - intermediate certificate caching: import on a background thread to not block certificate verification r=mgoodwin
authorDana Keeler <dkeeler@mozilla.com>
Tue, 26 Mar 2019 15:56:32 +0000
changeset 466241 ae1ddf7ebb7b85071331283e67db6ff39ba9c1c0
parent 466240 a84b8318c39b3224e5a9066cd9a674c1d2ab3b5e
child 466242 e06a25b5771c1024c6539d2d94fbc011a8cc6884
push id35762
push usercsabou@mozilla.com
push dateWed, 27 Mar 2019 04:44:00 +0000
treeherdermozilla-central@bc572aee49b6 [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
security/manager/ssl/tests/unit/test_missing_intermediate/missing-intermediate.der
--- 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,89 @@
 // 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",
-                    ",,");
+    // NB: missing-intermediate.der won't be regenerated when
+    // missing-intermediate.pem is. Hopefully by that time we can just use
+    // missing-intermediate.pem directly.
+    let args = [ "-A", "-n", "manually-added-missing-intermediate", "-i",
+                 "test_missing_intermediate/missing-intermediate.der", "-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();
 }
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..aca4dc079661fbe9a4a2a1964767c40e69692fc1
GIT binary patch
literal 740
zc$_n6VtQcE#CT!>GZP~dlZf$sDZU-+?)qGvc;J;ugXRCv2}jBdc-c6$+C196^D;7W
zvoaV6844Kiu`!3TF!Qj7q!yPbI6E3BN$?vP7@8Xz8krk_K@>>9$N-2;p#lc-hO!3I
z5Pc%PnZ?DKdFcwCc_pbuxv43ci6yB(XDA`t#mLIQ+{DPw02Jq9YGPz$*wqoZ!lh5_
zn&aQRZ5?c80=727H(xz5_1g00YesmEO7+3xsw*Dckl(v;4%_TJskTqqkJd+(W@n`o
za#SsiTJGBZUFNFF9HzRn&VS@GFIXsOPF}5W*z?Kloj!-MbNJqhHt~O~Tbn7h@iLQD
zx0d6(<pmG;d3*n+acgf-nY5&KXGdb?gpJ`7Y%f_KeXzqreueqv4(IpkVmCs!r$w!*
zW166ERV~oKrgBCmXZqS{iP761O%pm$YPgtZ;@vH$zW-S(t9qI9{Z4Vd<rxn*SL-X~
zXFlH2H8oN8o!T|kxvM8~_?exSQDM>9FV?O9^ke@_al;*(|9L9^DhfPec$In0EeS5u
zx4lfvj0}v6fq`hi0}Mu4VHQ>cW=6*U2HYSXKMM;m3fa&iiy0Jdt>U)HNz+6%OLpcN
zwmTM@9l!nc2dmcC$1#Gb2WE!yT@JX^Q6a6M!{D9Bc)RL^N4~+K$jb+r4O#a(uL-{P
z)9+w<s(Z@JqO9gmRgd<(>zVr0cHgCxL>?}WU+HDvJ}+JSQtX4*gRRnGPX%HYe)h^O
zyZ5U&;Rt`vVyoDtk_q4E{Zf6nv!raJMP8utofT&uW_?_9ax2HIlXvW2?m7{pn<s0z
zKDRRV(b~367xG$KV&*y=JlOv6IakFs&wGZ49W^biUIjEQ&uzQ+UasZm#uCeKl9T6^
zJaftot987Y`Q33LfArgAx4SGgryX*pxnB9Q<ilaZL$=}mH6PPoFO94)xcbZH?<D|V
C8ZuG<