Closed Bug 642727 Opened 9 years ago Closed 9 years ago

Don't trigger sync error when bad HMAC records are deleted

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: philikon, Assigned: rnewman)

Details

(Whiteboard: [verified in services][fixed in fx-sync])

Attachments

(1 file)

We allow engines to handle bad HMAC records. The clients engine simply deletes them. But it still triggers a Sync error. That should probably not happen.
Here's a stab at this.

* Introduce and use an enumeration for recovery strategies for bad items. The default is to error, as before.

* Reorganize the HMAC error handling code to use this, and to explicitly allow one retry.

* Don't pass through to the ordinary exception handler, which is what caused the failure reporting behavior.

* Add tests for the client engine in three scenarios:
  - HMAC error but redownload keys (client key is wrong)
  - HMAC error and delete record (record is wrong)
  - HMAC error and do both (both are wrong).

I reproduced the original error with some Web Console futzing (generate and upload some new keys, prompt a client download -- exactly like the tests), and verified that it does not propagate into the UI with this patch applied.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #520233 - Flags: review?(philipp)
Whiteboard: [has patch][needs review philikon]
Comment on attachment 520233 [details] [diff] [review]
Proposed patch. v1

>+// Enumeration to define approaches to handling bad records.
>+SyncEngine.RecoveryStrategy = {
>+  kIgnore: "ignore",
>+  kRetry:  "retry",
>+  kError:  "error"
>+};
>+

I wish we had ES Harmony records, they would make spelling these sort of constants less awkward.

Also, "interesting" choice of location. Not a module global, not part of the prototype, but on the constructor? Actually, I now see why a module global would be useless because implementations want to use the values, so maybe make them part of the prototype, e.g.:

  kRecoveryStrategy: {ignore: 1, retry: 2, error: 3},

seems descriptive enough to me. I wonder if we should rename 'retry' to 'refetchKeys' or something, though.

>+        } catch (ex if Utils.isHMACMismatch(ex)) {
>+          let strategy = self.handleHMACMismatch(item, true);
>+          if (strategy == SyncEngine.RecoveryStrategy.kRetry) {
>+            // You only get one retry.
>+            try {
>+              // Try decrypting again, typically because we've got new keys.
>+              self._log.info("Trying decrypt again...");
>+              item.decrypt();
>+            } catch (ex if Utils.isHMACMismatch(ex)) {
>+              strategy = self.handleHMACMismatch(item, false);
>+            }
>+          }
>+          switch (strategy) {
>+            case SyncEngine.RecoveryStrategy.kError:
>+              self._log.warn("Error decrypting record: " + Utils.exceptionStr(ex));
>+              failed.push(item.id);
>+              return;
>+            case SyncEngine.RecoveryStrategy.kIgnore:
>+              self._log.debug("Ignoring record " + item.id +
>+                              " with bad HMAC: already handled.");
>+              return;

I feel tempted to add

  case ...kRetry:
    this._log.debug("I said only *one* retry, you tw<censored>");
    return;

here, just to have a complete enumeration of all recovery strategy values.

>-  handleHMACMismatch: function handleHMACMismatch(item) {
>-    return Weave.Service.handleHMACEvent();
>+  handleHMACMismatch: function handleHMACMismatch(item, mayRetry) {

Please add a Javadoc style comment here explaining the possible return values and the parameters (in particular, mayRetry).

>+    _("Old record was not deleted, new one uploaded.");
>+    do_check_false(deleted);
>+    do_check_eq(2, clientsColl.count());
>+    _("Records now: " + clientsColl.get({}));
>+
>+    _("Old record was not deleted, new one uploaded.");
>+    do_check_false(deleted);
>+    do_check_eq(2, clientsColl.count());
>+    _("Records now: " + clientsColl.get({}));

Duped stanzas?

r=me with cosmetic changes discussed above.
Attachment #520233 - Flags: review?(philipp) → review+
(In reply to comment #2)

> I wish we had ES Harmony records, they would make spelling these sort of
> constants less awkward.

Aye!


> Also, "interesting" choice of location. Not a module global, not part of the
> prototype, but on the constructor? Actually, I now see why a module global
> would be useless because implementations want to use the values, so maybe make
> them part of the prototype, e.g.:
> 
>   kRecoveryStrategy: {ignore: 1, retry: 2, error: 3},

Having it on the constructor allows us to refer to it as SyncEngine.RecoveryStrategy, which seems neater to me. (About as close as we can get to a static in JS.)

Putting it on the prototype means we need to access it through an instance or the prototype. Easy enough to change, because all of the uses are in SyncEngine or its descendants, but slightly more messy (uses of 'self')...

Strings seem to be a more standard way of denoting constants, and they make more sense when printed... do you disagree?


> seems descriptive enough to me. I wonder if we should rename 'retry' to
> 'refetchKeys' or something, though.

Other way 'round -- this is what's returned when the keys _have been_ refetched, and thus the caller should try decrypting the item again.


> here, just to have a complete enumeration of all recovery strategy values.

Sounds good.


> Please add a Javadoc style comment here explaining the possible return values
> and the parameters (in particular, mayRetry).

Will do.


> Duped stanzas?

Hmm! Will delete that.
 

> r=me with cosmetic changes discussed above.

Thanks!
(In reply to comment #3)
> > Also, "interesting" choice of location. Not a module global, not part of the
> > prototype, but on the constructor? Actually, I now see why a module global
> > would be useless because implementations want to use the values, so maybe make
> > them part of the prototype, e.g.:
> > 
> >   kRecoveryStrategy: {ignore: 1, retry: 2, error: 3},
> 
> Having it on the constructor allows us to refer to it as
> SyncEngine.RecoveryStrategy, which seems neater to me. (About as close as we
> can get to a static in JS.)

Yeah, I get that. I just think it's confusing, but maybe that's just me. Don't feel strongly about this.

> Putting it on the prototype means we need to access it through an instance or
> the prototype. Easy enough to change, because all of the uses are in SyncEngine
> or its descendants, but slightly more messy (uses of 'self')...
> 
> Strings seem to be a more standard way of denoting constants, and they make
> more sense when printed... do you disagree?

I don't disagree. Keep the strings if you like.

> > seems descriptive enough to me. I wonder if we should rename 'retry' to
> > 'refetchKeys' or something, though.
> 
> Other way 'round -- this is what's returned when the keys _have been_
> refetched, and thus the caller should try decrypting the item again.

Ah right! Never mind then.
Whiteboard: [has patch][needs review philikon] → [has patch][has review]
Pushed:

https://hg.mozilla.org/services/services-central/rev/c7741414ca7a
Whiteboard: [has patch][has review] → [fixed in services]
Tested with CrossWeave successfully.

I'm inclined to push this to fx-sync to fix things for 1.7.1; any objection?
(In reply to comment #6)
> I'm inclined to push this to fx-sync to fix things for 1.7.1; any objection?

Not from me. +1
Pushed:

http://hg.mozilla.org/services/fx-sync/rev/ff8411b4499f
Whiteboard: [fixed in services] → [fixed in services][fixed in fx-sync]
STR for QA:

You'll need to manually cause keys to be regenerated. So...

* Create two profiles. Set them up to be syncing as usual. Verify all is well.
* Enable Sync logging and restart.
* In Profile A, open about:config then the Web Console. Enter:

  Components.utils.import("resource://services-sync/record.js").
CollectionKeys.generateNewKeys();

on one line. Hit Enter. You'll see "undefined" printed.

* In Profile B, add a bookmark and sync.
* In Profile A, sync.
* Profile A's log will include mention of an HMAC error and redownloading keys.
* There will be no user-visible error in Profile A.
Verified with Mac and XP using 4.2a1pre s-c builds fro last night.

1302283366534	Service.Main	INFO	Bad HMAC event detected. Attempting recovery or signaling to other clients.

no apparent error to user.
Whiteboard: [fixed in services][fixed in fx-sync] → [verified in services][fixed in fx-sync]
http://hg.mozilla.org/mozilla-central/rev/c7741414ca7a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla5
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.