434625 - First-time login with encryption turned off fails

Status: RESOLVED FIXED

(Cloud Services :: General, defect, P2)

Description

[Anant Narayanan [:anant]]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_2; en-us) AppleWebKit/525.18 (KHTML, like Gecko) Version/3.1.1 Safari/525.18
Build Identifier: Mercurial tip

If the preference extensions.weave.encryption is set to "none" just before the first login, weave fails to login subsequently. This is because a key is not generated, but weave tries to run openssl anyway with an empty key on login.

Reproducible: Always

Steps to Reproduce:
1. Ensure a clean weave installation
2. Set extensions.weave.encryption preference to "none"
3. Try to login
Actual Results:  
Weave fails because OpenSSL returns -1 (the command executed points to an empty key file)

Expected Results:  
Login proceeds as normal

It isn't actually possible to try the steps to reproduce unless bug 434623 is fixed. They're actually related bugs, and may possibly have a single-patch fix.

Comment 1

[Anant Narayanan [:anant]]

Attachment: Allow unencrypted login & sync

This patch allows login and sync when encryption is set to "none".

Comment 2

[Dan Mills [:thunder]]

Comment on attachment 321983 [details] [diff] [review]
Allow unencrypted login & sync

>diff -r 1b33292a2ab4 modules/engines.js
>--- a/modules/engines.js	Wed May 21 11:16:39 2008 -0700
>+++ b/modules/engines.js	Wed May 21 11:44:05 2008 -0700
>@@ -681,10 +681,15 @@
>     this._engineId.setTempPassword(symkey);
>     if (!this._engineId.password)
>       throw "Could not generate a symmetric encryption key";
>-
>-    Crypto.RSAencrypt.async(Crypto, self.cb,
>-                            this._engineId.password, this._pbeId);
>-    let enckey = yield;
>+      
>+    if ("none" != Utils.prefs.getCharPref("encryption")) {
>+      Crypto.RSAencrypt.async(Crypto, self.cb,
>+                              this._engineId.password, this._pbeId);
>+      let enckey = yield;
>+    } else {
>+      enckey = this._engineId.password;
>+    }
>+    

I think I prefer (this is elsewhere in weave code):

let foo = "somevalue";
if (cond) {
  foo = "othervalue";
}

in cases where I need to use 'foo' later.  It makes it easier (for me, anyway) to see the variable definition if it's outside the conditional.

>diff -r 1b33292a2ab4 modules/service.js
>--- a/modules/service.js	Wed May 21 11:16:39 2008 -0700
>+++ b/modules/service.js	Wed May 21 11:44:05 2008 -0700
>@@ -414,8 +414,10 @@
> 
>     this._versionCheck.async(this, self.cb);
>     yield;
>-    this._keyCheck.async(this, self.cb);
>-    yield;
>+    if ("none" != Weave.Utils.prefs.getCharPref("encryption")) {
>+      this._keyCheck.async(this, self.cb);
>+      yield;
>+    }

Hmm why not change _versionCheck instead?  I prefer that to changing its callers.

Comment 3

[Anant Narayanan [:anant]]

Attachment: Allow unencrypted login & sync

Ok, fixed. I assume you meant _keyCheck and not _versionCheck :)

Comment 4

[Dan Mills [:thunder]]

Comment on attachment 322020 [details] [diff] [review]
Allow unencrypted login & sync

Yeah, I meant _keyCheck.

>   _keyCheck: function WeaveSvc__keyCheck() {
>-    let self = yield;
>+    if ("none" != Weave.Utils.prefs.getCharPref("encryption")) {
>+      let self = yield;

This is an asynchronous function, it must yield once and get self, no matter what.  What I'd do instead is something like:

----
_keyCheck: function WeaveSvc__keyCheck() {
  let self = yield;

  if ("none" == Utils.prefs.getCharPref("encryption"))
    return;
  ...
----

The Weave.* namespace is used outside the modules (in chrome/*), but not inside.  You can just return because the callers don't expect a return value (otherwise you'd have to call self.done() before returning.

Do that, and r+ from me.

Comment 5

[Anant Narayanan [:anant]]

Checked-in revision 347

Comment 6

[Chris Beard]

fyi, as a matter of policy, we want to ensure that all data uploaded to a mozilla controlled sever is encrypted. we do not want to have any access to end-user data.

enabling unencrypted data uploads for non-mozilla servers is acceptable.

not clear what should be changed here, if anything.

Comment 7

[Anant Narayanan [:anant]]

We could make a check for 'server = services.mozilla.com' and 'encryption = none' and raise an exception if both conditions are met.

Comment 8

[Chris Beard]

yeah, something like that and also have the preferences UI force the encryption scheme when you use the default server

Comment 9

[Dan Mills [:thunder]]

Yep, we should check for services.m.c and not allow unencrypted data.

But this but is about unencrypted data not working at all, with any server.  It's useful for debugging, so we should fix it.