614737 - Simplified crypto: ensure older clients detect outdated version

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

From bug 603489 comment 73:

The storageVersion detection code so far in Sync does not anticipate a change in the crypto structure. Upon upgrade we delete keys/pubkey and keys/privkey. This makes older Sync versions think that the passphrase is wrong instead of surfacing the "Your version is too old" error message on Sync. It doesn't even get that far because it fails on login.

This is very unfortunate. I believe we should do two things about this:

1. Do not delete keys/pubkey and keys/privkey upon migration. Just leave them around, they won't do harm and instead trick old clients into logging in properly first and then detecting their version is too old.

2. Make sure that we won't run into this problem in the future. This could be the case already since we now call remoteSetup() from verifyLogin() which definitely does this check somewhere, but it would good to have a test for this.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Patch and tests for changes.

This patch adjusts wipeServer to leave the "keys" collection by default.

Tests exercise this particular functionality.

The scenario in question has been manually tested:

* Create two new 3.6/1.5 profiles with the same Sync credentials
* Sync successfully
* Update one to the current build, watch it wipe server and upgrade
* Attempt to sync other client: observe "upgrade needed" notification, rather than "wrong sync key".

For good measure, I then verified that upgrading the other client will allow syncing to resume. IT LIVES!

A subsequent patch will address the storage version change detection logic in an automated test.

(Two separate patches so we can keep things moving.)

Comment 2

[Richard Newman [:rnewman]]

Attachment: Tests for detection of upgrade.

This test verifies login, then verifies that sync fails for the correct reason if the server version is bumped. It verifies again after wiping the server, as would be done during an upgrade.

Note that the old logic for signin/passphrase/key verification is no longer in the tree, so this test doesn't exercise it -- in other words, if you take out the fix, this test still passes. This test is still valuable to avoid a future regression.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Tests for detection of upgrade. v2

Add a line to verify that login succeeded.

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 493772 [details] [diff] [review]
Patch and tests for changes.

> // Some tests hang on OSX debug builds. See bug 604565.
> let DISABLE_TESTS_BUG_604565 = false;
> #ifdef XP_MACOSX
> #ifdef MOZ_DEBUG_SYMBOLS
>-DISABLE_TESTS_BUG_604565 = true;
>+DISABLE_TESTS_BUG_604565 = false;

Please remove this :)

Comment 5

[Richard Newman [:rnewman]]

Pushed to fx-sync.

Comment 6

[Philipp von Weitershausen [:philikon]]

(In reply to comment #5)
> Pushed to fx-sync.

https://hg.mozilla.org/services/fx-sync/rev/66175eea6c82

Comment 7

[Tracy Walker [:tracy]]

verified testing recently nightly build to force update against a 1.5.1 ext on 3.6