603489 - Simplify crypto by removing keypair and cryptometa and use passphrase

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ed Lee :Mardak]

https://wiki.mozilla.org/Services/Sync/SimplifyCrypto

Comment 1

[Ed Lee :Mardak]

Attachment: wip v1

go go go!

Comment 2

[Ed Lee :Mardak]

Comment on attachment 482388 [details] [diff] [review]
wip v1

>+++ b/services/sync/modules/constants.js
> DESKTOP_VERSION_OUT_OF_DATE:           "error.sync.reason.desktop_version_out_of_date",
Need to remove this constant.

>+++ b/services/sync/modules/engines.js
>-      resp.failureCode = ENGINE_METARECORD_DOWNLOAD_FAIL;
>-        resp.failureCode = ENGINE_METARECORD_UPLOAD_FAIL;
These constants should probably be removed too.

>-  wipeServer: function wipeServer(ignoreCrypto) {
>+  wipeServer: function wipeServer() {
I don't think I searched for cleaning up these callers.

>+++ b/services/sync/modules/identity.js
>+  get hmacKey() {
>+    return Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, this.passwordUTF8);
>+  get symKey() {
>+    // TODO PBKDF2
>+    return Utils.sha1(this.passwordUTF8).slice(0, 32);
Perhaps memoize? Especially with strengthening.

>+++ b/services/sync/modules/service.js
>   _verifyPassphrase: function _verifyPassphrase()
>-        if (result)
>-          this._needUpdatedKeys = true;
This removes the utf8/not passphrase verification. Not sure if both .password and .passwordUTF8 are needed if we don't care about this anymore ?

>+        let item = new Resource(); // TODO decrypt some data
If meta/global is encrypted at some point, that would probably be a useful record to test against..

>-      // XXX Bug 531005 Wait long enough to allow potentially another concurrent
>-      // sync to finish generating the keypair and uploading them
>-      Sync.sleep(15000);
Not sure if this is still needed. Didn't think about it toooo much. This was needed to avoid two open firefoxes both triggering on idle and both deciding that they need to wipe the server perhaps due to incompat storage version. But both clients will be encrypting with the same passphrase, so.. not needed?

>-        Status.sync = SETUP_FAILED_NO_PASSPHRASE;
Is this used elsewhere?

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: wip v2

Improve upon Ed's WIP, mostly by making almost all tests pass.

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: pbkdf2 API in WeaveCrypto v0

Expose deriveKeyFromPassphrase in WeaveCrypto as a public method. r- because no tests.

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 484004 [details] [diff] [review]
wip v2

>diff --git a/services/sync/modules/identity.js b/services/sync/modules/identity.js
...
>+  get hmacKey() {
>+    return Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, this.passwordUTF8);
>+  },
>+
>+  get symKey() {
>+    // TODO need fallback for Fx 3.5/3.6
>+    if (!this._symKey)
>+      this._symcKey = Svc.Crypto.deriveKeyFromPassphrase(this.passwordUTF8,
>+                                                         Service.syncID);
>+    return this._symKey;
>+  },
>+

Been thinking about this some more. Adding this property to the Identity object seems wrong. Instead we should just have a separate identity that contains the key within its password field. It would be created / updated by Weave.Service. That way it gets securely persisted in the password manager and we don't create an unnecessary dependence from identity.js to service.js. This would also be the way to start caching keys locally in a multi-key scenario like the one we have now.

Comment 6

[juan becerra [:juanb]]

Nominating for blocking at least to evaluate whether this is feasible for Fx4.

Comment 7

[Ragavan S [:rags]]

Just want to make a note that we will need to coordinate this change with Firefox Home and Fennec as well.

Comment 8

[Mike Connor [:mconnor]]

Fennec will get this at the same time as Firefox, but we need to coordinate timing between add-on and trunk and FxHome (bug 605724)

Comment 9

[Richard Newman [:rnewman]]

Attachment: Updated version of Philipp's WIP.

Attached is an updated version of this WIP, with the following changes:

* Most callers use deriveEncodedKeyFromPassphrase, which does the additional work to get a usable return value from NSS and encode it as base64.

* Added a check-interactive make target.

* Integrated the _derive => derive name change and ThreadedCrypto export.

* Added a descriptive string to Utils.lock usage, which is very helpful in debugging. Altered tests to match.

* Small change to sync/tests/unit/Makefile; relative paths were wrong for this repo structure.

* Added a simple test for deriveEncodedKeyFromPassphrase.


All tests pass, with the exception of test_utils_stackTrace, which seems to be incorrect. I'm not worried about that one.


This is the basis of my attempt to implement our written-up proposal, which can be found at

  https://wiki.mozilla.org/Services/Sync/SimplifiedCryptoProposal

More to come.

Comment 10

[Philipp von Weitershausen [:philikon]]

Discussed some of this IRC already:

(In reply to comment #9)
> Attached is an updated version of this WIP, with the following changes:
> 
> * Most callers use deriveEncodedKeyFromPassphrase, which does the additional
> work to get a usable return value from NSS and encode it as base64.
> 
> * Added a check-interactive make target.

I suggest doing this in a separate patch, if not a separate bug.

> * Integrated the _derive => derive name change and ThreadedCrypto export.

This is obsolete now as I'm sure you know ;)

> * Added a descriptive string to Utils.lock usage, which is very helpful in
> debugging. Altered tests to match.
> 
> * Small change to sync/tests/unit/Makefile; relative paths were wrong for this
> repo structure.

Again, unrelated to this bug, so let's do this in a separate patch, if not separate bug (e.g. could be combined with the Makefile change from above).

> * Added a simple test for deriveEncodedKeyFromPassphrase.

I suggest doing all the changes to services/crypto/ in a separate patch, too. Smaller patches are easier to review, smaller commits make it easier to track down problems (e.g. test failures) later.

> All tests pass, with the exception of test_utils_stackTrace, which seems to be
> incorrect. I'm not worried about that one.

I am! Is this breaking without your changes, too? If so, can you file a bug so that it gets fixed.

Comment 11

[Richard Newman [:rnewman]]

(In reply to comment #10)

> I suggest doing all the changes to services/crypto/ in a separate patch, too.
> Smaller patches are easier to review, smaller commits make it easier to track
> down problems (e.g. test failures) later.

I'll rebase my changes on top of the latest crypto de-threadification, and submit new patches for review.


> > All tests pass, with the exception of test_utils_stackTrace, which seems to be
> > incorrect. I'm not worried about that one.
> 
> I am! Is this breaking without your changes, too? If so, can you file a bug so
> that it gets fixed.

Done: Bug 608324.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Why do we extract keys, convert them to strings, base-64-encoded them, then undo all of that to re-import them into NSS? Ideally the sync key would get imported into NSS, PK11_UnwrapSymKey and PK11_WrapSymKey would be used to unwrap the other keys into NSS PK11SymKey objects, and those PK11SymKey objects would get passed around without these conversions and import/export operations. This is how typical (C/C++) users of NSS do things.

Generally, any code that uses PK11_ExtractKeyValue is probably working sub-optimally. I am willing to help you out with improving the code in this area but I am not so familiar with (the limitations of) jsctypes so I don't know if there are Javascript-specific limitations that prevent the normal approach from being taken.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Since there are only a small number of collections, it may actually be reasonable to create two PK11Context objects for each collection (one for AES, one for HMAC) after unwrapping their keys, pre-load them with the keys, and then pass the PK11Context objects around instead of the base64-encoded keys. This should avoid significant overhead as there quite a few expensive calls (locking, memory allocation, and sometimes even internal encryption/decryption operations) inside of NSS whenever you create a new context or create a new key object.

Comment 14

[Richard Newman [:rnewman]]

(In reply to comment #12)
> Why do we extract keys, convert them to strings, base-64-encoded them, then
> undo all of that to re-import them into NSS?

I'm new here, so I don't have a good answer for this :)

My personal perspective, which I formed while getting deriveKeyFromPassphrase to work, is that:

* the existing codebase isn't all that flexible (e.g., almost every function expects its input as base64, not an NSS object)

* understanding how, when, and why NSS actually populates things like PK11SymKeyStrs is not straightforward

For a first draft I didn't want to make more changes than I needed to -- particularly when those changes would involve attempting to manage those js-ctypes lifecycles correctly, or introducing breakage into working functions.

These keys get loaded from various sources, stored on servers, schlepped around (possibly between thread boundaries, though it seems we've shelved that), etc., and I'm keen to evaluate the crypto proposal alone, rather than proposal + other code changes.

I totally agree that there's a huge amount of redundant work going on, and I have TODOs sitting in the code to resolve that. I'm personally intending to implement the basic flow before I add caching and switch to more efficient mechanisms for passing keys around.

I'll definitely take you up on your offer of help if I need to :D

Comment 15

[Ed Lee :Mardak]

Various keys were stored on the server before, so they needed to be transformed into a string for retrieval then use. If we do all the crypto key generation, etc locally, potentially we could pass around ctypes references.... ?

Comment 16

[Richard Newman [:rnewman]]

(In reply to comment #15)
> Various keys were stored on the server before, so they needed to be transformed
> into a string for retrieval then use. If we do all the crypto key generation,
> etc locally, potentially we could pass around ctypes references.... ?

Everything but the sync key itself (i.e., the default key, HMAC, collection-specific keys...) will be stored on the server, just as before.

We'll cache them locally, certainly.

Comment 17

[Richard Newman [:rnewman]]

Attachment: Enormous patch that implements most of the necessary work.

This isn't finished, or even cleaned up, but Philipp offered to take a look.

Here are my notes about this:

* This has had only minimal real-life testing. In particular, the interaction of multiple clients has not been tested. I have no idea if the whole "PBKDF2 to yield the same key" mechanism is correct.
* It could do with some more unit tests, too.
* There's still a lot of excess logging, *including of passphrases and keys*.
* Keys are not persisted locally. That means every startup involves a fetch of keys.
* There's no mechanism yet for adding new collection-specific keys (or uploading them), but the code itself should work.
* We pass in '16' as the key length to the PBKDF2 derivation process in order to get a 25-alphadigit output. I'm still not convinced by my base36 implementation, though.
* Pretty much everything is 256-bit. The exception is the sync key itself, which is kept to 128-bit (and base36-encoded) for typeability.
* All keys are stored as 5-grouped base36. They're just going to be used as string input to SHA-256 anyway...
* The UI has not been touched. That means "generate a new key" gives you 20 chars, as before... and then we PBKDF2 that up to 25.
* There's still a lot of redundant work going on:
  * Base64 is used to shuttle almost everything around.
  * info/collections is fetched twice on startup, because both verifyLogin and verifyPassphrase pull it (for different reasons). At least checking this should cause the local key cache to be invalidated on sync if necessary.
  * NSS key objects are re-built all the time.
 
This diff encompasses a whole bunch of my local Git commits. For final review, these will be cleaned up and partitioned into chunks that make sense (at least, I'll try!).

Comment 18

[Philipp von Weitershausen [:philikon]]

Comment on attachment 488137 [details] [diff] [review]
Enormous patch that implements most of the necessary work.

Some general feedback without digging in too deep:

* It would be good to split the changes to services/crypto off into a separate patch before we do the final review round.

* Let's not add non-cryptographic routines to WeaveCrypto. I should stress that WeaveCrypto.js right now is an equivalent implementation of the binary component that we still need on Firefox 3.5/3.6. Promoting deriveKeyFromPassphrase() to an API call is a worthwile exception we can make, but we'll have to ship a pure JS implementation as well for Firefox 3.5/3.6! IOW wherever we use Svc.Crypto.deriveKeyFromPassphrase() in Sync, we need to check whether it exists first and if it doesn't, use the pure JS variant. The pure JS variant can live in util.js. Separate patch would be best :)

* So the various utility functions for representing the AES key in a human readable manner (octetsToRawBase36, interpretSyncKeyStringAsEncodedKey, etc.) shouldn't live in WeaveCrytpo. I would suggest services/sync/modules/util.js since they feel more Sync-specific than general purpose crypto routines. They also need individual unit tests. And since they're an atomic addition, let's make those a separate patch as well. Atomicity ftw! :)

* Please follow the coding style (https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide). Also, for methods in new Sync code we have adopted the style

  foobar: function foobar(args) {

Comment 19

[Richard Newman [:rnewman]]

Thanks for the feedback, Philipp!

> * It would be good to split the changes to services/crypto off into a separate
> patch before we do the final review round.

Yup, for sure.
 
> * Let's not add non-cryptographic routines to WeaveCrypto.
> * Please follow the coding style...

OK, will do!

Comment 20

[Richard Newman [:rnewman]]

Attachment: Updated diff. Just services/crypto.

Changes since the last one:

* Split patch into three parts.

* Ripped all PKI stuff out of WeaveCrypto.js, and corresponding tests.

* Moved a lot of code into util.js, leaving WeaveCrypto *almost* unmolested; only two changes:

  * expandData is split out from encodeBase64, which is no longer present.

  * deriveKeyFromPassphrase loses the underscore, and now accepts two new arguments: whether to extract and return the key data, rather than an unfinished pointer; and the length of the key to derive (in bytes). Default behavior when called with two args is unchanged.

* Added tests for PlacesItem; also exercises CollectionKeys.

* Fix for per-collection key generation.

* JS style changes (incomplete, I'm sure).

The other two parts of the patch will follow momentarily.


Remaining work items include, but are not limited to:

* Persist CollectionKeys locally
* Reduce the amount of Base64 usage to communicate between functions
* Test for CK.updateNeeded
* Remove debug printing (particularly of passphrases and keys)
* Generate an appropriate 5x5 key in the UI, not a 4x5.

Comment 21

[Richard Newman [:rnewman]]

Attachment: Everything but crypto and sync.

Comment 22

[Richard Newman [:rnewman]]

Attachment: Updated diff. Just services/sync.

Comment 23

[Philipp von Weitershausen [:philikon]]

(In reply to comment #20)
> Remaining work items include, but are not limited to:
> 
> * Persist CollectionKeys locally
> * Reduce the amount of Base64 usage to communicate between functions
> * Test for CK.updateNeeded
> * Remove debug printing (particularly of passphrases and keys)
> * Generate an appropriate 5x5 key in the UI, not a 4x5.

I'm sure this is already on your list, I'm just writing this down so I won't forget:

We also need individual unit tests for WeaveCrypto.deriveKeyFromPassphrase now that it's a public API and has a new code path, as well as unit tests for the various new helpers (e.g. the base36 stuff).

Comment 24

[Richard Newman [:rnewman]]

For the record, as I'm going through my to-do list: work for this bug will not address any pervasive efficiency improvements (Bug 610914), or even some improvements to this code that are not blockers for getting this out the door (Bug 610913).

New megadiff coming soon.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Code freeze for Firefox 4 Beta 8 is aimed at Monday Nov 22nd, Fennec 2 Beta 3 on Tuesday Nov 23rd, so we should be aiming to have this resolved by Monday.

Comment 26

[Richard Newman [:rnewman]]

Attachment: Updated diff. Just services/crypto.

Tests for key derivation live in sync, along with the JS implementation and cross-testing.

Comment 27

[Philipp von Weitershausen [:philikon]]

Comment on attachment 490998 [details] [diff] [review]
Updated diff. Just services/crypto.

r- for lack of tests

>-    _deriveKeyFromPassphrase : function (passphrase, salt) {
>-        this.log("_deriveKeyFromPassphrase() called.");
>+        
>+    /**
>+     * Returns a PK11SymKeyStr: 
>+     * http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/secmodti.h#149
>+     * 
>+     * or, if returnKeyData is truthy, return the keyData object instead.
>+     */
>+    deriveKeyFromPassphrase : function (passphrase, salt, keyLength, returnKeyData) {
>+        this.log("deriveKeyFromPassphrase() called.");

Unless I'm mistaken, this method has zero test coverage now. Previously it was private and implicitly tested in test_crypto_keypair and test_crypto_rewrap, but those tests were removed. Moreover we need to exercise the new code paths introduced to the method (the ones dealing with keyLength, returnKeyData). I imagine parts of the tests could easily be copied from bug 610749 (services/sync/tests/unit/test_utils_pbkdf2)

>         let passItem = this.makeSECItem(passphrase, false);
>         let saltItem = this.makeSECItem(salt, true);
>-
>+        

Nit: trailing whitespace introduced

> function run_test() {
>   // First, do a normal run with expected usage... Generate a random key and
>   // iv, encrypt and decrypt a string.
>   var iv = cryptoSvc.generateRandomIV();
>   do_check_eq(iv.length, 24);
>-
>+ 

Nit: trailing whitespace introduced

Comment 28

[Philipp von Weitershausen [:philikon]]

(In reply to comment #26)
> Tests for key derivation live in sync, along with the JS implementation and
> cross-testing.

Ugh, sorry, didn't see this. As discussed on IRC, r=me if we have the tests that exercise WeaveCrypto in services/crypto as well.

Comment 29

[Richard Newman [:rnewman]]

Attachment: Updated diff. Just services/crypto.

Just for you, I extracted a bunch of tests from sync (and added some more) :)

Won't apply this just yet -- needs to go with a bunch of sync changes.

Comment 30

[Richard Newman [:rnewman]]

Attachment: Tests for PlacesItems. Separated out for ease of reading.

Comment 31

[Richard Newman [:rnewman]]

Attachment: UI changes for hyphenation and so forth.

Will only be testable in 3.x for now -- 4.x's UI won't call our methods (see Bug 597427).

Comment 32

[Richard Newman [:rnewman]]

Attachment: Everything else.

I think this is everything. Please give it a whirl!

Comment 33

[Philipp von Weitershausen [:philikon]]

Comment on attachment 491085 [details] [diff] [review]
Tests for PlacesItems. Separated out for ease of reading.

>diff --git a/services/sync/tests/unit/test_records_bookmark.js b/services/sync/tests/unit/test_records_bookmark.js

Nit: test_bookmark_record.js (to be consistent with other bookmark engine tests)

>+  passphrase.password = "ABCDE-ABCDE-ABCDE-ABCDE-ABCDE";

I realise the passphrase is just an opaque string, but I still found it confusing seeing dashes here.

>+    let prepareBookmarkItem = function(u) {

Could just do function prepareBookmarkItem() {...} ;) It also might not have to live in run_test() where it's two indention blocks down.

>+    server = httpd_setup({"/storage/bookmarks/foo": crypted_resource_handler});

Why exactly do we need a server for this test? I'm pretty sure we don't, otherwise you would've had to set Weave.Service.serverURL = "http://localhost:8080/" somewhere. ;)

>+    log.info("Checking getTypeObject");
>+    do_check_eq(placesItem.getTypeObject(placesItem.type), Bookmark);
>+    do_check_eq(bookmarkItem.getTypeObject(bookmarkItem.type), Bookmark);

I wouldn't view getTypeObject as the interesting bit to test here. It merely serves as a helper for PlacesItem.decrypt() to decide how to monkey patch itself. *That's* the interesting bit really and what makes PlacesItem special from ever other type record: decrypting some unknown WBO from the bookmarks collection and voila, it changes its prototype according to the type of places item (bookmark, folder, etc.).

>+    bookmarkItem.encrypt(passphrase);
...
>+    let payload = bookmarkItem.decrypt(passphrase);
...
>+    bookmarkItem.encrypt();
...
>+    do_check_eq(bookmarkItem.decrypt(CollectionKeys._default.keyStr).stuff, "my payload here");

I'm confused about the API here. Looks like encrypt() takes a parameter but it's optional? And decrypt() at some point takes an identity object, at another point the key as an actual string? (Also, CollectionKeys._default.keyStr doesn't seem particularly elegant as a way to access the default key.) That seems very confusing. Before we added the keyUri parameter as a short-circuiting hack, decrypt() and encrypt() would just take one parameter: an identity object containing the passphrase (typically the "WeaveCryptoID" identity). That seems very elegant and predictable. Why can't we keep that approach?

Also, since this is an API provided by CryptoWrapper, it seems like these tests should really be done in test_records_crypto.

>+    try {
>+      bookmarkItem.decrypt(CollectionKeys._default.keyStr);
>+    }
>+    catch (ex) {
>+      err = ex;
>+    }

Nit: cuddle catch, finally, else, etc. in braces. See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Comment 34

[Richard Newman [:rnewman]]

(In reply to comment #33)
> >diff --git a/services/sync/tests/unit/test_records_bookmark.js b/services/sync/tests/unit/test_records_bookmark.js
> 
> Nit: test_bookmark_record.js (to be consistent with other bookmark engine
> tests)

Hmm, I was being consistent with other records tests:

services/sync/tests/unit/test_records_crypto.js    
services/sync/tests/unit/test_records_bookmark.js
services/sync/tests/unit/test_records_wbo.js

Do you still want this changed? 

> 
> >+  passphrase.password = "ABCDE-ABCDE-ABCDE-ABCDE-ABCDE";
> 
> I realise the passphrase is just an opaque string, but I still found it
> confusing seeing dashes here.

Fixed.
 
> Could just do function prepareBookmarkItem() {...} ;)...

Fixed.
 
> Why exactly do we need a server for this test? I'm pretty sure we don't

Good catch. Dead code. Removed.

> I wouldn't view getTypeObject as the interesting bit to test here.

It's something that needs testing: these tests exercise that a PlacesItem's type is preserved across encryption and decryption.

> *That's* the interesting bit really and what makes PlacesItem special
> from ever other type record: decrypting some unknown WBO from the bookmarks
> collection and voila, it changes its prototype according to the type of places
> item (bookmark, folder, etc.).

I'll add something to exercise this. (Assuming it works... no existing tests for this :D)

> I'm confused about the API here. Looks like encrypt() takes a parameter but
> it's optional? 

Correct.

WBOs know how to decrypt themselves: by asking CollectionKeys for the appropriate key for their collection (defaulting to the default key).

If you pass in a string, that'll be used as the lookup for SymmetricKeys instead.

Nice and simple.

> And decrypt() at some point takes an identity object, at another
> point the key as an actual string?

Always a key string, if anything. Where do you see the identity object?

You'll see a test in this file where the default key is used to try to decrypt a WBO with a per-collection key, which fails.

> (Also, CollectionKeys._default.keyStr
> doesn't seem particularly elegant as a way to access the default key.)

You never need to access the default key in normal use; this is just for the test, so we can force a crypto failure.

I thought it would be more meaningful than simply using a random key, because default/per-collection keys would be a more worrying collision.

> That
> seems very confusing. Before we added the keyUri parameter as a
> short-circuiting hack, decrypt() and encrypt() would just take one parameter:
> an identity object containing the passphrase (typically the "WeaveCryptoID"
> identity). That seems very elegant and predictable. Why can't we keep that
> approach?

This is even more elegant: no parameters. :) 

Do you still have an objection after reading my explanation above?

> Also, since this is an API provided by CryptoWrapper, it seems
> like these tests
> should really be done in test_records_crypto.

The encryption part, yes. I will move those.

> Nit: cuddle catch, finally, else, etc. in braces. See
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Gotcha.

Thanks for the review! Will submit an updated patch this morning.

Comment 35

[Philipp von Weitershausen [:philikon]]

(In reply to comment #34)
> (In reply to comment #33)
> > >diff --git a/services/sync/tests/unit/test_records_bookmark.js b/services/sync/tests/unit/test_records_bookmark.js
> > 
> > Nit: test_bookmark_record.js (to be consistent with other bookmark engine
> > tests)
> 
> Hmm, I was being consistent with other records tests:
> 
> services/sync/tests/unit/test_records_crypto.js    
> services/sync/tests/unit/test_records_wbo.js

These two are base_records/* files (which we're going to combine to records.js or so at the top level, bug 609421)

> Do you still want this changed? 

Yes, because we're going to move the type_records/* files in with the engines/* files (bug 609421) and then the naming scheme will make sense :)

> > I wouldn't view getTypeObject as the interesting bit to test here.
> 
> It's something that needs testing: these tests exercise that a PlacesItem's
> type is preserved across encryption and decryption.

Ok. My point was that getTypeObject() is an implementation detail to PlacesItem.decrypt()...

> > *That's* the interesting bit really and what makes PlacesItem special
> > from ever other type record: decrypting some unknown WBO from the bookmarks
> > collection and voila, it changes its prototype according to the type of places
> > item (bookmark, folder, etc.).
> 
> I'll add something to exercise this. (Assuming it works... no existing tests
> for this :D)

Great, thanks! (Missing existing tests is no excuse ;), we have to get to better test coverage somehow...)

> > I'm confused about the API here. Looks like encrypt() takes a parameter but
> > it's optional? 
> 
> Correct.
> 
> WBOs know how to decrypt themselves: by asking CollectionKeys for the
> appropriate key for their collection (defaulting to the default key).
> 
> If you pass in a string, that'll be used as the lookup for SymmetricKeys
> instead.

So wbo.decrypt()/encrypt() will taken optional parameter that's a key *name*, yes? Or the key string itself? I'm confused.

> > And decrypt() at some point takes an identity object, at another
> > point the key as an actual string?
> 
> Always a key string, if anything. Where do you see the identity object?

In the test:

+    bookmarkItem.encrypt(passphrase);

> You'll see a test in this file where the default key is used to try to decrypt
> a WBO with a per-collection key, which fails.
> 
> > (Also, CollectionKeys._default.keyStr
> > doesn't seem particularly elegant as a way to access the default key.)
> 
> You never need to access the default key in normal use; this is just for the
> test, so we can force a crypto failure.
> 
> I thought it would be more meaningful than simply using a random key, because
> default/per-collection keys would be a more worrying collision.

That makes sense to me, but please comment the hell out of it so that other people reading or refactoring those tests in the future will know the intention.

> > That
> > seems very confusing. Before we added the keyUri parameter as a
> > short-circuiting hack, decrypt() and encrypt() would just take one parameter:
> > an identity object containing the passphrase (typically the "WeaveCryptoID"
> > identity). That seems very elegant and predictable. Why can't we keep that
> > approach?
> 
> This is even more elegant: no parameters. :) 

+1

> Do you still have an objection after reading my explanation above?

Let me have a closer look at the actual implementation in the other patch. Regarding this test I withdraw my objections so long as the intentions are well documented.

> > Also, since this is an API provided by CryptoWrapper, it seems
> > like these tests
> > should really be done in test_records_crypto.
> 
> The encryption part, yes. I will move those.

Great!

Comment 36

[Richard Newman [:rnewman]]

> Yes, because we're going to move the type_records/* files in with the engines/*
> files (bug 609421) and then the naming scheme will make sense :)

Gotcha.
 
> Great, thanks! (Missing existing tests is no excuse ;), we have to get to
> better test coverage somehow...)

Filed Bug 612917 per IRC discussion.

  
> So wbo.decrypt()/encrypt() will taken optional parameter that's a key *name*,
> yes? Or the key string itself? I'm confused.

I introduce the concept of a "key string", which is the input to SHA256 to produce the HMAC and encryption keys. It's the thing that looks like a key, not a name, but it's not directly used for encryption.

The SymmetricKeys singleton is basically a memoization of this operation:

  keyStr => (keyStr, HMAC = sha256("hmac": + keyStr), enc = ..., ...)

CollectionKeys uses SymmetricKeys as a slave to get those key bundles.

The CollectionKeys singleton is this:

  collection => (keyStr, HMAC, enc, ...)

When you call

  foo.encrypt()

foo itself looks at foo.collection to find its collection (from its URL), then asks CollectionKeys for the symmetric key. If there's a per-collection mapping, you get that; if not, you get the default key.

When you call

  foo.encrypt("some string")      // E.g., the passphrase

that string gets passed through to SymmetricKeys. This allows you to encrypt an object with a key string of your choosing... which is how we encrypt the keys themselves, using the passphrase.


> > Always a key string, if anything. Where do you see the identity object?
> 
> In the test:
> 
> +    bookmarkItem.encrypt(passphrase);

Sorry, forgot about that little bit of magic. It'll handle either one.

  
  _keyStr: function (k) {
             if (!k)
               throw new Error("No key string provided to key manager.");
             return k.password || k;
           },

> 
> > You'll see a test in this file where the default key is used to try to decrypt
> > a WBO with a per-collection key, which fails.
> > 
> > > (Also, CollectionKeys._default.keyStr
> > > doesn't seem particularly elegant as a way to access the default key.)
> > 
> > You never need to access the default key in normal use; this is just for the
> > test, so we can force a crypto failure.
> > 
> > I thought it would be more meaningful than simply using a random key, because
> > default/per-collection keys would be a more worrying collision.
> 
> That makes sense to me, but please comment the hell out of it so that other
> people reading or refactoring those tests in the future will know the
> intention.

Done.

> > The encryption part, yes. I will move those.
> 
> Great!

Done. Will submit a patch with these tests, and update whatever else I've touched.

Thanks!

Comment 37

[Richard Newman [:rnewman]]

Attachment: Bookmarks tests, renamed and shifted into crypto.

Included the CryptoWrapper tests here, though they won't necessarily make much sense without the rest of the patches.

Comment 38

[Philipp von Weitershausen [:philikon]]

(In reply to comment #36)
> > So wbo.decrypt()/encrypt() will taken optional parameter that's a key *name*,
> > yes? Or the key string itself? I'm confused.
> 
> I introduce the concept of a "key string", which is the input to SHA256 to
> produce the HMAC and encryption keys. It's the thing that looks like a key, not
> a name, but it's not directly used for encryption.

Summarizing IRC discussion: to streamline the API and allow caching keys locally in the future (bug 610913), this functionality will be rolled into a KeyBundle object. It'll be instantiated from the key string and does the HMAC + encryption key memoization. Basically it'll incorporate the KeyManager functionality that we have right now. At a later point we can then make these KeyBundles inherit from Identity and add persistency logic to them.

decrypt() and encrypt() will take a key bundle object as an optional argument, defaulting to the default key bundle when you don't pass it in.

Comment 39

[Richard Newman [:rnewman]]

(In reply to comment #38)
> At a later point we can then make these
> KeyBundles inherit from Identity and add persistency logic to them.

In fact, the draft implementation (in the Git repo you're looking at) already has KeyBundle inherit from Identity, so it's only the persistence part that will be covered by Bug 610913.


> decrypt() and encrypt() will take a key bundle object as an optional argument,

The current code doesn't pass key bundles around: it passes keyStrs (the "names" of KeyBundles), which are used to look up or construct KeyBundles.

The only time this actually occurs is when the passphrase/sync key is used to encrypt storage/crypto/keys itself. That is, the only relevant code is stuff like:
  

  // Actually happens in service.js, so doesn't look quite like this.
  let p = Weave.Service.passphrase;  // An Identity.

  ...
  storageKeys.encrypt(p.password);           // Pass in the string.


If we required KeyBundles to be used, this code would change to:

  ...
  let bundle = new KeyBundle(p.password);
  storageKeys.encrypt(bundle);


I'm happy to do either way; the current way is simpler for calling code, but I understand the elegance of passing in the bundle.

Thoughts?


> defaulting to the default key bundle when you don't pass it in.

Not quite (depending on your definition of "default"): CryptoWrapper.decrypt and encrypt will default to using the appropriate key bundle *for the object's collection*, which will usually be the default key bundle.

(Confusing conflation of "default" here, I know.)

That is, you don't need to know an object's collection and do the bundle lookup yourself to determine if there's a collection-specific key -- the WBO does it for you. Calling code just calls wbo.encrypt(), and the right thing happens.

Comment 40

[Ed Lee :Mardak]

(In reply to comment #39)
>   storageKeys.encrypt(p.password);           // Pass in the string.
I believe logging of stack traces will include string arguments, so people posting logs might accidentally send in a passphrase.

Comment 41

[Philipp von Weitershausen [:philikon]]

(In reply to comment #40)
> (In reply to comment #39)
> >   storageKeys.encrypt(p.password);           // Pass in the string.
> I believe logging of stack traces will include string arguments, so people
> posting logs might accidentally send in a passphrase.

Good point. We already discussed this on IRC anyway and we'll change the API back to taking identity objects, well, actually the KeyBundle objects which are identities on steroids.

Comment 42

[Philipp von Weitershausen [:philikon]]

Some general comments before I hand review over to mconnor:

* The new KeyBundle object looks great, but I would suggest making the constructor less DWIM-y about null/false/string values. My suggestion: Let's never auto-generate the keyStr. Instead, expose generateRandom as a method (e.g. setRandomKeyStr) and use it in the appropriate places. According to grep there aren't that many ;)

* For the KeyBundle constructor I would also suggest keeping the same argument order as the Identity() constructor. I mean, we're calling it later anyway. So that instantiating a KeyBundle looks very much like making an Identity. It's purely aesthetics, but why randomly change the order when we don't have to.

* Is there a reason why CollectionKeyManager, even though it essentially represents the key record on the server, isn't a WBORecord itself (=inherits from it)? That would make a bit more sense to me than the asWBO() method, and it would nicely encapsulate the get()/put() part as well. It would also make the rather awkward setContents/updateContents/generateNewKeys obsolete or at least more straight forward. Right now we're passing around information that could just as well be encapsulated in total in CollectionKeys (the instance of CollectionKeyManager).

* You removed the passphrase check from Service.login() and instead introduced Service._loginAndSetup(). Thing is, Service.login() is called from UI code and we need it to check the passphrase. Can't change semantics of UI facing methods.

* Style nits:
  - lots of uncuddled elses etc.
  - lots of extra trailing whitespace and empty lines after function definitions. pls remove
  - not always wrapping at 80 chars
  - sometimes weird line wrapping when defining methods (should all be one line and always "foo: function foo()")
  - random whitespace and formatting changes (make it hard to review)

Comment 43

[Philipp von Weitershausen [:philikon]]

Comment on attachment 491087 [details] [diff] [review]
Everything else.

>   // Keep both plaintext and encrypted versions of the id to verify integrity
>   set id(val) {
>     WBORecord.prototype.__lookupSetter__("id").call(this, val);
>+    
>+    // Workaround for bug I don't understand:
>+    // Error creating record: this.cleartext is undefined
>+    // JS Stack trace: ("aY~fCXK4nU")@crypto.js:118
>+    // < SyncEngine__createRecord("aY~fCXK4nU")@engines.js:338
>+    if (!this.cleartext) {
>+      this.cleartext = {};
>+    }
>     return this.cleartext.id = val;
>   },
> };

This seems indicative of a lurking bug. There are bigger fish to catch right now, but should definitely investigate before this lands!

Comment 44

[Richard Newman [:rnewman]]

About to start uploading some new patches.

I will not be updating the crypto patch that Philipp r'ed in comment 28. The only changes since then are a single line of reformatting, and the addition of my Contributor line.

Since the last set of patches:

* SymmetricKeys have been melded with Identity (and are now called KeyBundles), and are thus in a great place to be persisted.

* AES and HMAC keys are generated and stored in KeyBundles, eliminating a key derivation step. This makes bsmith happy.

* Encryption operations take KeyBundles, not key strings.

* The sync key is also stored in a KeyBundle, with the necessary derivation taking place. This is persisted using ID.get/set, and completely replaces service.js's use of Identity.

* There's more logging and more (and different) tests, as well as some cleanup.

* Keys are now hyphenated differently (see discussion with faaborg).

* Key derivation now uses SHA256-HMAC with descriptive tagging and a counter, chaining to produce the two keys. This makes bsmith happier.

There's possibly other stuff, too.

Comment 45

[Richard Newman [:rnewman]]

Note that there are more work items on the table:

* Considering whether to drop down to 128-bit encryption operations
* We'd like to use keys directly as byte arrays, rather than in any textual form, with encoding and decoding occurring during IO

I'd also like to address Philipp's concerns about the CollectionKeyManager in Comment #42. I'd need to think a little about how to lazily orchestrate the association of a singleton with a WBO, but I'm sure it could be done.

And more besides. Working on his login comment first.

Comment 46

[Richard Newman [:rnewman]]

Attachment: Crypto. Largely unchanged (see prior comment).

Comment 47

[Richard Newman [:rnewman]]

Attachment: Tests for PlacesItems.

Comment 48

[Richard Newman [:rnewman]]

Attachment: UI changes for hyphenation and so forth.

Comment 49

[Richard Newman [:rnewman]]

Attachment: Everything else.

Comment 50

[Richard Newman [:rnewman]]

I addressed Philipp's login comments: we now verify the passphrase in verifyLogin, by doing the whole key setup process. This required touching several tests.

mconnor, please review the last four patches. Thanks!

Comment 51

[Richard Newman [:rnewman]]

Also, for the record (viz Bug 602876), this is how I do key derivation:

      // First key.
      let u = this.username; 
      let k1 = Utils.makeHMACKey("" + HMAC_INPUT + u + "\x01");
      let enc = Utils.sha256HMACBytes(m, k1, h);
      
      // Second key: depends on the output of the first run.
      let k2 = Utils.makeHMACKey(enc + HMAC_INPUT + u + "\x02");
      let hmac = Utils.sha256HMACBytes(m, k2, h);

where

      HMAC_INPUT: "Sync-AES_256_CBC-HMAC256",

Comment 52

[Richard Newman [:rnewman]]

Attachment: Everything else, v3

(Marking old UI code as obsolete; apparently missed that earlier.)

This new patch includes two changes to base_records/crypto.js:

*    Remove workaround mentioned in Comment 43 on Bug 603489.
*    Spot HMAC failure in verifyAndFetchSymmetricKeys. Report login failure for the correct reason.

and one other small change that I think was omitted:

*    Tidying lock function name.

Comment 53

[Richard Newman [:rnewman]]

Attachment: Additional test for key derivation.

Making sure that Sync and Home agree on keys derived from the sync key.

Comment 54

[Richard Newman [:rnewman]]

Just imagine that this change is in "Everything else, v3".

diff --git a/services/sync/tests/unit/test_utils_lock.js b/services/sync/tests/unit/test_utils_lock.js
index 24b2683..fd8a4b1 100644
--- a/services/sync/tests/unit/test_utils_lock.js
+++ b/services/sync/tests/unit/test_utils_lock.js
@@ -3,10 +3,8 @@ Cu.import("resource://services-sync/util.js");
 
 // Utility that we only use here.
 
-// Crappy version of String.startsWith. Won't work if you have special chars in 
-// the tested string, yadda yadda. Sorry.
 function do_check_begins(thing, startsWith) {
-  if (!(thing && thing.substr && (thing.substr(0, startsWith.length) == startsWith)))
+  if (!(thing && thing.indexOf && (thing.indexOf(startsWith) == 0)))
     do_throw(thing + " doesn't begin with " + startsWith);
 }
 
(See Bug 560580.)

Comment 55

[Justin Dolske [:Dolske]]

Comment on attachment 491860 [details] [diff] [review]
Crypto. Largely unchanged (see prior comment).

Quick driveby:

>+    /**
>+     * Returns a PK11SymKeyStr: 
>+     * http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/secmodti.h#149
>+     * 
>+     * or, if returnKeyData is truthy, return the keyData object instead.
>+     */
>+    deriveKeyFromPassphrase : function (passphrase, salt, keyLength, returnKeyData) {

This isn't a very good API. :/

Different return types by argument is kind of frowny in its own, but more importantly we shouldn't have JS APIs that leak memory if they're not used carefully (other than those used internally to WeaveCrypto).

I think _deriveKeyFromPassphrase() should remain a private API, and you should create a new deriveKeydataFromPassphrase() API that basically does what you're doing here via the returnKeyData argument. More on this below.

>         // Bug 436577 prevents us from just using SEC_OID_PKCS5_PBKDF2 here
>+        // 
>+        // That was fixed long ago, so the following should work:
>+        //let pbeAlg = this.nss.SEC_OID_PKCS5_PBKDF2;
>+        //let cipherAlg = this.nss.SEC_OID_HMAC_SHA1;
>+        //let prfAlg = this.nss.SEC_OID_PKCS5_PBKDF2;

Just remove the old code an comments, they're just noise now.

>+        if (returnKeyData) {
>+          // Take the PK11SymKeyStr, returning the extracted key data.
>+          if (this.nss.PK11_ExtractKeyValue(symKey)) {
>+            throw this.makeException("PK11_ExtractKeyValue failed.", Cr.NS_ERROR_FAILURE);
>+          }
>+          let keyData = this.nss.PK11_GetKeyData(symKey);
>+          return keyData.contents;
>+        }
>         return symKey;
>     },

This will leak!

NSS allocates |symKey|, so it needs to be explicitly free'd. (Callers of _deriveKeyFromPassphrase() within WeaveCrypto already do this.)

But the key data's lifetime is tied to the lifetime of the key, so you can't just free symKey while returning .contents. You'll need to duplicate the bits into, say, a new JS string and return that to the caller. (Who in turn can be ignorant of allocation lifetimes, because it'll be GC'd like any other JS string).

Comment 56

[Justin Dolske [:Dolske]]

(In reply to comment #12)
> Why do we extract keys, convert them to strings, base-64-encoded them, then
> undo all of that to re-import them into NSS?

Yes, it's all quite silly, isn't it? :)

The shortish answer is that that's how it was done originally (I think even in Thunder's original OpenSSL based code?), as base64 strings are easy to pass around. It's worked ok so far, and so one's bothered to change it. Definitely known as not ideal, and that's one of the reasons I keep telling people not to use WeaveCrypto as a general-purpose crypto tool.

Using JS typed arrays would probably be the way to go, these days (especially once it's not going through XPCOM).

Comment 57

[Richard Newman [:rnewman]]

(In reply to comment #55)
> I think _deriveKeyFromPassphrase() should remain a private API, and you should
> create a new deriveKeydataFromPassphrase() API that basically does what you're
> doing here via the returnKeyData argument.

That sounds like a good idea. (I might slot it into the calling code in Utils, which is the only place it's used, IIRC.)

Thanks for the voice of experience! Drive-by reviews are always welcome.

This was moderately hobbled together, with the goal of trying not to deviate too much from the existing API. That, coupled with my own ignorance, led to the code you see here :)

I'll adjust things tomorrow (or tonight, if I'm sufficiently motivated/whisky-nated). Would you mind lending an eye to the updated patch when I upload it?

Comment 58

[Richard Newman [:rnewman]]

(In reply to comment #56)
> Yes, it's all quite silly, isn't it? :)

Incidentally, we're (I'm!) hoping to address some of these "why the heck do we do all this extra work?" issues after the current batch of work items are done. bsmith's ideas seem like good starting points for investigation.

Make it work, make it right, make it fast, as they say. We're still moving through the list! :)

Comment 59

[Mike Connor [:mconnor]]

Comment on attachment 491911 [details] [diff] [review]
Everything else, v3

In addition to dolske's comments:

* It's not clear to me why keyBundle isn't a property of CryptoWrapper, rather than an argument that needs to be checked and passed around.
* there's still a lot of ambiguous single-letter vars
* I haven't reviewed tests yet.

Specific code comments:
FWIW, I agree 100% with dolske's comments, so I won't repeat those.


>+    if (json_result && ((typeof json_result) == "object")) {

if (json_result && json_result instanceof Object)

>+    if (!collection)
>+      return this._default;
>+    
>+    if (collection in this._collections)
>+      return this._collections[collection] || this._default;
>+    
>+    return this._default;

This seems a little over-complex.  You should be able to just do:

if (collection && this._collections[collection])
  return this._collections[collection];

return this._default;


>+  generateNewKeys: function(generateForCollections) {

This sounds like a boolean arg.  Maybe just "collections" with a proper comment on the function?

>+  setContents: function setContents(payload, modified) {
>+    if ("collections" in payload) {
>+      let out_coll = {};
>+      let colls = payload["collections"];
>+      let c = 0;
>+      for (let k in colls) {
>+        let v = colls[k];
>+        if (v) {
>+          let keyObj = new KeyBundle(null, k);
>+          keyObj.keyPair = v;
>+          if (keyObj) {
>+            c++;
>+            out_coll[k] = keyObj;
>+          }
>+        }
>+      }

c appears to be cruft here.


>+    // The server will round the time, which can lead to us having spurious
>+    // key refreshes. Do the best we can to get an accurate timestamp, but
>+    // rounded to 2 decimal places.
>+    // We could use .toFixed(2), but that's a little more multiplication and
>+    // division...
>+    this._lastModified = modified || (Math.round(Date.now()/10)/100);
>+    return payload;

Rounding could work, or we could get the timestamp directly from the server (X-Weave-Timestamp header on the request response)

>+  updateContents: function updateContents(syncKeyBundle, storage_keys) {
>+    let logger = Log4Moz.repository.getLogger("CollectionKeyManager");
>+    logger.info("Updating collection keys...");
>+    
>+    // storage_keys is a WBO, fetched from storage/crypto/keys.
>+    // Its payload is the default key, and a map of collections to keys.
>+    // We lazily compute the key objects from the strings we're given.
>+    
>+    let payload;
>+    try {
>+      payload = storage_keys.decrypt(syncKeyBundle);
>+    } catch (ex) {
>+      logger.error("Got exception \"" + ex + "\" decrypting storage keys with sync key.");
>+      logger.info("Aborting updateContents. Rethrowing");
>+      throw ex;
>+    }
>+    
>+    let r = this.setContents(payload, storage_keys.modified);
>+    logger.info("Collection keys updated.");
>+    return r;

we need to update _lastModified here, no?

>diff --git a/services/sync/modules/base_records/wbo.js b/services/sync/modules/base_records/wbo.js

> function WBORecord(uri) {
>   if (uri == null)
>     throw "WBOs must have a URI!";
> 
>   this.data = {};
>   this.payload = {};
>+  this.baseUri = null;
>   this.uri = uri;
> }
> WBORecord.prototype = {
>   _logName: "Record.WBO",
> 
>   // NOTE: baseUri must have a trailing slash, or baseUri.resolve() will omit
>   //       the collection name
>   get uri() {
>     return Utils.makeURL(this.baseUri.resolve(encodeURI(this.id)));
>   },
>   set uri(value) {
>     if (typeof(value) != "string")
>       value = value.spec;
>-    let parts = value.split('/');
>-    this.id = parts.pop();
>-    this.baseUri = Utils.makeURI(parts.join('/') + '/');
>+    let parts = value.split("/");
>+    if (parts) {
>+      this.id = parts.pop();
>+      this.baseUri = Utils.makeURI(parts.join("/") + "/");
>+    }
>   },

this.baseUri is no longer needed/relevant now that we don't allow for specifying a crypto url per record.  We should just cull all of this code...

>+  get collection() {
>+    let b = this.baseUri.path.split("/");
>+    // Collection should have a trailing slash. First pop will be null.
>+    return b.pop() || b.pop();

ew. b[b.length - 2] feels less ew.

>+  fetch: function fetch() {
>+    let r = new Resource(this.uri).get();
>+    if (r.success) {
>+      this.deserialize(r);   // Warning! Muffles exceptions!
>+    }
>+    this.response = r;
>+    return this;
>+  },
>+  
>+  upload: function upload() {
>+    return new Resource(this.uri).put(this);
>+  },
>+  

So... I'm not convinced we need this.uri as a property, but we can deal with that as a followup

>+      // ,"collection: " + (this.collection || "undefined")

Please remove commented-out code before review.

>diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js
>+      
>+      // TODO: do we need to generate new meta and keys?

I would assume that if we're wiping the server, we're generating new meta and keys before we upload.

>diff --git a/services/sync/modules/service.js b/services/sync/modules/service.js
 

>@@ -1322,23 +1338,26 @@ WeaveSvc.prototype = {
>-
>+    

random whitespace addition

>   /**
>    * Sync up engines with the server.
>    */
>   sync: function sync()
>-    this._catch(this._lock(this._notify("sync", "", function() {
>+    this._catch(this._lock("service.js: sync", 
>+          this._notify("sync", "", function() {

align the args, please

>+    this._log.info("In sync().");

We already log start of sync, no?

>diff --git a/services/sync/modules/status.js b/services/sync/modules/status.js
>index ee36c5b..e1efa23 100644
>--- a/services/sync/modules/status.js
>+++ b/services/sync/modules/status.js
>@@ -35,16 +35,18 @@
> const EXPORTED_SYMBOLS = ["Status"];
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cr = Components.results;
> const Cu = Components.utils;
> 
> Cu.import("resource://services-sync/constants.js");
>+Cu.import("resource://services-sync/base_records/crypto.js");
>+Cu.import("resource://services-sync/log4moz.js");

this is something we always import, even when sync isn't enabled, so importing logging is not desirable in an unconditional way.  If we really must log, import in that scope.

>diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js

>+  lock: function lock(label, func) {
>       finally {
>         thisArg.unlock();
>+        // thisArg._log.info("Releasing lock. Label: \"" + label + "\".");

no commented out lines please.


>@@ -592,26 +626,46 @@ let Utils = {
>     
>     // No UTF-8 encoding for you, sunshine.
>     let bytes = [b.charCodeAt() for each (b in message)];
>     h.update(bytes, bytes.length);
>     return h.finish(false);
>   },
>   
>   /**
>-   * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>+   * Generate a sha256 HMAC for a string message and a given nsIKeyObject.
>+   * Optionally provide an existing hasher, which will be 
>+   * initialized and reused.

pick a margin width. :)

>+  /**
>+   * Turn RFC 4648 base32 into our own user-friendly version.
>+   *   ABCDEFGHIJKLMNOPQRSTUVWXYZ234567 
>+   * becomes
>+   *   abcdefghijkmnpqrstuvwxyz23456789
>+   */
>+  translateBase32: function translateBase32(input, map) {


this hurts me, but it's probably about right...

  
>-  /**
>-   * Remove hyphens as inserted by hyphenatePassphrase().
>-   */
>-  normalizePassphrase: function(pp) {
>-    if (pp.length == 23 && pp[5] == '-' && pp[11] == '-' && pp[17] == '-')
>-      return pp.slice(0, 5) + pp.slice(6, 11)
>-           + pp.slice(12, 17) + pp.slice(18, 23);
>+  dehyphenatePassphrase: function(pp) {
>+    if (pp.length == 31 && [1, 7, 13, 19, 25].every(function(i) pp[i] == '-'))
>+      return pp.slice(0, 1) + pp.slice(2, 7)
>+             + pp.slice(8, 13) + pp.slice(14, 19)
>+             + pp.slice(20, 25) + pp.slice(26, 31);
>     return pp;
>   },
>+  
>+  normalizePassphrase: function(pp) {
>+    return Utils.dehyphenatePassphrase(pp);
>+  },
>

why rename the function and leave a wrapper?  Feels like unnecessary churn.

Comment 60

[Richard Newman [:rnewman]]

(In reply to comment #59)

> * It's not clear to me why keyBundle isn't a property of CryptoWrapper, rather
> than an argument that needs to be checked and passed around.

That might be a better way to do it. Will explore.


> if (json_result && json_result instanceof Object)

Thanks. Forgetting my JS 101.


> This seems a little over-complex.  You should be able to just do:
> 
> if (collection && this._collections[collection])
>   return this._collections[collection];

Yup, I did this deliberately to show the decision logic more clearly. I can simplify if you don't see the value in that; no big deal.


> >+  generateNewKeys: function(generateForCollections) {
> 
> This sounds like a boolean arg.  Maybe just "collections" with a proper comment on the function?

Agreed.


> c appears to be cruft here.

Thanks.


> Rounding could work, or we could get the timestamp directly from the server
> (X-Weave-Timestamp header on the request response)

This modification time comes from generateNewKeys, as well as here, where it's set from a server value. In the former we'd still need to round :( 

Rounding seems to work, despite being ugly, so I'm inclined to stick with it for now.


> we need to update _lastModified here, no?

See setContents:

    this._lastModified = modified || (Math.round(Date.now()/10)/100);


> this.baseUri is no longer needed/relevant now that we don't allow for
> specifying a crypto url per record.  We should just cull all of this code...

Hurrah!

> 
> >+  get collection() {
> >+    let b = this.baseUri.path.split("/");
> >+    // Collection should have a trailing slash. First pop will be null.
> >+    return b.pop() || b.pop();
> 
> ew. b[b.length - 2] feels less ew.

Heh.


> So... I'm not convinced we need this.uri as a property, but we can deal with
> that as a followup

OK.


> >+      // ,"collection: " + (this.collection || "undefined")
> 
> Please remove commented-out code before review.

Sure.


> >-    this._catch(this._lock(this._notify("sync", "", function() {
> >+    this._catch(this._lock("service.js: sync", 
> >+          this._notify("sync", "", function() {
> 
> align the args, please

Can you clarify how we like this to be aligned? Like this?


+    this._catch(this._lock("service.js: sync", 
+                           this._notify("sync", "", function() {
+                                                      // Code goes here

or otherwise? (Presumably otherwise...)


> >+    this._log.info("In sync().");
> 
> We already log start of sync, no?

Apparently not. sync() does not get called every time Sync starts doing things, which I found a little surprising. I found this to be new information, so I left in the log statement.


> this is something we always import, even when sync isn't enabled, so importing
> logging is not desirable in an unconditional way.  If we really must log,
> import in that scope.

Sorry, there might well be leftover log imports where I've stripped out debug logging over time -- this is one of them. Will clean this up.


> pick a margin width. :)

Heh.
 

> >+  translateBase32: function translateBase32(input, map) {
> 
> 
> this hurts me, but it's probably about right...

Yeah, me too.

Currently only used in tests, though, so (depending on your attitude to dormant code) could be removed.


> why rename the function and leave a wrapper?  Feels like unnecessary churn.

I was trying to leave a layer between what the UI calls and the specific implementation details, given our goal of decoupling key representation and the UI. Seems wrong to have the UI know that the key needs to be "dehyphenated".

Next week maybe we'll want to change the case, or add spacing, or…

There is an inconsistency here, though, so I'm glad you brought it up -- UI code calls Utils.hyphenatePassphrase. I'm inclined to put in "presentPassphrase" (for want of a better name) function to mirror normalizePassphrase.

Thoughts?

Thanks for the thorough review; will hit these tomorrow morning.

Comment 61

[Philipp von Weitershausen [:philikon]]

Comment on attachment 491911 [details] [diff] [review]
Everything else, v3

This patch contains a bug: if I log into an existing account that has storageVersion < 4, all my engines are disabled.

This is because a new meta record is written upon login verification. This will not contain the 'engines' information. This is usually ok because _freshStart sets meta.isNew = true. That stops _updateEnabledEngines from thinking that all engines were disabled on other clients.

However, now that we upload a new meta record on login(), the 'meta' collection will be marked modified in sync() so it ditches the local record 

>     // If the modified time of the meta record ever changes, clear the cache.
>-    if (info.obj.meta != this.metaModified) {
>-      this._log.debug("Clearing cached meta record.");
>+    // I've seen this come back undefined, so let's be careful.

Seen what come back undefined? info.obj? That means the response wasn't valid JSON. This is more indicative of a lurking bug somewhere, so let's have it blow up rather than tip-toe around this. It might just be a problem with your test fixtures (and then it should be fixed *there* rather than here.)

>+    if (info.obj && info.obj.meta && (info.obj.meta != this.metaModified)) {
>+      this._log.debug("Clearing cached meta record. metaModified is " +
>+          JSON.stringify(this.metaModified) + ", setting to " +
>+          JSON.stringify(info.obj.meta));
>       Records.del(this.metaURL);
>       this.metaModified = info.obj.meta;
>     }
> 
>-    if (!(this._remoteSetup()))
>+    if (!(this._remoteSetup(info)))

Here's the problem. _remoteSetup would previously only be called *after* the local meta record had been ditched. Now login() calls _remoteSetup() which is well before this. sync() will see the 'meta' collection has been updated, ditch the local record, and meta.isNew = true is lost, hence _updateEnabledEngines will disable all engines.

I think the solution is to make the if (info.obj.meta != this.metaModified) check check for meta.isNew. If meta.isNew is true, don't ditch the local record.

Comment 62

[Philipp von Weitershausen [:philikon]]

Comment on attachment 491911 [details] [diff] [review]
Everything else, v3

Some drive-by comments:

>diff --git a/services/sync/modules/status.js b/services/sync/modules/status.js
>index ee36c5b..e1efa23 100644
>--- a/services/sync/modules/status.js
>+++ b/services/sync/modules/status.js
>@@ -35,16 +35,18 @@
> const EXPORTED_SYMBOLS = ["Status"];
> 
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cr = Components.results;
> const Cu = Components.utils;
> 
> Cu.import("resource://services-sync/constants.js");
>+Cu.import("resource://services-sync/base_records/crypto.js");
>+Cu.import("resource://services-sync/log4moz.js");

mconnor already mentioned this, but I'd like to emphasize this: Please don't add any imports to status.js, it would penalize startup performance for users that don't use Sync. Every Firefox user will import main.js and status.js so they need to do as little as possible (logging is definitely not something crucial.)

At some point (work week?) we need to sit down and document these implicit constraints in the codebase.

>+// Fake crypto storage handler.
>+
>+// For request stream handling.
>+Components.utils.import("resource://gre/modules/NetUtil.jsm");
>+
>+function make_crypto_handler(collectionsObj) {

Have you seen ServerWBO? It should make this unnecessary I think.

>+  let crypt;
>+  return function (request, response) {
>+    if (request.method == "GET") {
>+      if (crypt) {
>+        response.setStatusLine(request.httpVersion, 200, "OK");
>+        response.bodyOutputStream.write(crypt, crypt.length);
>+      }
>+      else {
>+        response.setStatusLine(request.httpVersion, 404, "Not Found");
>+        response.bodyOutputStream.write("", 0);
>+      }
>+      return;
>+    }
>+    if (request.method == "PUT") {
>+      let stream = request.bodyInputStream;
>+      crypt = NetUtil.readInputStreamToString(stream, stream.available());

NetUtil doesn't exist in Firefox 3.5 (I hope you're running tests with Fx 3.5 as well!). Use readBytesFromInputStream from head_http_server.js (as a bonus, it makes the second argument optional! \o/)

Comment 63

[Richard Newman [:rnewman]]

(In reply to comment #61)

> This is because a new meta record is written upon login verification.

Fixed, thanks.

> Seen what come back undefined? info.obj? That means the response wasn't valid
> JSON.

Stripped it out. Things work, which suggests it was a WIP bug. Will keep eyes open.

Comment 64

[Philipp von Weitershausen [:philikon]]

(In reply to comment #60)
> > why rename the function and leave a wrapper?  Feels like unnecessary churn.
> 
> I was trying to leave a layer between what the UI calls and the specific
> implementation details, given our goal of decoupling key representation and the
> UI. Seems wrong to have the UI know that the key needs to be "dehyphenated".
> 
> Next week maybe we'll want to change the case, or add spacing, or…
> 
> There is an inconsistency here, though, so I'm glad you brought it up -- UI
> code calls Utils.hyphenatePassphrase. I'm inclined to put in
> "presentPassphrase" (for want of a better name) function to mirror
> normalizePassphrase.

Just talked this over in IRC. Essentially the hyphenatePassphrase and normalizePassphrase are UI facing methods that are possibly poorly named but for the sake of backwards compat, let's keep them that way. We can add a comment explaining that they might also do other transformation to/from UI code.

We also need a hyphenatePartialPassphrase for UI code that takes any length input and hyphenates it. This will be used to add hyphenation as the user types.

Comment 65

[Richard Newman [:rnewman]]

Getting close to dinner time, so here are the changes I've made today:

    * Partial hyphenation (hyphenatePartialPassphrase).
    * Minimize imports in status.js.
    * Add comments about supported methods for UI code.
    * Trace verbose log items; use a single logger rather than recreating all the time.
    * Avoid trampling existing meta on login. (Bug fix.)
    * WBOs now track their id and collection, not a URI. This touched a lot of code.
    * Code tidying per mconnor.
    * deriveKeyFromPassphrase now returns a JS string of key data, and it's down to a single code path. No more leaks?
    * Better implementation of do_check_begins.

Delta patches upcoming (on top of the big ones for simplicity of review).

Comment 66

[Richard Newman [:rnewman]]

Attachment: Delta patch 1

See Comment 65 for contents.

Individual commits are separated out in my git repo, if you want to view them that way.

Comment 67

[Philipp von Weitershausen [:philikon]]

Comment on attachment 492545 [details] [diff] [review]
Delta patch 1

Awesome work, as usual!

> function CollectionKeyManager() {
>   this._lastModified = 0;
>   this._collections = {};
>   this._default = null;
>+  
>+  this._logger = Log4Moz.repository.getLogger("CollectionKeys");

Nit: The global convention in Sync is to store loggers as this._log. Would be good to stick to that, especially since there are some helpers that expect this to be called this._log (We might not use them on this particular object, but why cause potential confusion in the future...)

> // TODO: persist this locally as an Identity.

We should either get rid of this comment OR add the bug number here. People who revisit this code need to either know what's up or not misled.

>-    let logger = Log4Moz.repository.getLogger("CollectionKeys");
>-    logger.info("keyForCollection: " + collection + ". Default is " + (this._default ? "not null." : "null."));
>-    
>-    if (!collection)
>-      return this._default;
>+    this._logger.trace("keyForCollection: " + collection + ". Default is " + (this._default ? "not null." : "null."));

Thanks for demoting that log msg :)

>-  asWBO: function(url, id) {
>-    let wbo = new CryptoWrapper(url);
>+  asWBO: function(collection, id) {
>+    let wbo = new CryptoWrapper(collection || "crypto", id || "keys");

I asked about the need for this method (as well as setContents, updateContents, etc.) as opposed to making CollectionKeyManager/CollectionKeys a WBORecord itself in comment 42. The current way seems somewhat inelegant to me, but seeing the usage patterns in other parts of the code as well as the time constraints, I think this is fine. I would only suggest to make naming consistent: asWBO vs setContents/updateContents, if that makes sense at all.

>-    let logger = Log4Moz.repository.getLogger("CollectionKeyManager");
>-    logger.info("Testing for updateNeeded. Last modified: " + this._lastModified);
>+    this._logger.info("Testing for updateNeeded. Last modified: " + this._lastModified);

Nit: this._log (see above)

>   deserialize: function deserialize(json) {
>     this.data = json.constructor.toString() == String ? JSON.parse(json) : json;
> 
>     try {
>       // The payload is likely to be JSON, but if not, keep it as a string
>       this.payload = JSON.parse(this.payload);
>     } catch(ex) {}
>+      
>+    // Extract other details, too.
>+    if (this.data["id"]) {
>+      this.id = this.data["id"];
>+    }
>+    if (this.data["collection"])
>+      this.collection = this.data["collection"];
>   },

wbo.id is automatically retrieved and stored in the 'data' attribute, by the power of the following declaration:

> Utils.deferGetSet(WBORecord, "data", ["id", "modified", "sortindex", "payload"]);

So first stanza there is unnecessary.

As for the collection, we can't simply add the "collection" field to this because it's not part of the storage format of WBOs. The server won't know how to deal with it. But it's also not necessary because it's just for local bookkeeping. "collection" shouldn't be part of wbo.data, it should just be a property on the WBO that the engine sets. Please remove the second stanza therefore as well.

Also, this being JavaScript, you can write this.data.id and this.data.collection

>   toString: function WBORec_toString() "{ " + [
>       "id: " + this.id,
>       "index: " + this.sortindex,
>       "modified: " + this.modified,
>-      "payload: " + JSON.stringify(this.payload)
>-      // ,"collection: " + (this.collection || "undefined")
>+      "payload: " + JSON.stringify(this.payload),
>+      "collection: " + this.collection
>     ].join("\n  ") + " }",
> };

Same here: Can't promote "collection" to an official property of the wbo. Please remove.

>+  hyphenatePartialPassphrase: function hyphenatePartialPassphrase(passphrase) {
>+    if (passphrase.length < 2)
>       return passphrase;
>     
>     let y = passphrase.substr(0,1);
>-    let z = passphrase.substr(1).replace(/(.{5})/g, "-$1");
>+    let z = passphrase.substr(1).replace(/(.{1,5})/g, "-$1");
>     return y + z;
>   },

This is a great start, but not as helpful as it could be. Imagine the user starts entering the Sync Key into the textfield. After every keystroke we replace the contents of the textfield with the hyphenated version. That means the thing we keep feeding back into the hyphenation routine will already be hyphenated. It might even be hyphenated incorrectly because the user may realize they forgot to type a latter at the beginning, so they'll add it.

So to make this perfect,

* it should just strip every foreign character including dashes, so basically anything that's not in our base32 set (that way we also get foreign character exclusion for free)

* *then* we can do the partial hyphenation like above

* except that we should always include trailing dashes where appropriate (e.g. 'a' should become 'a-', 'a-asdfsd' should become 'a-asdfsd-', etc.)

>-  dehyphenatePassphrase: function(pp) {
>+  dehyphenatePassphrase: function dehyphenatePassphrase(pp) {
>+    // Short var name... have you seen the lines below?!
>     if (pp.length == 31 && [1, 7, 13, 19, 25].every(function(i) pp[i] == '-'))
>       return pp.slice(0, 1) + pp.slice(2, 7)
>              + pp.slice(8, 13) + pp.slice(14, 19)
>              + pp.slice(20, 25) + pp.slice(26, 31);
>     return pp;
>   },
>   
>-  normalizePassphrase: function(pp) {
>-    return Utils.dehyphenatePassphrase(pp);
>+  normalizePassphrase: function normalizePassphrase(passphrase) {
>+    return Utils.dehyphenatePassphrase(passphrase);
>   },

Given comment 64, we can just rename dehyphenatePassphrase to normalizePassphrase no?

>+  deriveKeyFromPassphrase : function (passphrase, salt, keyLength) {

Nit: style

  deriveKeyFromPassphrase: function deriveKeyFromPassphrase(...) {

Comment 68

[Philipp von Weitershausen [:philikon]]

(In reply to comment #67)
> As for the collection, we can't simply add the "collection" field to this
> because it's not part of the storage format of WBOs.

Sorry, I meant to say that we can't simply add the "collection" field to *wbo.data*. We can of course add it as a property to the wbo directly.

Comment 69

[Philipp von Weitershausen [:philikon]]

One more thing that we definitely should address is to base32-decode the Sync Key before we pass it to the HKDF in KeyBundle.

That reminds me: looking at the KeyBundle code it almost looks like it is two separate objects intertwingled. One is for turning the Sync Key into something that decrypt/encrypt can deal with (basically by applying the HKDF) and one is for dealing with bulk keys that have random generated HMAC and encryption keys.

So I think we should split those up into
* SyncKeyBundle (does the base32-decode and HKDF magic)
* BulkKeyBundle (generates two random keys)

Comment 70

[Richard Newman [:rnewman]]

Attachment: Delta patch 2

Partially supersedes "Delta patch 1". Responses to some more review items.

More to come later today.

Comment 71

[Richard Newman [:rnewman]]

(In reply to comment #70)
> Created attachment 492777 [details] [diff] [review]
> Delta patch 2

Contents:

    * Add label for deriveKeyFromPassphrase.
    * Better partial hyphenation; eliminate dehyphenatePassphrase.
    * Pointer to Bug 610913.
    * log, not logger.

Comment 72

[Richard Newman [:rnewman]]

Brief update:

We decided to leave asWBO named as-is for now, because it should be going away soonish, and there's no name that's massively better.

Working on WBO stuff next.

Comment 73

[Philipp von Weitershausen [:philikon]]

A thing I ran into earlier: The storageVersion detection code so far in Sync does not anticipate a change in the crypto structure. Upon upgrade we delete keys/pubkey and keys/privkey which makes older Sync versions think that the passphrase is wrong instead of surfacing the "Your version is too old" error message on Sync. It doesn't even get that far because it fails on login.

This is very unfortunate. I believe we should do two things about this:

1. Do not delete keys/pubkey and keys/privkey upon migration. Just leave them around, they won't do harm and instead trick old clients into logging in properly first and then detecting their version is too old.

2. Make sure that we won't run into this problem in the future. This could be the case already since we now call remoteSetup() from verifyLogin() which definitely does this check somewhere, but it would good to have a test for this.

Comment 74

[Richard Newman [:rnewman]]

Attachment: Delta patch 3

This delta patch includes work since:

commit fc894ad089ac67d9370036d7be02a3fb80b59359
Author: Richard Newman <[e-mail]>
Date:   Tue Nov 23 13:24:26 2010 -0800

    Add label for deriveKeyFromPassphrase.


It includes the following work (newest first):

c4afba8 Update test for key derivation. Waiting for confirmation from st3fan.
4964216 Test fixes.
d82eada Split KeyBundle into SyncKeyBundle and BulkKeyBundle.
faeb786 Add tests for encodeKeyBase32.
c0341d6 Do not attempt to be smart when encountering wrong-cased sync keys. Trust the UI.
2d5c706 Use a valid sync key in test_service_persistLogin.
8d6e30e Base64 fixes in test harness and around binary WeaveCrypto. safeAtoB.
b8de595 Fix some tests to use KeyBundle rather than trying to use passphrase.
d85f011 Eliminate make_crypto_handler.
8e79d61 Default key length in JS PBKDF2 implementation. Fix failure in Fx3.6.
9177bf2 Fix old typo.
ef568a5 Align args per mconnor.
7dafa8a Fix bug in generatePassphrase. encodeKeyBase32 takes a string, not a byte array.
702f7b7 Factor out Utils.byteArrayToString.
f0d2bb0 Whitespace.
274fa40 More efficient replaces in base32To/FromFriendly.
970e8da Clean up generatePassphrase.
0dd5982 Base32-decode the key before handing it into key derivation.

7232cb7 Bug 614489: add base32 decoder and tests. r=philiKON

07f9d29 Simplification of createRecord signature, per philiKON.
e1378c4 Actually run test_sync_rollback. Fix for new crypto.
8683c41 Fix merge of test_syncengine_sync.js.
d79c8c0 Fix engine handling of IDs.
7a6d80e Don't include collection in the body of a WBO, per philiKON.
19f0b95 Remove redundant comments per mconnor.
48c7b24 Better tracking of failures in remoteSetup.
0e72b1c Correct hyphenation, passphrase truncation.
63e625d Don't be so alarmist with logging output.
d42c4ba Test and fix for failure in remoteSetup when credentials have changed (missing return).
225399e Alter base32 encoding scheme.


I believe this addresses all of the review comments so far (at least, the ones that haven't deliberately been pushed back).

A full everything-since-master patch will follow shortly for additional review.

Comment 75

[Richard Newman [:rnewman]]

Attachment: Complete patch for final review, v1

(Not marking as obsoleting others in order to better track r+s.)

This is a complete patch against this HEAD:

 Bug 610749: (FIXED) add pure-JS PBKDF2 implementation. r=philikon

Sorry that I made this patch very long; I did not have the leisure to make it shorter.

This includes the contents of previous delta patches; a thorough review of "Delta patch 3" (attachment 493180 [details] [diff] [review]) and a cursory review of this should suffice.

Thanks in advance for your entire morning, mconnor!

Comment 76

[Mike Connor [:mconnor]]

Comment on attachment 493180 [details] [diff] [review]
Delta patch 3

>diff --git a/services/sync/modules/base_records/crypto.js b/services/sync/modules/base_records/crypto.js


>@@ -298,138 +298,144 @@ CollectionKeyManager.prototype = {
>  * it'll default to PWDMGR_KEYBUNDLE_REALM.
>  * 
>  * KeyBundle has to be able to adapt to two different situations:
>  * 
>  * * A key string is provided, and it must be hashed to yield
>  *   two different keys (one HMAC, one AES).
>  *   
>  * * Two independent keys are provided, or generated on request.
>+ * 
>+ * Consequently, it has two subclasses: SyncKeyBundle (the former) and
>+ * BulkKeyBundle (the latter).

Maybe this is nitpicky, but the comment isn't nearly as crisp as I'd like it.  Maybe something more like this:

KeyBundle is a base class for two similar classes:

* SyncKeyBundle 

  A key string is provided, and it must be hashed to yield
  two different keys (one HMAC, one AES)

* BulkKeyBundle

  Two independent keys are provided, or generated on request.


>diff --git a/services/sync/modules/constants.js b/services/sync/modules/constants.js

>+SYNC_KEY_HYPHENATED_LENGTH:            26 + 5,

bah. I hate consts that require math when we eval the file.  It seems like silly use of cycles.  File a followup on making the consts just be static values?

> // Sync intervals for various clients configurations
> SINGLE_USER_SYNC:                      24 * 60 * 60 * 1000, // 1 day
> MULTI_DESKTOP_SYNC:                    60 * 60 * 1000, // 1 hour
> MULTI_MOBILE_SYNC:                     5 * 60 * 1000, // 5 minutes
> PARTIAL_DATA_SYNC:                     60 * 1000, // 1 minute

>diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js

>   _createRecord: function SyncEngine__createRecord(id) {
>-    let record = this._store.createRecord(id, this.engineURL + "/" + id);
>+    let record = this._store.createRecord(id, this.name, id);

should be (id, this.name)

I didn't dig into all of the tests, but the ones I picked looked sane enough.

Comment 77

[Richard Newman [:rnewman]]

(In reply to comment #76)

Thanks for the speedy review, mconnor.

If I happen to have spare cycles, this'll happen tonight or tomorrow:

* Reflect final review comments in code
* Rebase to current hg head, deal with merge conflicts
* Apply patch to hg, test
* Send new XPI to Stefan for final testing against Home
* Figure out tryserver and kick off a build
* Crack champagne and mark this as RESOLVED.

Comment 78

[Richard Newman [:rnewman]]

Tryserver fails on these:

* n900-gtk "unit chrome":

---
  There was a failure on n900-gtk tryserver unit chrome for revision ce73ae2344bc using n900-041
---

No details at all. Other n900-gtk tests pass.


* Warnings on tryserver_fedora-debug_test-xpcshell:

---
xpcshell
1031/42
---

All of these seem to be failures in loading libraries...

---
### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpFXcF-v/runxpcshelltests_leaks.log
pldhash: for the table at address 0x8b5bc80, the given entrySize of 48 probably favors chaining over double hashing.
nsNativeModuleLoader::LoadModule("/home/cltbld/talos-slave/tryserver_fedora-debug_test-xpcshell/build/firefox/components/libxpcomsample.so") - load FAILED, rv: 80004005, error:
	/home/cltbld/talos-slave/tryserver_fedora-debug_test-xpcshell/build/firefox/components/libxpcomsample.so: cannot open shared object file: No such file or directory
pldhash: for the table at address 0x8becf48, the given entrySize of 52 probably favors chaining over double hashing.
Setting the identity for passphrase
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../../js/src/xpconnect/loader/mozJSComponentLoader.cpp, line 1168
uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIXPCComponents_Utils.import]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: /home/cltbld/talos-slave/tryserver_fedora-debug_test-xpcshell/build/xpcshell/tests/services/sync/tests/unit/test_records_crypto_generateEntry.js :: <TOP_LEVEL> :: line 3"  data: no]
WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../xpcom/base/nsExceptionService.cpp, line 197
WARNING: Failed to get XPConnect?!: 'rts', file ../../../xpcom/base/nsCycleCollector.cpp, line 3415
WARNING: Failed to get XPConnect?!: 'rts', file ../../../xpcom/base/nsCycleCollector.cpp, line 3415
WARNING: Failed to get XPConnect?!: 'rts', file ../../../xpcom/base/nsCycleCollector.cpp, line 3415
WARNING: Failed to get XPConnect?!: 'rts', file ../../../xpcom/base/nsCycleCollector.cpp, line 3415
WARNING: OOPDeinit() without successful OOPInit(): file ../../../toolkit/crashreporter/nsExceptionHandler.cpp, line 1742
---


Logs and build output here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4540655ba02e/


Can someone please advise me:

* Should I worry about these warnings (in particular the library ones, which at first glance don't seem to be related to my work?

* Should the n900 failure be down to me, how exactly might I go about debugging that?

Mails are still trickling in, so we'll see if there are any more :D

Happy Thanksgiving, all!

Comment 79

[Philipp von Weitershausen [:philikon]]

Your try build is broken because the patch didn't apply cleanly (see all the .rej and .orig files in https://hg.mozilla.org/try/rev/ce73ae2344bc). That's because fx-sync has a few patches applied that haven't been merged to mozilla-central yet. You'll have to import those into your patch queue first before applying your patch.

Comment 80

[Richard Newman [:rnewman]]

Pushed to fx-sync. Just waiting for Bug 615021 now!

Comment 81

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/services/fx-sync/rev/cc0c232b63ed

Comment 82

[Tracy Walker [:tracy]]

verified with 1.6b1

Comment 83

[Richard Newman [:rnewman]]

Comment on attachment 492545 [details] [diff] [review]
Delta patch 1

Clearing r? for delta patch.

Comment 84

[Richard Newman [:rnewman]]

Comment on attachment 492777 [details] [diff] [review]
Delta patch 2

Clearing r? for delta patch.