Last Comment Bug 499417 - Refactor login manager's crypto code
: Refactor login manager's crypto code
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.3a1
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on:
Blocks: 534565 534589
  Show dependency treegraph
 
Reported: 2009-06-19 17:30 PDT by Justin Dolske [:Dolske]
Modified: 2009-12-15 00:26 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (WIP) (29.97 KB, patch)
2009-06-19 17:35 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.2 (33.47 KB, patch)
2009-09-04 17:29 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch v.3 (32.68 KB, patch)
2009-09-28 20:34 PDT, Justin Dolske [:Dolske]
vladimir: review+
paul: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2009-06-19 17:30:45 PDT
The code login manger uses for doing encryption/decryption should ideally live in a separate component. This will eventually allow for greater flexibility, such as replacing it with alternative backends (eg, something based on the OS keyring/keychain) while still allowing encrypted signon data to live in signons.sqlite (and use storage-mozStorage.js).
Comment 1 Justin Dolske [:Dolske] 2009-06-19 17:35:38 PDT
Created attachment 384187 [details] [diff] [review]
Patch v.1 (WIP)

First pass, mostly done.

This basically just moves _encrypt() / _decrypt() out of nsLoginManager.js and into crypto-SDR.js.

* Adds a new nsILoginManagerCrypto interface
* Cleans up error handling. I noticed all the userCanceled stuff wasn't really needed. We always just threw an exception when we saw that it was true, so now the exception is just thrown earlier.

Probably not worth changing storage-Legacy to use this.
Comment 2 Justin Dolske [:Dolske] 2009-09-04 17:29:29 PDT
Created attachment 398796 [details] [diff] [review]
Patch v.2
Comment 3 Justin Dolske [:Dolske] 2009-09-28 20:34:42 PDT
Created attachment 403411 [details] [diff] [review]
Patch v.3

Updated to tip.
Comment 4 Justin Dolske [:Dolske] 2009-10-14 13:33:51 PDT
Comment on attachment 403411 [details] [diff] [review]
Patch v.3

Let's run this by zpao too, and see if that will spur mconnor into a review. :)
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-10-14 16:51:48 PDT
Comment on attachment 403411 [details] [diff] [review]
Patch v.3

>+++ b/toolkit/components/passwordmgr/src/crypto-SDR.js
>+    __logService : null, // Console logging service, used for debugging.
>+    get _logService() {
>+        if (!this.__logService)
>+            this.__logService = Cc["@mozilla.org/consoleservice;1"].
>+                                getService(Ci.nsIConsoleService);
>+        return this.__logService;
>+    },
>+
>+    __decoderRing : null,  // nsSecretDecoderRing service
>+    get _decoderRing() {
>+        if (!this.__decoderRing)
>+            this.__decoderRing = Cc["@mozilla.org/security/sdr;1"].
>+                                 getService(Ci.nsISecretDecoderRing);
>+        return this.__decoderRing;
>+    },
>+
>+    __utfConverter : null, // UCS2 <--> UTF8 string conversion
>+    get _utfConverter() {
>+        if (!this.__utfConverter) {
>+            this.__utfConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+                                  createInstance(Ci.nsIScriptableUnicodeConverter);
>+            this.__utfConverter.charset = "UTF-8";
>+        }
>+        return this.__utfConverter;
>+    },
>+
>+    _utfConverterReset : function() {
>+        this.__utfConverter = null;
>+    },
>+
>+    __observerService : null,
>+    get _observerService() {
>+        if (!this.__observerService)
>+            this.__observerService = Cc["@mozilla.org/observer-service;1"].
>+                                     getService(Ci.nsIObserverService);
>+        return this.__observerService;
>+    },

Do we want to start using XPCOMUtils.defineLazy(Service)Getter for these? It's not how the other password manager code is written (yet), but we might be good to start using it. Granted, it'll mean we have to do it in the function, not the prototype. Maybe that can be something we do all at once instead.

>+    _debug        : false, // mirrors signon.debug

Nit: No need for all those spaces

>+++ b/toolkit/components/passwordmgr/src/storage-mozStorage.js
>     _encryptLogin : function (login) {
>+        let encUsername, encPassword;
>+        encUsername = this._crypto.encrypt(login.username);
>+        encPassword = this._crypto.encrypt(login.password);

let encUsername = ...;
let encPassword = ...;

>@@ -1149,26 +1083,27 @@
>+                // user might have canceled master password entry, just ignore.

Nit: Start your sentence with a capital letter ;)

r=zpao with nits fixed & assuming all the tests pass. I'll leave the defineLazyGetter change up to you.
Comment 6 Justin Dolske [:Dolske] 2009-12-13 17:01:33 PST
(In reply to comment #5)

> Do we want to start using XPCOMUtils.defineLazy(Service)Getter for these?

Eh, maybe, just to become consistent with other code. Spin off a bug if you'd like.

I fixed the other nits.
Comment 7 Justin Dolske [:Dolske] 2009-12-13 17:06:07 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/ec5877cfb217
Comment 8 Henrik Gemal 2009-12-15 00:17:13 PST
just updated to latest nightly and hit this:

Failed to load XPCOM component: C:\Program Files\Mozilla\Mozilla Thunderbird\Beta\components\crypto-SDR.js
Error: Cc['@mozilla.org/login-manager/crypto/SDR;1'] is undefined
Source File: file:///C:/Program%20Files/Mozilla/Mozilla%20Thunderbird/Beta/components/storage-mozStorage.js
Line: 72
Comment 9 Phil Ringnalda (:philor) 2009-12-15 00:26:01 PST
Fixed in http://hg.mozilla.org/comm-central/rev/0fa0825af0ac

Note You need to log in before you can comment on or make changes to this bug.