If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

First-time login with encryption turned off fails

RESOLVED FIXED in 0.2

Status

Cloud Services
General
P2
normal
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: anant, Assigned: anant)

Tracking

unspecified
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.

Updated

10 years ago
Blocks: 433899
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Target Milestone: -- → 0.2
(Assignee)

Comment 1

10 years ago
Created attachment 321983 [details] [diff] [review]
Allow unencrypted login & sync

This patch allows login and sync when encryption is set to "none".
Assignee: nobody → anarayanan
Status: NEW → ASSIGNED
Attachment #321983 - Flags: review?(thunder)

Comment 2

10 years ago
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.
Attachment #321983 - Flags: review?(thunder) → review-
(Assignee)

Comment 3

10 years ago
Created attachment 322020 [details] [diff] [review]
Allow unencrypted login & sync

Ok, fixed. I assume you meant _keyCheck and not _versionCheck :)
Attachment #321983 - Attachment is obsolete: true
Attachment #322020 - Flags: review?(thunder)

Comment 4

10 years ago
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.
Attachment #322020 - Flags: review?(thunder) → review+
(Assignee)

Comment 5

10 years ago
Checked-in revision 347
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 6

10 years ago
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.
(Assignee)

Comment 7

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

Comment 8

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

Comment 9

10 years ago
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.

Updated

8 years ago
Component: Weave → General
Product: Mozilla Labs → Weave
QA Contact: weave → general
You need to log in before you can comment on or make changes to this bug.