Bug 1453690 - Fail nicely when RemoteSetting collection is not signed r?mgoodwin draft
authorMathieu Leplatre <mathieu@mozilla.com>
Tue, 08 May 2018 18:41:50 +0200
changeset 792575 21655129ccc9bfff4ad88bb9dd0ad9469990291d
parent 792086 2d95d70298e248161b629ef1d0d57049c0b62d71
push id109145
push usermleplatre@mozilla.com
push dateTue, 08 May 2018 16:42:22 +0000
reviewersmgoodwin
bugs1453690
milestone62.0a1
Bug 1453690 - Fail nicely when RemoteSetting collection is not signed r?mgoodwin MozReview-Commit-ID: ESP7GXfmYzU
services/common/remote-settings.js
services/common/tests/unit/test_blocklist_signatures.js
--- a/services/common/remote-settings.js
+++ b/services/common/remote-settings.js
@@ -30,16 +30,17 @@ const PREF_SETTINGS_LAST_UPDATE        =
 const PREF_SETTINGS_LAST_ETAG          = "services.settings.last_etag";
 const PREF_SETTINGS_CLOCK_SKEW_SECONDS = "services.settings.clock_skew_seconds";
 const PREF_SETTINGS_LOAD_DUMP          = "services.settings.load_dump";
 
 // Telemetry update source identifier.
 const TELEMETRY_HISTOGRAM_KEY = "settings-changes-monitoring";
 
 const INVALID_SIGNATURE = "Invalid content/signature";
+const MISSING_SIGNATURE = "Missing signature";
 
 
 function mergeChanges(collection, localRecords, changes) {
   const records = {};
   // Local records by id.
   localRecords.forEach((record) => records[record.id] = collection.cleanLocalFields(record));
   // All existing records are replaced by the version from the server.
   changes.forEach((record) => records[record.id] = record);
@@ -52,25 +53,25 @@ function mergeChanges(collection, localR
       if (a.id < b.id) {
         return -1;
       }
       return a.id > b.id ? 1 : 0;
     });
 }
 
 
-function fetchCollectionMetadata(remote, collection) {
+async function fetchCollectionMetadata(remote, collection) {
   const client = new KintoHttpClient(remote);
-  return client.bucket(collection.bucket).collection(collection.name).getData()
-    .then(result => {
-      return result.signature;
-    });
+  const { signature } = await client.bucket(collection.bucket)
+                                    .collection(collection.name)
+                                    .getData();
+  return signature;
 }
 
-function fetchRemoteCollection(remote, collection) {
+async function fetchRemoteCollection(remote, collection) {
   const client = new KintoHttpClient(remote);
   return client.bucket(collection.bucket)
            .collection(collection.name)
            .listRecords({sort: "id"});
 }
 
 async function fetchLatestChanges(url, lastEtag) {
   //
@@ -297,18 +298,21 @@ class RemoteSettingsClient {
                 syncResult.created.push(r);
               }
             }
             // Records that remain in our map now are those missing from remote
             syncResult.deleted = Array.from(oldById.values());
           }
 
         } else {
-          // The sync has thrown, it can be a network or a general error.
-          if (/NetworkError/.test(e.message)) {
+          // The sync has thrown, it can be related to metadata, network or a general error.
+          if (e.message == MISSING_SIGNATURE) {
+            // Collection metadata has no signature info, no need to retry.
+            reportStatus = UptakeTelemetry.STATUS.SIGNATURE_ERROR;
+          } else if (/NetworkError/.test(e.message)) {
             reportStatus = UptakeTelemetry.STATUS.NETWORK_ERROR;
           } else if (/Backoff/.test(e.message)) {
             reportStatus = UptakeTelemetry.STATUS.BACKOFF;
           } else {
             reportStatus = UptakeTelemetry.STATUS.SYNC_ERROR;
           }
           throw e;
         }
@@ -362,19 +366,22 @@ class RemoteSettingsClient {
       throw new Error(`Could not read from '${fileURI}'`);
     }
     // Will be rejected if JSON is invalid.
     return response.json();
   }
 
   async _validateCollectionSignature(remote, payload, collection, options = {}) {
     const {ignoreLocal} = options;
-
     // this is a content-signature field from an autograph response.
-    const {x5u, signature} = await fetchCollectionMetadata(remote, collection);
+    const signaturePayload = await fetchCollectionMetadata(remote, collection);
+    if (!signaturePayload) {
+      throw new Error(MISSING_SIGNATURE);
+    }
+    const {x5u, signature} = signaturePayload;
     const certChainResponse = await fetch(x5u);
     const certChain = await certChainResponse.text();
 
     const verifier = Cc["@mozilla.org/security/contentsignatureverifier;1"]
                        .createInstance(Ci.nsIContentSignatureVerifier);
 
     let toSerialize;
     if (ignoreLocal) {
--- a/services/common/tests/unit/test_blocklist_signatures.js
+++ b/services/common/tests/unit/test_blocklist_signatures.js
@@ -257,16 +257,29 @@ add_task(async function test_check_signa
     ],
     status: {status: 200, statusText: "OK"},
     responseBody: JSON.stringify({"data": []})
   };
 
   const RESPONSE_BODY_META_EMPTY_SIG = makeMetaResponseBody(1000,
     "vxuAg5rDCB-1pul4a91vqSBQRXJG_j7WOYUTswxRSMltdYmbhLRH8R8brQ9YKuNDF56F-w6pn4HWxb076qgKPwgcEBtUeZAO_RtaHXRkRUUgVzAr86yQL4-aJTbv3D6u");
 
+  const RESPONSE_META_NO_SIG = {
+    sampleHeaders: [
+      "Content-Type: application/json; charset=UTF-8",
+      `ETag: \"123456\"`
+    ],
+    status: {status: 200, statusText: "OK"},
+    responseBody: JSON.stringify({
+      data: {
+        last_modified: 123456
+      }
+    })
+  };
+
   // The collection metadata containing the signature for the empty
   // collection.
   const RESPONSE_META_EMPTY_SIG =
     makeMetaResponse(1000, RESPONSE_BODY_META_EMPTY_SIG,
                      "RESPONSE_META_EMPTY_SIG");
 
   // Here, we map request method and path to the available responses
   const emptyCollectionResponses = {
@@ -558,16 +571,41 @@ add_task(async function test_check_signa
   } catch (e) {
     await checkRecordCount(OneCRLBlocklistClient, 2);
   }
 
   // Ensure that the failure is reflected in the accumulated telemetry:
   endHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
   expectedIncrements = {[UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR]: 1};
   checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
+
+
+  const missingSigResponses = {
+    // In this test, we deliberately serve metadata without the signature attribute.
+    // As if the collection was not signed.
+    "GET:/v1/buckets/blocklists/collections/certificates?":
+      [RESPONSE_META_NO_SIG],
+  };
+
+  startHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
+  registerHandlers(missingSigResponses);
+  try {
+    await OneCRLBlocklistClient.maybeSync(6000, startTime);
+    do_throw("Sync should fail (the signature is missing)");
+  } catch (e) {
+    await checkRecordCount(OneCRLBlocklistClient, 2);
+  }
+
+  // Ensure that the failure is reflected in the accumulated telemetry:
+  endHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
+  expectedIncrements = {
+    [UptakeTelemetry.STATUS.SIGNATURE_ERROR]: 1,
+    [UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR]: 0  // Not retried since missing.
+  };
+  checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
 });
 
 function run_test() {
   BlocklistClients.initialize();
 
   // ensure signatures are enforced
   Services.prefs.setBoolPref(PREF_SETTINGS_VERIFY_SIGNATURE, true);