545725 - Changing passphrase should prevent other clients from syncing

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Ragavan S [:rags]]

If you change your passphrase on one device, then you should not be able to continue syncing from other devices. 

So, basically this means Change Passphrase goes away and you can only Reset Passphrase.

The reason we are doing this is because your passphrase is essentially your key and only you have access to it.

Comment 1

[Ed Lee :Mardak]

We discussed that perhaps we should just replace change with reset, which currently actively tells other clients to log out. But that remote command isn't necessary because resetPassphrase will also wipe the server and should eventually result in syncID changing, and remote clients seeing a syncID change will drop their cached keys and fail to sync.

Comment 2

[Mike Connor [:mconnor]]

Plan of record:

Combine Reset Passphrase and Change Passphrase to use the reset passphrase codepath. (wipe server, generate new keys, re-upload from scratch, force other clients to log out)

Existing, known codepaths, just need to design and implement some UI.

Comment 3

[chris hofmann]

we should also check the code paths of where passwords and pass phrases might get cached.  places like the password manager(s), and any internal caches.  I was seeing many inconsistent results with testing the steps in the blocking bug 553756 and that suggests internal caches of both passwords and passphrases were not being cleared consistently and reliably.

Comment 5

[Mike Connor [:mconnor]]

In general, the passphrase caching isn't the issue, it's the caching of the encryption key, which this bug will address.  I don't see anything about the password being improperly cached after a reset, can you file a bug on any password issues you have?

Comment 6

[Mike Connor [:mconnor]]

Attachment: wip

Comment 7

[Mike Connor [:mconnor]]

Attachment: wip2

Comment 8

[Mike Connor [:mconnor]]

Attachment: v1

This works, not sure it's perfect, but wanted feedback ASAP.

Comment 9

[Ed Lee :Mardak]

Comment on attachment 436400 [details] [diff] [review]
v1

>+++ b/source/chrome/content/generic-change.js
>     switch (Weave.Utils._genericDialogType) {
>-      case "ResetPassphrase":
>+      case "ChangePassphrase":
The UI bits in fx-prefs need to be updated to only have the one ChangePassphrase now. Right now resetPassphrase is provided conditionally only if the passphrase fails and changePassphrase is available on login. The conditional reset also appears when setting up a second machine and you enter the wrong passphrase.

>+  validate: function () {
>+    else if (val1 && val2 && val1 == val2 &&
Shouldn't this be a separate check to inform the user that the passwords aren't the same?

>+             val1.length >= Weave.MIN_PASS_LENGTH)
Did we want the password to be this long? It used to be 8. The passphrase is 12 I think. But now there's only one value and both need to be 12?

>+++ b/source/chrome/content/generic-change.xul
>+    <description flex="1">
>+      <html:p style="margin-top: 2px" id="introText"/>
>+      <html:p id="introText2"/>
>+    </description>
Is the description not sufficient and html:p are needed? o.O?

>+++ b/source/chrome/content/sync.js
>-    // Don't notify on missing passphrase errors
>-    if (!Weave.Service.passphrase)
>-      return;
>+    // if login fails, any other notifications are essentially moot
>+    Weave.Notifications.removeAll();
This silent passphrase failure is to prevent notifications from showing up when creating a new account and signing in on a second machine. The secret phrase panel is separate, so it tries logging in first with just the password. So I guess extra logic will be needed to clear out the notification from those interfaces.

>+    buttons.push(new Weave.NotificationButton(
>+      this._stringBundle.getString("error.login.prefs.label"),
>+      this._stringBundle.getString("error.login.prefs.accesskey"),
>+      function() { gWeaveWin.openPrefs(); return true; }
>+    ));
Good. Curious, any idea how this will interact with zpao's "weave needs to be setup" status text where clicking it will bring up the pref window? Should we just be opening the pref window on failure?

>+++ b/source/modules/service.js
>       case "weave:service:sync:error":
>         this._handleSyncError();
>+        if (this._needToReLogin) {
>+          this.logout();
>+          Utils.delay(function() this.login(), 0, this);
>+          this._needToReLogin = false;
What does this accomplish? Are you just trying to trigger the login:error notification? The needToReLogin was set because login failed on syncid change and it's a bad password/passphrase. You could just send the login:error notification here instead.. but I suppose you lose out on the reason for login failure....

Does Status.login get reset by this point?

>+  changePassphrase: function WeaveSvc_changePassphrase(newphrase)
>+    this._catch(this._notify("changepph", "", function() {
>       /* Make remote commands ready so we have a list of clients beforehand */
>       this.prepCommand("logout", []);
>       let clientsBackup = Clients._store.clients;
> 
>       /* Wipe */
>       this.wipeServer();
>       PubKeys.clearCache();
>       PrivKeys.clearCache();
Do we still need the prepCommand logout stuff? The syncID change + failed verifyLogin should do the trick.

This code should just call resetService or actually _freshStart to do reset + wipe. Probably need to persistLogin() too..

>+      // bug 545725 - re-verify creds and fail sanely
>+      if (!this._verifyLogin()) {
>+        this._needToReLogin = true;
>+        return false;
Should be able to reuse Status.sync and set a new constant.. CREDENTIALS_CHANGED instead of adding a new _needToReLogin property.

Comment 10

[Mike Connor [:mconnor]]

(In reply to comment #9)
> The UI bits in fx-prefs need to be updated to only have the one
> ChangePassphrase now. 

Right, fixed.

> >+  validate: function () {
> >+    else if (val1 && val2 && val1 == val2 &&
> Shouldn't this be a separate check to inform the user that the passwords aren't
> the same?

I don't really want to do that, since that will basically always be true.

> >+             val1.length >= Weave.MIN_PASS_LENGTH)
> Did we want the password to be this long? It used to be 8. The passphrase is 12
> I think. But now there's only one value and both need to be 12?

Mmm, right, fixed.  We should probably look at hooking up a pw strength indicator.

> >+++ b/source/chrome/content/generic-change.xul
> >+    <description flex="1">
> >+      <html:p style="margin-top: 2px" id="introText"/>
> >+      <html:p id="introText2"/>
> >+    </description>
> Is the description not sufficient and html:p are needed? o.O?

Yeah, description can't be set dynamically _and_ wrap... <description> is such a dumb tag.

> >+++ b/source/chrome/content/sync.js
> >-    // Don't notify on missing passphrase errors
> >-    if (!Weave.Service.passphrase)
> >-      return;
> >+    // if login fails, any other notifications are essentially moot
> >+    Weave.Notifications.removeAll();
> This silent passphrase failure is to prevent notifications from showing up when
> creating a new account and signing in on a second machine. The secret phrase
> panel is separate, so it tries logging in first with just the password. So I
> guess extra logic will be needed to clear out the notification from those
> interfaces.

Yeah, but that means if your pp goes missing, there's no indication something is wrong.  Not exactly awesome.

> >+    buttons.push(new Weave.NotificationButton(
> >+      this._stringBundle.getString("error.login.prefs.label"),
> >+      this._stringBundle.getString("error.login.prefs.accesskey"),
> >+      function() { gWeaveWin.openPrefs(); return true; }
> >+    ));
> Good. Curious, any idea how this will interact with zpao's "weave needs to be
> setup" status text where clicking it will bring up the pref window?

That'll trigger in other cases, this would be "you've set up the client, but you have a problem"

> Should we just be opening the pref window on failure?

No, except possibly in response to a user action.

> >+++ b/source/modules/service.js
> >       case "weave:service:sync:error":
> >         this._handleSyncError();
> >+        if (this._needToReLogin) {
> >+          this.logout();
> >+          Utils.delay(function() this.login(), 0, this);
> >+          this._needToReLogin = false;
> What does this accomplish? Are you just trying to trigger the login:error
> notification? The needToReLogin was set because login failed on syncid change
> and it's a bad password/passphrase. You could just send the login:error
> notification here instead.. but I suppose you lose out on the reason for login
> failure....
> 
> Does Status.login get reset by this point?

I don't see why we'd fake login failure instead of just actually retrying login and not worrying about manually setting various other states... we know it's going to fail, but logging out/logging in means we do not have to duplicate functionality.

> >+  changePassphrase: function WeaveSvc_changePassphrase(newphrase)
> >+    this._catch(this._notify("changepph", "", function() {
> >       /* Make remote commands ready so we have a list of clients beforehand */
> >       this.prepCommand("logout", []);
> >       let clientsBackup = Clients._store.clients;
> > 
> >       /* Wipe */
> >       this.wipeServer();
> >       PubKeys.clearCache();
> >       PrivKeys.clearCache();
> Do we still need the prepCommand logout stuff? The syncID change + failed
> verifyLogin should do the trick.

the failed verifyLogin is because the logout stuff doesn't work (encrypted commands with a different key == useless!)

> This code should just call resetService or actually _freshStart to do reset +
> wipe. Probably need to persistLogin() too..

Let's fix that separately, trying to not conflate too much here...

> >+      // bug 545725 - re-verify creds and fail sanely
> >+      if (!this._verifyLogin()) {
> >+        this._needToReLogin = true;
> >+        return false;
> Should be able to reuse Status.sync and set a new constant..
> CREDENTIALS_CHANGED instead of adding a new _needToReLogin property.

Yeah, good idea. Fixed.

Comment 11

[Mike Connor [:mconnor]]

Attachment: v1.1

Comment 12

[Ed Lee :Mardak]

Attachment: diff v1 - v1.1

Comment 13

[Ed Lee :Mardak]

Attachment: diff v1 - v1.1

Comment 14

[Ed Lee :Mardak]

Comment on attachment 436761 [details] [diff] [review]
v1.1

(In reply to comment #10)
> the failed verifyLogin is because the logout stuff doesn't work (encrypted
> commands with a different key == useless!)
We should at least not call it and do the clients store saving which shouldn't work right now anyway.

Comment 15

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/1792785b9852

Comment 16

[Ed Lee :Mardak]

Comment on attachment 436761 [details] [diff] [review]
v1.1

>+++ b/source/chrome/content/preferences/fx-setup.js
>-    if (state == 0 && (val.length < 12 || valConfirm.length < val.length))
>+    if (state == 0 && (val.length < Weave.MIN_PASS_LENGTH || valConfirm.length < val.length))
This should be MIN_PP_LENGTH

Comment 17

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/rev/eb8ef75539b9
Followup to bug 545725 to have creating new account passphrase check for 12 instead of 8.

Comment 18

[Tracy Walker [:tracy]]

verified with 1.2b4  changing passphrase on client A prevents client B from sync until the new passphrase is entered on client B (which weave on client B prompts for)