Bug 642727 - Don't trigger sync error when bad HMAC records are deleted. r=philiKON
authorRichard Newman <rnewman@mozilla.com>
Sun, 20 Mar 2011 16:10:40 -0700
changeset 67759 c7741414ca7ab91e95a7e7f36f5c2484d1ae7c2e
parent 63446 8618a149ea2e673f6c0af23ee1f6183e39f0a6ef
child 67760 5fb27adf75a90394271d0b400e3d7778da687c4a
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersphiliKON
bugs642727
milestone2.0b13pre
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 642727 - Don't trigger sync error when bad HMAC records are deleted. r=philiKON
services/sync/modules/engines.js
services/sync/modules/engines/clients.js
services/sync/tests/unit/test_clients_engine.js
--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -411,16 +411,25 @@ Engine.prototype = {
     this._notify("wipe-client", this.name, this._wipeClient)();
   }
 };
 
 function SyncEngine(name) {
   Engine.call(this, name || "SyncEngine");
   this.loadToFetch();
 }
+
+// Enumeration to define approaches to handling bad records.
+// Attached to the constructor to allow use as a kind of static enumeration.
+SyncEngine.kRecoveryStrategy = {
+  ignore: "ignore",
+  retry:  "retry",
+  error:  "error"
+};
+
 SyncEngine.prototype = {
   __proto__: Engine.prototype,
   _recordObj: CryptoWrapper,
   version: 1,
   downloadLimit: null,
   applyIncomingBatchSize: DEFAULT_STORE_BATCH_SIZE,
 
   get storageURL() Svc.Prefs.get("clusterURL") + Svc.Prefs.get("storageAPI") +
@@ -635,24 +644,47 @@ SyncEngine.prototype = {
       item.collection = self.name;
       
       // Remember which records were processed
       handled.push(item.id);
 
       try {
         try {
           item.decrypt();
-        } catch (ex if (Utils.isHMACMismatch(ex) &&
-                        self.handleHMACMismatch(item))) {
-          // Let's try handling it.
-          // If the callback returns true, try decrypting again, because
-          // we've got new keys.
-          self._log.info("Trying decrypt again...");
-          item.decrypt();
-        }       
+        } catch (ex if Utils.isHMACMismatch(ex)) {
+          let strategy = self.handleHMACMismatch(item, true);
+          if (strategy == SyncEngine.kRecoveryStrategy.retry) {
+            // You only get one retry.
+            try {
+              // Try decrypting again, typically because we've got new keys.
+              self._log.info("Trying decrypt again...");
+              item.decrypt();
+              strategy = null;
+            } catch (ex if Utils.isHMACMismatch(ex)) {
+              strategy = self.handleHMACMismatch(item, false);
+            }
+          }
+          
+          switch (strategy) {
+            case null:
+              // Retry succeeded! No further handling.
+              break;
+            case SyncEngine.kRecoveryStrategy.retry:
+              self._log.debug("Ignoring second retry suggestion.");
+              // Fall through to error case.
+            case SyncEngine.kRecoveryStrategy.error:
+              self._log.warn("Error decrypting record: " + Utils.exceptionStr(ex));
+              failed.push(item.id);
+              return;
+            case SyncEngine.kRecoveryStrategy.ignore:
+              self._log.debug("Ignoring record " + item.id +
+                              " with bad HMAC: already handled.");
+              return;
+          }
+        }
       } catch (ex) {
         self._log.warn("Error decrypting record: " + Utils.exceptionStr(ex));
         failed.push(item.id);
         return;
       }
 
       let shouldApply;
       try {
@@ -1002,13 +1034,32 @@ SyncEngine.prototype = {
     this.resetLastSync();
     this.toFetch = [];
   },
 
   wipeServer: function wipeServer() {
     new Resource(this.engineURL).delete();
     this._resetClient();
   },
-  
-  handleHMACMismatch: function handleHMACMismatch(item) {
-    return Weave.Service.handleHMACEvent();
+
+  /*
+   * Decide on (and partially effect) an error-handling strategy.
+   *
+   * Asks the Service to respond to an HMAC error, which might result in keys
+   * being downloaded. That call returns true if an action which might allow a
+   * retry to occur.
+   *
+   * If `mayRetry` is truthy, and the Service suggests a retry,
+   * handleHMACMismatch returns kRecoveryStrategy.retry. Otherwise, it returns
+   * kRecoveryStrategy.error.
+   *
+   * Subclasses of SyncEngine can override this method to allow for different
+   * behavior -- e.g., to delete and ignore erroneous entries.
+   *
+   * All return values will be part of the kRecoveryStrategy enumeration.
+   */
+  handleHMACMismatch: function handleHMACMismatch(item, mayRetry) {
+    // By default we either try again, or bail out noisily.
+    return (Weave.Service.handleHMACEvent() && mayRetry) ?
+           SyncEngine.kRecoveryStrategy.retry :
+           SyncEngine.kRecoveryStrategy.error;
   }
 };
--- a/services/sync/modules/engines/clients.js
+++ b/services/sync/modules/engines/clients.js
@@ -190,27 +190,29 @@ ClientEngine.prototype = {
   _resetClient: function _resetClient() this._wipeClient(),
 
   _wipeClient: function _wipeClient() {
     SyncEngine.prototype._resetClient.call(this);
     this._store.wipe();
   },
   
   // Override the default behavior to delete bad records from the server.
-  handleHMACMismatch: function handleHMACMismatch(item) {
+  handleHMACMismatch: function handleHMACMismatch(item, mayRetry) {
     this._log.debug("Handling HMAC mismatch for " + item.id);
-    if (SyncEngine.prototype.handleHMACMismatch.call(this, item))
-      return true;
+    
+    let base = SyncEngine.prototype.handleHMACMismatch.call(this, item, mayRetry);
+    if (base != SyncEngine.kRecoveryStrategy.error)
+      return base;
 
     // It's a bad client record. Save it to be deleted at the end of the sync.
     this._log.debug("Bad client record detected. Scheduling for deletion.");
     this._deleteId(item.id);
 
-    // Don't try again.
-    return false;
+    // Neither try again nor error; we're going to delete it.
+    return SyncEngine.kRecoveryStrategy.ignore;
   }
 };
 
 function ClientStore(name) {
   Store.call(this, name);
 }
 ClientStore.prototype = {
   __proto__: Store.prototype,
--- a/services/sync/tests/unit/test_clients_engine.js
+++ b/services/sync/tests/unit/test_clients_engine.js
@@ -1,12 +1,13 @@
 Cu.import("resource://services-sync/constants.js");
 Cu.import("resource://services-sync/record.js");
 Cu.import("resource://services-sync/identity.js");
 Cu.import("resource://services-sync/util.js");
+Cu.import("resource://services-sync/engines.js");
 Cu.import("resource://services-sync/engines/clients.js");
 Cu.import("resource://services-sync/service.js");
 
 const MORE_THAN_CLIENTS_TTL_REFRESH = 691200; // 8 days
 const LESS_THAN_CLIENTS_TTL_REFRESH = 86400; // 1 day
 
 function test_bad_hmac() {
   _("Ensure that Clients engine deletes corrupt records.");
@@ -71,16 +72,71 @@ function test_bad_hmac() {
     do_check_true(!deleted);
     Clients.sync();
 
     _("Old record was deleted, new one uploaded.");
     do_check_true(deleted);
     do_check_eq(1, clientsColl.count());
     _("Records now: " + clientsColl.get({}));
 
+    _("Now change our keys but don't upload them. " +
+      "That means we get an HMAC error but redownload keys.");
+    Service.lastHMACEvent = 0;
+    Clients.localID = Utils.makeGUID();
+    Clients.resetClient();
+    CollectionKeys.generateNewKeys();
+    deleted = false;
+    do_check_eq(1, clientsColl.count());
+    Clients.sync();
+
+    _("Old record was not deleted, new one uploaded.");
+    do_check_false(deleted);
+    do_check_eq(2, clientsColl.count());
+    _("Records now: " + clientsColl.get({}));
+
+    _("Now try the scenario where our keys are wrong *and* there's a bad record.");
+    // Clean up and start fresh.
+    clientsColl.wbos = {};
+    Service.lastHMACEvent = 0;
+    Clients.localID = Utils.makeGUID();
+    Clients.resetClient();
+    deleted = false;
+    do_check_eq(0, clientsColl.count());
+
+    // Create and upload keys.
+    CollectionKeys.generateNewKeys();
+    serverKeys = CollectionKeys.asWBO("crypto", "keys");
+    serverKeys.encrypt(Weave.Service.syncKeyBundle);
+    do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
+
+    // Sync once to upload a record.
+    Clients.sync();
+    do_check_eq(1, clientsColl.count());
+
+    // Generate and upload new keys, so the old client record is wrong.
+    CollectionKeys.generateNewKeys();
+    serverKeys = CollectionKeys.asWBO("crypto", "keys");
+    serverKeys.encrypt(Weave.Service.syncKeyBundle);
+    do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
+
+    // Create a new client record and new keys. Now our keys are wrong, as well
+    // as the object on the server. We'll download the new keys and also delete
+    // the bad client record.
+    Clients.localID = Utils.makeGUID();
+    Clients.resetClient();
+    CollectionKeys.generateNewKeys();
+    let oldKey = CollectionKeys.keyForCollection();
+
+    do_check_false(deleted);
+    Clients.sync();
+    do_check_true(deleted);
+    do_check_eq(1, clientsColl.count());
+    let newKey = CollectionKeys.keyForCollection();
+    do_check_false(oldKey.equals(newKey));
+
   } finally {
     server.stop(do_test_finished);
     Svc.Prefs.resetBranch("");
     Records.clearCache();
   }
 }
 
 function test_properties() {