Bug 1620185 - Improve behaviour when signature is bad after retry r=glasserc

When the signature verification fails after retry we don't want to apply the changes, and if possible we want to leave the local data as it was before sync. Differential Revision: https://phabricator.services.mozilla.com/D67139 --HG-- extra : moz-landing-system : lando
2020-03-17 20:24:01 +00:00 · 2020-03-17 20:24:01 +00:00 · 24fcb6ab53
--- a/services/common/tests/unit/head_helpers.js
+++ b/services/common/tests/unit/head_helpers.js
@ -276,3 +276,7 @@ async function withFakeChannel(channel, f) {
    module.Policy = oldPolicy;
  }
 }
+
+function arrayEqual(a, b) {
+  return JSON.stringify(a) == JSON.stringify(b);
+}
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@ -427,6 +427,7 @@ class RemoteSettingsClient extends EventEmitter {
      const allData = await this.db.list();
      // Local data can contain local fields, strip them.
      let localRecords = allData.map(r => this._cleanLocalFields(r));
+      const localMetadata = await this.db.getMetadata();

      // If there is no data currently in the collection, attempt to import
      // initial data from the application defaults.
@ -461,10 +462,7 @@ class RemoteSettingsClient extends EventEmitter {

          // If the data is up-to-date but don't have metadata (records loaded from dump),
          // we fetch them and validate the signature immediately.
-          if (
-            this.verifySignature &&
-            ObjectUtils.isEmpty(await this.db.getMetadata())
-          ) {
+          if (this.verifySignature && ObjectUtils.isEmpty(localMetadata)) {
            console.debug(`${this.identifier} pull collection metadata`);
            const metadata = await this.httpClient().getData({
              query: { _expected: expectedTimestamp },
@ -502,6 +500,7 @@ class RemoteSettingsClient extends EventEmitter {
          syncResult = await this._importChanges(
            localRecords,
            collectionLastModified,
+            localMetadata,
            expectedTimestamp
          );
          if (gTimingEnabled) {
@ -546,8 +545,9 @@ class RemoteSettingsClient extends EventEmitter {
            syncResult = await this._importChanges(
              localRecords,
              collectionLastModified,
+              localMetadata,
              expectedTimestamp,
-              { clear: true }
+              { retry: true }
            );
          } catch (e) {
            // If the signature fails again, or if an error occured during wiping out the
@ -763,10 +763,11 @@ class RemoteSettingsClient extends EventEmitter {
  async _importChanges(
    localRecords,
    localTimestamp,
+    localMetadata,
    expectedTimestamp,
    options = {}
  ) {
-    const { clear = false } = options;
+    const { retry = false } = options;

    // Fetch collection metadata from server.
    const client = this.httpClient();
@ -774,7 +775,7 @@ class RemoteSettingsClient extends EventEmitter {
      query: { _expected: expectedTimestamp },
    });

-    // Fetch list of changes from server (or all records on clear)
+    // Fetch list of changes from server (or all records on retry)
    const {
      data: remoteRecords,
      last_modified: remoteTimestamp,
@ -782,7 +783,7 @@ class RemoteSettingsClient extends EventEmitter {
      filters: {
        _expected: expectedTimestamp,
      },
-      since: clear || !localTimestamp ? undefined : `${localTimestamp}`,
+      since: retry || !localTimestamp ? undefined : `${localTimestamp}`,
    });

    // We build a sync result, based on remote changes.
@ -809,15 +810,8 @@ class RemoteSettingsClient extends EventEmitter {
    );

    const start = Cu.now() * 1000;
-    if (clear) {
-      // In the retry situation, we fetched all server data,
-      // and we clear all local data before applying updates.
-      console.debug(`${this.identifier} clear local data`);
-      await this.db.clear();
-    } else {
-      // Otherwise delete local records for each tombstone.
-      await this.db.deleteBulk(toDelete);
-    }
+    // Delete local records for each tombstone.
+    await this.db.deleteBulk(toDelete);
    // Overwrite all other data.
    await this.db.importBulk(toInsert);
    await this.db.saveLastModified(remoteTimestamp);
@ -834,14 +828,45 @@ class RemoteSettingsClient extends EventEmitter {

    // Read the new local data, after updating.
    const newLocal = await this.db.list();
-    let newRecords = newLocal.map(r => this._cleanLocalFields(r));
+    const newRecords = newLocal.map(r => this._cleanLocalFields(r));
    // And verify the signature on what is now stored.
    if (this.verifySignature) {
-      await this._validateCollectionSignature(
-        newRecords,
-        remoteTimestamp,
-        metadata
-      );
+      try {
+        await this._validateCollectionSignature(
+          newRecords,
+          remoteTimestamp,
+          metadata
+        );
+      } catch (e) {
+        // Signature failed, clear local data.
+        console.debug(`${this.identifier} clear local data`);
+        await this.db.clear();
+
+        // If signature failed again after retry, then
+        // we restore the local data that we had before sync.
+        if (retry) {
+          try {
+            // Make sure the local data before sync was not tampered.
+            await this._validateCollectionSignature(
+              localRecords,
+              localTimestamp,
+              localMetadata
+            );
+            // Signature of data before sync is good. Restore it.
+            await this.db.importBulk(localRecords);
+            await this.db.saveLastModified(localTimestamp);
+            await this.db.saveMetadata(localMetadata);
+          } catch (_) {
+            // Local data before sync was tampered. Restore dump if available.
+            if (
+              await Utils.hasLocalDump(this.bucketName, this.collectionName)
+            ) {
+              await this._importJSONDump();
+            }
+          }
+        }
+        throw e;
+      }
    } else {
      console.warn(`${this.identifier} has signature disabled`);
    }
--- a/services/settings/test/unit/test_remote_settings_signatures.js
+++ b/services/settings/test/unit/test_remote_settings_signatures.js
@ -601,7 +601,7 @@ add_task(async function test_check_synchronization_with_signatures() {
  // - collection: [RECORD2, RECORD3] -> [RECORD2, RECORD3]
  // - timestamp: 4000 -> 5000
  //
-  // Check that a tempered local DB will be overwritten and
+  // Check that a tampered local DB will be overwritten and
  // sync event contain the appropriate data.

  const badLocalContentGoodSigResponses = {
@ -641,7 +641,7 @@ add_task(async function test_check_synchronization_with_signatures() {
  await client.maybeSync(5000);

  // Local data was replaced. But we use records IDs to determine
-  // what was created and deleted. So fake local data will appaer
+  // what was created and deleted. So fake local data will appear
  // in the sync event.
  equal(syncData.current.length, 2);
  equal(syncData.created.length, 1);
@ -654,11 +654,28 @@ add_task(async function test_check_synchronization_with_signatures() {

  //
  // 8.
-  // - collection: [RECORD2, RECORD3] -> [RECORD2, RECORD3]
-  // - timestamp: 4000 -> 5000
+  // - collection: [RECORD2, RECORD3] -> [RECORD2, RECORD3] (unchanged because of error)
+  // - timestamp: 4000 -> 6000
  //
-  // Check that a failing signature throws after retry.
+  // Check that a failing signature throws after retry, and that sync changes
+  // are not applied.

+  const RESPONSE_ONLY_RECORD4 = {
+    comment: "Delete RECORD3, create RECORD4",
+    sampleHeaders: [
+      "Content-Type: application/json; charset=UTF-8",
+      'ETag: "6000"',
+    ],
+    status: { status: 200, statusText: "OK" },
+    responseBody: JSON.stringify({
+      data: [
+        {
+          id: "f765df30-b2f1-42f6-9803-7bd5a07b5098",
+          last_modified: 6000,
+        },
+      ],
+    }),
+  };
  const allBadSigResponses = {
    // In this test, we deliberately serve only a bad signature.
    "GET:/v1/buckets/main/collections/signed?_expected=6000": [
@ -670,10 +687,9 @@ add_task(async function test_check_synchronization_with_signatures() {
    "GET:/v1/buckets/main/collections/signed/records?_expected=6000&_sort=-last_modified&_since=4000": [
      RESPONSE_EMPTY_NO_UPDATE,
    ],
-    // The next request is for the full collection sorted by id. This will be
-    // checked against the valid signature - so the sync should succeed.
+    // The next request is for the full collection.
    "GET:/v1/buckets/main/collections/signed/records?_expected=6000&_sort=-last_modified": [
-      RESPONSE_COMPLETE_INITIAL,
+      RESPONSE_ONLY_RECORD4,
    ],
  };

@ -683,7 +699,7 @@ add_task(async function test_check_synchronization_with_signatures() {
    await client.maybeSync(6000);
    do_throw("Sync should fail (the signature is intentionally bad)");
  } catch (e) {
-    equal((await client.get()).length, 2);
+    ok(true, "Sync failed as expected (bad signature after retry)");
  }

  // Ensure that the failure is reflected in the accumulated telemetry:
@ -691,12 +707,46 @@ add_task(async function test_check_synchronization_with_signatures() {
  expectedIncrements = { [UptakeTelemetry.STATUS.SIGNATURE_RETRY_ERROR]: 1 };
  checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);

+  // When signature fails after retry, the local data present before sync
+  // should be maintained.
+  ok(
+    arrayEqual(
+      (await client.get()).map(r => r.id),
+      [RECORD3.id, RECORD2.id]
+    ),
+    "Remote changes were not changed"
+  );
+  // And local data should still be valid.
+  await client.get({ verifySignature: true }); // Not raising.
+
  //
  // 9.
-  // - collection: [RECORD2, RECORD3] -> [RECORD2, RECORD3]
-  // - timestamp: 5000 -> 6000
+  // - collection: [RECORD2, RECORD3] -> [] (cleared)
+  // - timestamp: 4000 -> 6000
  //
-  // Check that sync throws if metadata has no signature.
+  // Check that local data is cleared during sync if signature is not valid.
+
+  await client.db.create({
+    id: "c6b19c67-2e0e-4a82-b7f7-1777b05f3e81",
+    last_modified: 42,
+    tampered: true,
+  });
+
+  try {
+    await client.maybeSync(6000);
+    do_throw("Sync should fail (the signature is intentionally bad)");
+  } catch (e) {
+    ok(true, "Sync failed as expected (bad signature after retry)");
+  }
+  // Since local data was tampered, it was cleared.
+  equal((await client.get()).length, 0, "Local database is now empty.");
+
+  //
+  // 11.
+  // - collection: [] -> []
+  // - timestamp: 4000 -> 6000
+  //
+  // Check that we don't apply changes when signature is missing in remote.

  const RESPONSE_META_NO_SIG = {
    sampleHeaders: [
@ -719,13 +769,16 @@ add_task(async function test_check_synchronization_with_signatures() {
    ],
  };

+  // Local data was empty after last test.
+  equal((await client.get()).length, 0);
+
  startHistogram = getUptakeTelemetrySnapshot(TELEMETRY_HISTOGRAM_KEY);
  registerHandlers(missingSigResponses);
  try {
    await client.maybeSync(6000);
    do_throw("Sync should fail (the signature is missing)");
  } catch (e) {
-    equal((await client.get()).length, 2);
+    equal((await client.get()).length, 0, "Local remains empty");
  }

  // Ensure that the failure is reflected in the accumulated telemetry: