Closed
Bug 614737
Opened 14 years ago
Closed 14 years ago
Simplified crypto: ensure older clients detect outdated version
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta8+ |
People
(Reporter: philikon, Assigned: rnewman)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
philikon
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•14 years ago
|
Assignee: nobody → rnewman
Updated•14 years ago
|
blocking2.0: --- → beta8+
Assignee | ||
Comment 1•14 years ago
|
||
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.)
Attachment #493772 -
Flags: review?(philipp)
Assignee | ||
Comment 2•14 years ago
|
||
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.
Attachment #493802 -
Flags: review?(philipp)
Assignee | ||
Comment 3•14 years ago
|
||
Add a line to verify that login succeeded.
Attachment #493802 -
Attachment is obsolete: true
Attachment #493805 -
Flags: review?(philipp)
Attachment #493802 -
Flags: review?(philipp)
Reporter | ||
Comment 4•14 years ago
|
||
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 :)
Attachment #493772 -
Flags: review?(philipp) → review+
Reporter | ||
Updated•14 years ago
|
Attachment #493805 -
Flags: review?(philipp) → review+
Assignee | ||
Comment 5•14 years ago
|
||
Pushed to fx-sync.
Reporter | ||
Comment 6•14 years ago
|
||
(In reply to comment #5)
> Pushed to fx-sync.
https://hg.mozilla.org/services/fx-sync/rev/66175eea6c82
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 7•14 years ago
|
||
verified testing recently nightly build to force update against a 1.5.1 ext on 3.6
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
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.
Description
•