Bug 1286600 - prevent errors in calls to the CertBlocklist from causing blocklist sync to fail. r=MattN draft
authorMark Goodwin <mgoodwin@mozilla.com>
Thu, 14 Jul 2016 21:31:54 +0100
changeset 387776 35597c7e60589a77182838aeba38ee5db53eabc4
parent 387623 08f8a5aacd8308a73f6040fe522be7ba38497561
child 525451 9d8b9bc746f1b455e49fa23aa919b8e682b388e1
push id23072
push usermgoodwin@mozilla.com
push dateThu, 14 Jul 2016 21:07:01 +0000
reviewersMattN
bugs1286600
milestone50.0a1
Bug 1286600 - prevent errors in calls to the CertBlocklist from causing blocklist sync to fail. r=MattN MozReview-Commit-ID: 42O2QnPQRuj
services/common/blocklist-clients.js
services/common/tests/unit/test_blocklist_certificates.js
--- a/services/common/blocklist-clients.js
+++ b/services/common/blocklist-clients.js
@@ -227,24 +227,29 @@ class BlocklistClient {
  * Revoke the appropriate certificates based on the records from the blocklist.
  *
  * @param {Object} records   current records in the local db.
  */
 function* updateCertBlocklist(records) {
   let certList = Cc["@mozilla.org/security/certblocklist;1"]
                    .getService(Ci.nsICertBlocklist);
   for (let item of records) {
-    if (item.issuerName && item.serialNumber) {
-      certList.revokeCertByIssuerAndSerial(item.issuerName,
-                                           item.serialNumber);
-    } else if (item.subject && item.pubKeyHash) {
-      certList.revokeCertBySubjectAndPubKey(item.subject,
-                                            item.pubKeyHash);
-    } else {
-      throw new Error("Cert blocklist record has incomplete data");
+    try {
+      if (item.issuerName && item.serialNumber) {
+        certList.revokeCertByIssuerAndSerial(item.issuerName,
+                                            item.serialNumber);
+      } else if (item.subject && item.pubKeyHash) {
+        certList.revokeCertBySubjectAndPubKey(item.subject,
+                                              item.pubKeyHash);
+      }
+    } catch (e) {
+      // prevent errors relating to individual blocklist entries from
+      // causing sync to fail. At some point in the future, we may want to
+      // accumulate telemetry on these failures.
+      Cu.reportError(e);
     }
   }
   certList.saveEntries();
 }
 
 /**
  * Write list of records into JSON file, and notify nsBlocklistService.
  *
--- a/services/common/tests/unit/test_blocklist_certificates.js
+++ b/services/common/tests/unit/test_blocklist_certificates.js
@@ -100,16 +100,23 @@ add_task(function* test_something(){
   yield OneCRLBlocklistClient.maybeSync(3000, Date.now());
 
   // Check the OneCRL check time pref is modified, even if the collection
   // hasn't changed
   Services.prefs.setIntPref("services.blocklist.onecrl.checked", 0);
   yield OneCRLBlocklistClient.maybeSync(3000, Date.now());
   let newValue = Services.prefs.getIntPref("services.blocklist.onecrl.checked");
   do_check_neq(newValue, 0);
+
+  // Check that a sync completes even when there's bad data in the
+  // collection. This will throw on fail, so just calling maybeSync is an
+  // acceptible test.
+  Services.prefs.setCharPref("services.settings.server",
+                             `http://localhost:${server.identity.primaryPort}/v1`);
+  yield OneCRLBlocklistClient.maybeSync(5000, Date.now());
 });
 
 function run_test() {
   // Ensure that signature verification is disabled to prevent interference
   // with basic certificate sync tests
   Services.prefs.setBoolPref("services.blocklist.signing.enforced", false);
 
   // Set up an HTTP Server
@@ -178,14 +185,40 @@ function getSampleResponse(req, port) {
         "id":"dabafde9-df4a-ddba-2548-748da04cc02c",
         "last_modified":4000
       },{
         "subject":"MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5",
         "pubKeyHash":"VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=",
         "id":"dabafde9-df4a-ddba-2548-748da04cc02d",
         "last_modified":4000
       }]})
+    },
+    "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=-last_modified&_since=4000": {
+      "sampleHeaders": [
+        "Access-Control-Allow-Origin: *",
+        "Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff",
+        "Content-Type: application/json; charset=UTF-8",
+        "Server: waitress",
+        "Etag: \"5000\""
+      ],
+      "status": {status: 200, statusText: "OK"},
+      "responseBody": JSON.stringify({"data":[{
+        "issuerName":"not a base64 encoded issuer",
+        "serialNumber":"not a base64 encoded serial",
+        "id":"dabafde9-df4a-ddba-2548-748da04cc02e",
+        "last_modified":5000
+      },{
+        "subject":"not a base64 encoded subject",
+        "pubKeyHash":"not a base64 encoded pubKeyHash",
+        "id":"dabafde9-df4a-ddba-2548-748da04cc02f",
+        "last_modified":5000
+      },{
+        "subject":"MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5",
+        "pubKeyHash":"VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=",
+        "id":"dabafde9-df4a-ddba-2548-748da04cc02g",
+        "last_modified":5000
+      }]})
     }
   };
   return responses[`${req.method}:${req.path}?${req.queryString}`] ||
          responses[req.method];
 
 }