499417 - Refactor login manager's crypto code

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

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]]

Attachment: 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]]

Attachment: Patch v.2

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Updated to tip.

Comment 4

[Justin Dolske [:Dolske]]

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 bugmail, email directly)]

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]]

(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]]

Pushed http://hg.mozilla.org/mozilla-central/rev/ec5877cfb217

Comment 8

[Henrik Gemal]

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)]

Fixed in http://hg.mozilla.org/comm-central/rev/0fa0825af0ac