Bug 1359879: add logging and robustness to key syncing, r=kmag

It seems like the server-side keyring is getting deleted somehow. This
causes syncing of the keyring to fail. We haven't completely
fixed the root problem yet, but this adds logging to try to flag
down the dangerous operation of deleting a bucket to make sure we
aren't doing that by accident. We also try to recover from the
server-keyring-was-deleted situation by wiping all storage.sync data,
the same as if we found a keyring that we couldn't decrypt.

MozReview-Commit-ID: JMB0IxApbGq

--HG--
extra : rebase_source : 54982a374aa5c7d7e60e8175b99734c672b91799

toolkit/components/extensions/ExtensionStorageSync.jsm

@@ -105,6 +105,12 @@ if (AppConstants.platform != "android") {
   _fxaService = fxAccounts;
 }
 
+class ServerKeyringDeleted extends Error {
+  constructor() {
+    super("server keyring appears to have disappeared; we were called to decrypt null");
+  }
+}
+
 /**
  * Check for FXA and throw an exception if we don't have access.
  *
@@ -315,6 +321,28 @@ class KeyRingEncryptionRemoteTransformer extends EncryptionRemoteTransformer {
   }
 
   async decode(record) {
+    if (record === null) {
+      // XXX: This is a hack that detects a situation that should
+      // never happen by using a technique that shouldn't actually
+      // work. See
+      // https://bugzilla.mozilla.org/show_bug.cgi?id=1359879 for
+      // the whole gory story.
+      //
+      // For reasons that aren't clear yet,
+      // sometimes the server-side keyring is deleted. When we try
+      // to sync our copy of the keyring, we get a conflict with the
+      // deleted version. Due to a bug in kinto.js, we are called to
+      // decode the deleted version, which is represented as
+      // null. For now, try to handle this by throwing a specific
+      // kind of exception which we can catch and recover from the
+      // same way we would do with any other kind of undecipherable
+      // keyring -- wiping the bucket and reuploading everything.
+      //
+      // Eventually we will probably fix the bug in kinto.js, and
+      // this will have to move somewhere else, probably in the code
+      // that detects a resolved conflict.
+      throw new ServerKeyringDeleted();
+    }
     try {
       return await super.decode(record);
     } catch (e) {
@@ -881,6 +909,7 @@ class ExtensionStorageSync {
    * @returns {Promise<void>}
    */
   async _deleteBucket() {
+    log.error("Deleting default bucket and everything in it");
     return await this._requestWithToken("Clearing server", async function(token) {
       const headers = {Authorization: "Bearer " + token};
       const kintoHttp = new KintoHttpClient(prefStorageSyncServerURL, {
@@ -943,6 +972,7 @@ class ExtensionStorageSync {
       return collectionKeys;
     }
 
+    log.info(`Need to create keys and/or salts for ${JSON.stringify(extIds)}`);
     const kbHash = await getKBHash(this._fxaService);
     const newKeys = await collectionKeys.ensureKeysFor(extIds);
     const newSalts = await this.ensureSaltsFor(keysRecord, extIds);
@@ -1051,11 +1081,13 @@ class ExtensionStorageSync {
       // No conflicts, or conflict was just someone else adding keys.
       return result;
     } catch (e) {
-      if (KeyRingEncryptionRemoteTransformer.isOutdatedKB(e)) {
+      if (KeyRingEncryptionRemoteTransformer.isOutdatedKB(e) ||
+          e instanceof ServerKeyringDeleted) {
         // Check if our token is still valid, or if we got locked out
         // between starting the sync and talking to Kinto.
         const isSessionValid = await this._fxaService.sessionStatus();
         if (isSessionValid) {
+          log.error("Couldn't decipher old keyring; deleting the default bucket and resetting sync status");
           await this._deleteBucket();
           await this.cryptoCollection.resetSyncStatus();
 

toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js

@@ -744,6 +744,58 @@ add_task(function* ensureCanSync_handles_conflicts() {
   });
 });
 
+add_task(function* ensureCanSync_handles_deleted_conflicts() {
+  // A keyring can be deleted, and this changes the format of the 412
+  // Conflict response from the Kinto server. Make sure we handle it correctly.
+  const extensionId = uuid();
+  const extensionId2 = uuid();
+  yield* withContextAndServer(function* (context, server) {
+    server.installCollection("storage-sync-crypto");
+    server.installDeleteBucket();
+    yield* withSignedInUser(loggedInUser, function* (extensionStorageSync, fxaService) {
+      server.etag = 700;
+      yield extensionStorageSync.cryptoCollection._clear();
+
+      // Generate keys that we can check for later.
+      let collectionKeys = yield extensionStorageSync.ensureCanSync([extensionId]);
+      const extensionKey = collectionKeys.keyForCollection(extensionId);
+      server.clearPosts();
+
+      // This is the response that the Kinto server return when the
+      // keyring has been deleted.
+      server.addRecord({collectionId: "storage-sync-crypto", conflict: true, data: null, etag: 765});
+
+      // Try to add a new extension to trigger a sync of the keyring.
+      let collectionKeys2 = yield extensionStorageSync.ensureCanSync([extensionId2]);
+
+      assertKeyRingKey(collectionKeys2, extensionId, extensionKey,
+                       `syncing keyring should keep our local key for ${extensionId}`);
+
+      deepEqual(server.getDeletedBuckets(), ["default"],
+                "Kinto server should have been wiped when keyring was thrown away");
+
+      let posts = server.getPosts();
+      equal(posts.length, 2,
+            "syncing keyring should have tried to post a keyring twice");
+      // The first post got a conflict.
+      const failedPost = posts[0];
+      assertPostedUpdatedRecord(failedPost, 700);
+      let body = yield assertPostedEncryptedKeys(fxaService, failedPost);
+
+      deepEqual(body.keys.collections[extensionId], extensionKey.keyPairB64,
+                `decrypted failed post should have the key for ${extensionId}`);
+
+      // The second post was after the wipe, and succeeded.
+      const afterWipePost = posts[1];
+      assertPostedNewRecord(afterWipePost);
+      let afterWipeBody = yield assertPostedEncryptedKeys(fxaService, afterWipePost);
+
+      deepEqual(afterWipeBody.keys.collections[extensionId], extensionKey.keyPairB64,
+                `decrypted new post should have preserved the key for ${extensionId}`);
+    });
+  });
+});
+
 add_task(function* checkSyncKeyRing_reuploads_keys() {
   // Verify that when keys are present, they are reuploaded with the
   // new kB when we call touchKeys().