Closed Bug 943521 Opened 11 years ago Closed 10 years ago

Update desktop FXAccountsClient to use 'onepw' protocol

Categories

(Firefox :: Firefox Accounts, defect)

Product:
Firefox
Component:
Firefox Accounts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 30
Tracking Flags:
Tracking Status
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: spenrose, Assigned: jedp)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 10 obsolete files)

Use onepw protocol in fxa client
55.29 KB, patch
Details | Diff | Splinter Review
[aurora rebase] use onepw protocol for auth
57.05 KB, patch
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
[beta] Make signIn() method API conform v2
1.03 KB, patch
jedp
: review+
ckarlof
: feedback+
lsblakk
: approval-mozilla-aurora-
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
The FxA Server API is going to change, in large part to better support us. Conform to the result.
Whiteboard: [qa+]
Blocks: 949053
Looks like this is going to be https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol.
+ more Android and desktop client devs
Depends on: 945449
Depends on: 938461
Depends on: 955953
Is this bug specifically for Desktop or Android or both?
I assumed this was for desktop.  For Android, see Bug 955808.  Updating titles.
Summary: Update FXAccountsClient for new server API per https://github.com/mozilla/fxa-auth-server/issues/344 → Update desktop FXAccountsClient for new server API per https://github.com/mozilla/fxa-auth-server/issues/344
This is primarily to support login to FxOS devices.
The API, "onepw", is here:

  https://github.com/mozilla/fxa-auth-server/tree/onepw

and will be rolled out to:

  https://api-accounts.dev.lcip.org/v1

very soon, which will break all server connectivity. I will start work on updating FxAccountsClient.jsm now.
Assignee: nobody → spenrose
Prefing "identity.fxaccounts.auth.uri" to "https://api-accounts-legacy.dev.lcip.org" should get around this breakage for now.
The correct custom preference is:

  user_pref("identity.fxaccounts.auth.uri", "https://api-accounts-legacy.dev.lcip.org/v1");

apologies for leaving off the "v1".
Sam, if you like, I can take this.  I'm working on some FxAccountsClient bugs at the moment, so I'm already in this code.
Flags: needinfo?(spenrose)
We'll look at this one together tomorrow.
Flags: needinfo?(spenrose)
Assignee: spenrose → jparsons
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
This implements the onepw protocol in the FxAccountsClient.  It follows the implementation of fxa-js-client closely.

Patch includes:
- credentials tests, incl warner's test vectors for the onepw protocol
- updated tests for FxAccountsClient
- some tweaks to CryptoUtils

Many thanks to :ckarlof for holding my hand for two hours today and helping me stumble past my clumsiness.

Mark, do you mind taking a look?  Thanks!
j
Attachment #8370520 - Flags: review?(mhammond)
Summary: Update desktop FXAccountsClient for new server API per https://github.com/mozilla/fxa-auth-server/issues/344 → Update desktop FXAccountsClient to use 'onepw' protocol
Comment on attachment 8370520 [details] [diff] [review]
Use onepw protocol in fxa client

Review of attachment 8370520 [details] [diff] [review]:
-----------------------------------------------------------------

Looking reasonable to me, but I think we'd also want review from someone who understand some of the crypto bits and who can also verify the tests are covering the important stuff.

::: services/common/utils.js
@@ +196,5 @@
>      return [String.fromCharCode(byte) for each (byte in bytes)].join("");
>    },
>  
> +  stringAsHex: function stringAsHex(str) {
> +    return CommonUtils.bytesAsHex(CommonUtils.encodeUTF8(str));

I think |this| would be clearer here (and below)

@@ +203,2 @@
>    bytesAsHex: function bytesAsHex(bytes) {
> +    return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2) 

nit: trailing space

::: services/fxaccounts/FxAccountsClient.jsm
@@ +71,5 @@
>     *          sessionToken: a session token
>     *        }
>     */
> +  signUp: function(email, password) {
> +    return credentials.setup(email, password)

This file's style is already all over the place WRT promises, but I don't like this indentation - it makes it harder for my brain to read.  I tend to prefer the .this(() => {\n type style, meaning each nested promise just gets a single indent level.

@@ +90,5 @@
> +            }
> +          );
> +        },
> +        (error) => {
> +          throw error;

I don't think this error handler adds any value - IIUC, the behaviour would be the same without this (ie, it might make sense if you logged the error before throwing etc).  Please correct me if I'm wrong about this :)

@@ +113,5 @@
>     */
>    signIn: function signIn(email, password) {
> +    let hexEmail = CommonUtils.stringAsHex(email);
> +    return credentials.setup(email, password)
> +      .then(

ditto here re indentation and final throw.

@@ +179,5 @@
>    accountKeys: function (keyFetchTokenHex) {
>      let creds = this._deriveHawkCredentials(keyFetchTokenHex, "keyFetchToken");
>      let keyRequestKey = creds.extra.slice(0, 32);
>      let morecreds = CryptoUtils.hkdf(keyRequestKey, undefined,
> +                                     kw("account/keys"), 3 * 32);

see comment in credentials.js about the naming of |kw| etc.

::: services/fxaccounts/credentials.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I think this should be named Credentials.jsm (FxAccountsCommon.js sucks 'cos it doesn't use .jsm)

@@ +36,5 @@
> +  HMAC_ALGORITHM: HMAC_ALGORITHM,
> +  HMAC_LENGTH: HMAC_LENGTH,
> +};
> +
> +this.kw = function(context) {

can we give these better names?  The names were slightly more palatable when the function was local to the module that used it, but now that fxAccountsClient calls a function called "kw" it's not at all obvious where it comes from.  Another alternative that might be better still would be to have an object exported from here called, say, "CredentialsUtils" (or even directly on the credentials object below) - then seeing Credentials.kw(...) might not make much sense, but at least you know where it came from!

@@ +62,5 @@
> +this.kwe = function kwe(name, email) {
> +  return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
> +};
> +
> +this.credentials = {

should be Credentials

@@ +95,5 @@
> +        CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, kw("unwrapBkey"), hkdfLength);
> +
> +      deferred.resolve(result);
> +    } catch(err) {
> +      deferred.reject(err);

What exceptions are expected here?  It looks like it might make sense to log this before throwing it, probably via Cu.reportError("fail to blah: " + err);

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +8,3 @@
>  
>  const FAKE_SESSION_TOKEN = "a0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebf";
> +const CU = CommonUtils;

not that keen on this name - too close to Cu.  Is it even used?

@@ +122,4 @@
>          let body = CommonUtils.readBytesFromInputStream(request.bodyInputStream);
>          let jsonBody = JSON.parse(body);
> +        // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#wiki-test-vectors
> +        do_check_eq(jsonBody.email, "andré@example.org");

I don't understand these tests (but that's almost certainly my fault).  I was expecting to see the addition of tests rather then just modifying existing ones.

::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://services-common/utils.js");
> +Cu.import("resource://services-crypto/utils.js");
> +
> +const CU = CommonUtils;

ditto here re "CU" vs "Cu".  If it is only used below, something like:

let {hexToBytes: h2b, hexAsString: h2s, ...} = CommonUtils;

should work (untested, but I *think* the order of the object-literal-like-thing is correct :)

@@ +172,5 @@
> +      "quickStretchedPW is wrong");
> +
> +  do_check_eq(expected.authPW, b2h(results.authPW),
> +      "authPW is wrong");
> +  

another space nit :)
Attachment #8370520 - Flags: review?(mhammond) → feedback+
(In reply to Mark Hammond [:markh] from comment #15)
> Comment on attachment 8370520 [details] [diff] [review]
> Use onepw protocol in fxa client
> 
> Review of attachment 8370520 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking reasonable to me, but I think we'd also want review from someone who
> understand some of the crypto bits and who can also verify the tests are
> covering the important stuff.

Mark, thank you as always for your thorough reviews.  I shall ask :warner for review of the crypto components, having modified the patch according to your feedback.

(:warner returns from PTO next Monday, I think)

> ::: services/common/utils.js
> @@ +196,5 @@
> >      return [String.fromCharCode(byte) for each (byte in bytes)].join("");
> >    },
> >  
> > +  stringAsHex: function stringAsHex(str) {
> > +    return CommonUtils.bytesAsHex(CommonUtils.encodeUTF8(str));
> 
> I think |this| would be clearer here (and below)

Hrm.  This (no pun intended) will require restructuring this file a bit.  I agree that it's kind of gross as it is, but I'm hesitant to make the change.

> @@ +203,2 @@
> >    bytesAsHex: function bytesAsHex(bytes) {
> > +    return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2) 
> 
> nit: trailing space

Sorry.  Thanks.

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +71,5 @@
> >     *          sessionToken: a session token
> >     *        }
> >     */
> > +  signUp: function(email, password) {
> > +    return credentials.setup(email, password)
> 
> This file's style is already all over the place WRT promises, but I don't
> like this indentation - it makes it harder for my brain to read.  I tend to
> prefer the .this(() => {\n type style, meaning each nested promise just gets
> a single indent level.

I agree.  I've updated this to be less of a Pyramid Of Doom.  I hope the revised version is OK.

> @@ +90,5 @@
> > +            }
> > +          );
> > +        },
> > +        (error) => {
> > +          throw error;
> 
> I don't think this error handler adds any value - IIUC, the behaviour would
> be the same without this (ie, it might make sense if you logged the error
> before throwing etc).  Please correct me if I'm wrong about this :)

You're right.  I don't see the point of throwing this.  And I don't see what error could occur.  I'm removing the error handler.

> @@ +113,5 @@
> >     */
> >    signIn: function signIn(email, password) {
> > +    let hexEmail = CommonUtils.stringAsHex(email);
> > +    return credentials.setup(email, password)
> > +      .then(
> 
> ditto here re indentation and final throw.

Thank you.  Yes.

> @@ +179,5 @@
> >    accountKeys: function (keyFetchTokenHex) {
> >      let creds = this._deriveHawkCredentials(keyFetchTokenHex, "keyFetchToken");
> >      let keyRequestKey = creds.extra.slice(0, 32);
> >      let morecreds = CryptoUtils.hkdf(keyRequestKey, undefined,
> > +                                     kw("account/keys"), 3 * 32);
> 
> see comment in credentials.js about the naming of |kw| etc.

Adjusted ...

> ::: services/fxaccounts/credentials.js
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> I think this should be named Credentials.jsm (FxAccountsCommon.js sucks 'cos
> it doesn't use .jsm)

Better watch out or ima rewrite this whole thing in Dart.  Just kidding.  I've updated it to be a fresh jsm.

> @@ +36,5 @@
> > +  HMAC_ALGORITHM: HMAC_ALGORITHM,
> > +  HMAC_LENGTH: HMAC_LENGTH,
> > +};
> > +
> > +this.kw = function(context) {
> 
> can we give these better names?  The names were slightly more palatable when
> the function was local to the module that used it, but now that
> fxAccountsClient calls a function called "kw" it's not at all obvious where
> it comes from.  Another alternative that might be better still would be to
> have an object exported from here called, say, "CredentialsUtils" (or even
> directly on the credentials object below) - then seeing Credentials.kw(...)
> might not make much sense, but at least you know where it came from!

I confess I took these names straight from the spec.  And I double-confess that I initially found them confusing as well.  I have followed your advice by both extending the names and placing the methods on the Credentials object.  I hope it's clearer now.

> @@ +62,5 @@
> > +this.kwe = function kwe(name, email) {
> > +  return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
> > +};
> > +
> > +this.credentials = {
> 
> should be Credentials

Fixed.

> @@ +95,5 @@
> > +        CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, kw("unwrapBkey"), hkdfLength);
> > +
> > +      deferred.resolve(result);
> > +    } catch(err) {
> > +      deferred.reject(err);
> 
> What exceptions are expected here?  It looks like it might make sense to log
> this before throwing it, probably via Cu.reportError("fail to blah: " + err);

Yeah, I don't think we're expecting anything to blow up here.  I've removed the error handler.

> ::: services/fxaccounts/tests/xpcshell/test_client.js
> @@ +8,3 @@
> >  
> >  const FAKE_SESSION_TOKEN = "a0a1a2a3a4a5a6a7a8a9aaabacadaeafb0b1b2b3b4b5b6b7b8b9babbbcbdbebf";
> > +const CU = CommonUtils;
> 
> not that keen on this name - too close to Cu.  Is it even used?

So right.  And so not used.  Removed.

> @@ +122,4 @@
> >          let body = CommonUtils.readBytesFromInputStream(request.bodyInputStream);
> >          let jsonBody = JSON.parse(body);
> > +        // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#wiki-test-vectors
> > +        do_check_eq(jsonBody.email, "andré@example.org");
> 
> I don't understand these tests (but that's almost certainly my fault).  I
> was expecting to see the addition of tests rather then just modifying
> existing ones.

In this case, we are replacing the /raw_password endpoints with new endpoints, but the overall logic remains the same.  I've added some extra comments and checks in the test to make the flow clearer (especially as /accounts/login gets hit twice, once each by different checks).

I don't think we actually want new tests here; we just want the endpoints replaced and the checks for payload data to change a bit.

> ::: services/fxaccounts/tests/xpcshell/test_credentials.js
> @@ +5,5 @@
> > +Cu.import("resource://gre/modules/Promise.jsm");
> > +Cu.import("resource://services-common/utils.js");
> > +Cu.import("resource://services-crypto/utils.js");
> > +
> > +const CU = CommonUtils;
> 
> ditto here re "CU" vs "Cu".  If it is only used below, something like:
> 
> let {hexToBytes: h2b, hexAsString: h2s, ...} = CommonUtils;

You rule.  Done.

> should work (untested, but I *think* the order of the
> object-literal-like-thing is correct :)
> 
> @@ +172,5 @@
> > +      "quickStretchedPW is wrong");
> > +
> > +  do_check_eq(expected.authPW, b2h(results.authPW),
> > +      "authPW is wrong");
> > +  
> 
> another space nit :)

Argh.  Sorry.

Thanks again for your comments and feedback.  Revision forthcoming.
Cheers
j
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
f=markh

Hi, Brian, and welcome back!  How's this look?
thanks
j
Attachment #8370520 - Attachment is obsolete: true
Attachment #8370571 - Flags: review?(warner-bugzilla)
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #16)
> > I think |this| would be clearer here (and below)
> 
> Hrm.  This (no pun intended) will require restructuring this file a bit.  I
> agree that it's kind of gross as it is, but I'm hesitant to make the change.

I'm surprised by this (but I'm always surprised by |this| in javascript :)  How are they called in a manner that screws this?  But yeah, no big deal if it is a PITA.
Jed, and Mark, thank you so much for the excellent and timely work. Chris, can you think of an alternate reviewer? This bug is blocking a lot of progress for FxA on FxOS.
Flags: needinfo?(ckarlof)
Minor revisions forthcoming - some comment cleanup and normative pbkdf2 tests moved to common/crypto where they belong.
Comment on attachment 8370571 [details] [diff] [review]
Use onepw protocol in fxa client

Review of attachment 8370571 [details] [diff] [review]:
-----------------------------------------------------------------

Test coverage of /account/create would be nice, as well as tests for correct handling of error conditions for /account/create and /account/login: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md
However, I don't think the lack of tests should block these from landing. However, there are a few issues which need be fixed, e.g., accountExists is broken. r+ with the inline issues addressed and we need to have warner review this when he gets back.

::: services/common/utils.js
@@ +200,5 @@
> +    return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2)
> +      for (byte in bytes)].join("");
> +  },
> +
> +  stringAsHex: function stringAsHex(str) {

I assume this is new. Is there a test for this?

@@ +212,5 @@
>      }
>      return String.fromCharCode.apply(String, bytes);
>    },
>  
> +  hexAsString: function hexAsString(hex) {

I assume this is new. Is there a test for this?

::: services/crypto/modules/utils.js
@@ +170,5 @@
>     *
>     * The output is an octet string of length dkLen, which you
>     * can encode as you wish.
>     */
> +  pbkdf2Generate : function pbkdf2Generate(P, S, c, dkLen, hmacAlg="SHA1", hmacLen=20) {

It would be nice if we could default this to nsICryptoHMAC.SHA1 instead of this magic string.

::: services/fxaccounts/Credentials.jsm
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Link to our protocol/API docs would be helpful here.

@@ +23,5 @@
> +const PBKDF2_ROUNDS = 1000;
> +const STRETCHED_PW_LENGTH_BYTES = 32;
> +const HKDF_SALT = h2b("00");
> +const HKDF_LENGTH = 32;
> +const HMAC_ALGORITHM = "SHA256";

It would be nicer if this was nsICryptoHMAC.SHA256

@@ +37,5 @@
> +  HMAC_LENGTH: HMAC_LENGTH,
> +};
> +
> +this.Credentials = {
> +  keyWrap: function(context) {

I'm not completely sure here, but I don't think "KW" stands for "keyWrap". I think it's "keyword".

@@ +44,5 @@
> +    // Firefox Accounts API.  For this reason, it is not exposed as a pref.
> +    //
> +    // See:
> +    // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#creating-the-account
> +    return h2b(s2h(PROTOCOL_VERSION + context));

s2b might be useful.

@@ +59,5 @@
> +   *   @return {bitArray} the salt combination with the namespace
> +   *
> +   * Exported for testing
> +   */
> +  keyWrapExtend: function(name, email) {

I'm not completely sure here, but I don't think "KWE" stands for "keyWrapExtend". I think it's "keyword encoding" or "keyword extend".

@@ +60,5 @@
> +   *
> +   * Exported for testing
> +   */
> +  keyWrapExtend: function(name, email) {
> +    return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));

s2b might be useful.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +77,5 @@
> +        email: creds.emailUTF8,
> +        authPW: b2h(creds.authPW),
> +      };
> +      return this._request("/account/create", "POST", null, data).then((response) => {
> +        return this.signIn(email, password)

You don't need to do this signIn call anymore in signUp. You get the session and keyFetch tokens directly back from the /account/create call now: https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-v1accountcreate

@@ +103,5 @@
>     *          verified: flag indicating verification status of the email
>     *        }
>     */
>    signIn: function signIn(email, password) {
> +    let hexEmail = CommonUtils.stringAsHex(email);

email isn't hex encoded in the API anymore. I think this line can be deleted because you seem to be doing the right thing below.

@@ +230,5 @@
>     * @return Promise
>     *        The promise resolves to true if the account exists, or false
>     *        if it doesn't. The promise is rejected on other errors.
>     */
>    accountExists: function (email) {

accountsExists is going to fail because /auth/start is no longer part of the API. We don't have an "accountExists" API, so you'll have to change this /auth/start -> /account/login with the (non-hex encoded) email and an empty password. If you get a 102 the account doesn't exist and a 103 means that the account exists. This isn't ideal because too many hits against /account/login with the incorrect password will hit some throttling at some point.

::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +87,5 @@
> +  // Simple test vectors from the interwebs
> +  let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 20);
> +  do_check_eq(b2h(result), "0c60c80f961f0e71f3a9b524af6012062fe037a6");
> +
> +  let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 32, "SHA256", 32);

nsICryptoHMAC.SHA256 here and other places would be nicer.
Attachment #8370571 - Flags: review?(warner-bugzilla) → review+
I reviewed on behalf of warner. We can land because it's greenfield and low risk, but he need to have warner review when he returns from vacation.
Flags: needinfo?(ckarlof)
Attachment #8370571 - Flags: feedback?(warner-bugzilla)
(In reply to Chris Karlof [:ckarlof] from comment #22)
> Comment on attachment 8370571 [details] [diff] [review]
> Use onepw protocol in fxa client
> 
> Review of attachment 8370571 [details] [diff] [review]:
> -----------------------------------------------------------------

This was a very helpful review.  Thanks, Chris.

> Test coverage of /account/create would be nice, as well as tests for correct
> handling of error conditions for /account/create and /account/login:
> https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md
> However, I don't think the lack of tests should block these from landing.
> However, there are a few issues which need be fixed, e.g., accountExists is
> broken. r+ with the inline issues addressed and we need to have warner
> review this when he gets back.

All sounds good.

> ::: services/common/utils.js
> @@ +200,5 @@
> > +    return [("0" + bytes.charCodeAt(byte).toString(16)).slice(-2)
> > +      for (byte in bytes)].join("");
> > +  },
> > +
> > +  stringAsHex: function stringAsHex(str) {
> 
> I assume this is new. Is there a test for this?

I've added a slew of services/common tests for these functions.

> @@ +212,5 @@
> >      }
> >      return String.fromCharCode.apply(String, bytes);
> >    },
> >  
> > +  hexAsString: function hexAsString(hex) {
> 
> I assume this is new. Is there a test for this?

Added.

> ::: services/crypto/modules/utils.js
> @@ +170,5 @@
> >     *
> >     * The output is an octet string of length dkLen, which you
> >     * can encode as you wish.
> >     */
> > +  pbkdf2Generate : function pbkdf2Generate(P, S, c, dkLen, hmacAlg="SHA1", hmacLen=20) {
> 
> It would be nice if we could default this to nsICryptoHMAC.SHA1 instead of
> this magic string.

Agreed.  Fixed.

> ::: services/fxaccounts/Credentials.jsm
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> > + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Link to our protocol/API docs would be helpful here.

Good point.  Added it.

> @@ +23,5 @@
> > +const PBKDF2_ROUNDS = 1000;
> > +const STRETCHED_PW_LENGTH_BYTES = 32;
> > +const HKDF_SALT = h2b("00");
> > +const HKDF_LENGTH = 32;
> > +const HMAC_ALGORITHM = "SHA256";
> 
> It would be nicer if this was nsICryptoHMAC.SHA256

Ditto.

> @@ +37,5 @@
> > +  HMAC_LENGTH: HMAC_LENGTH,
> > +};
> > +
> > +this.Credentials = {
> > +  keyWrap: function(context) {
> 
> I'm not completely sure here, but I don't think "KW" stands for "keyWrap". I
> think it's "keyword".

I wasn't sure either.  I couldn't remember, and it's not unpacked in the docs.  I've gone for keyWord and keyWordExtended.  If it's wrong, warner can tell us and we can patch it in a follow-up, I guess.

> @@ +44,5 @@
> > +    // Firefox Accounts API.  For this reason, it is not exposed as a pref.
> > +    //
> > +    // See:
> > +    // https://github.com/mozilla/fxa-auth-server/wiki/onepw-protocol#creating-the-account
> > +    return h2b(s2h(PROTOCOL_VERSION + context));
> 
> s2b might be useful.

Yes indeed.  Added.

> @@ +59,5 @@
> > +   *   @return {bitArray} the salt combination with the namespace
> > +   *
> > +   * Exported for testing
> > +   */
> > +  keyWrapExtend: function(name, email) {
> 
> I'm not completely sure here, but I don't think "KWE" stands for
> "keyWrapExtend". I think it's "keyword encoding" or "keyword extend".

Ditto as above.

> @@ +60,5 @@
> > +   *
> > +   * Exported for testing
> > +   */
> > +  keyWrapExtend: function(name, email) {
> > +    return h2b(s2h(PROTOCOL_VERSION + name + ':' + email));
> 
> s2b might be useful.

Ditto as above.

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +77,5 @@
> > +        email: creds.emailUTF8,
> > +        authPW: b2h(creds.authPW),
> > +      };
> > +      return this._request("/account/create", "POST", null, data).then((response) => {
> > +        return this.signIn(email, password)
> 
> You don't need to do this signIn call anymore in signUp. You get the session
> and keyFetch tokens directly back from the /account/create call now:
> https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#post-
> v1accountcreate

Oh great!  Simplified.

> @@ +103,5 @@
> >     *          verified: flag indicating verification status of the email
> >     *        }
> >     */
> >    signIn: function signIn(email, password) {
> > +    let hexEmail = CommonUtils.stringAsHex(email);
> 
> email isn't hex encoded in the API anymore. I think this line can be deleted
> because you seem to be doing the right thing below.

Yes, that was an oversight.  Removed.

> @@ +230,5 @@
> >     * @return Promise
> >     *        The promise resolves to true if the account exists, or false
> >     *        if it doesn't. The promise is rejected on other errors.
> >     */
> >    accountExists: function (email) {
> 
> accountsExists is going to fail because /auth/start is no longer part of the
> API. We don't have an "accountExists" API, so you'll have to change this
> /auth/start -> /account/login with the (non-hex encoded) email and an empty
> password. If you get a 102 the account doesn't exist and a 103 means that
> the account exists. This isn't ideal because too many hits against
> /account/login with the incorrect password will hit some throttling at some
> point.

Ah, ok.  I've rewritten accountExists and revised the tests.

> ::: services/fxaccounts/tests/xpcshell/test_credentials.js
> @@ +87,5 @@
> > +  // Simple test vectors from the interwebs
> > +  let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 20);
> > +  do_check_eq(b2h(result), "0c60c80f961f0e71f3a9b524af6012062fe037a6");
> > +
> > +  let result = CryptoUtils.pbkdf2Generate("password", "salt", 1, 32, "SHA256", 32);
> 
> nsICryptoHMAC.SHA256 here and other places would be nicer.

Ditto, as above.

As an extra detail, I've moved the pbkdf2 tests to the services/crypto tests, and filled them out with ietf test vectors for both sha1 and sha256.  Sorting that out yesterday was too laborious not to leave a coherent test trail behind :)

Thanks for such a thorough read, Chris, and for all your time working together yesterday.  Revised patch is building against inbound on my machine atm.  I'll update it and check it in when it's done and all tests are still green.

Cheers
j
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
f=markh; r=ckarlof
Attachment #8370571 - Attachment is obsolete: true
Attachment #8370571 - Flags: feedback?(warner-bugzilla)
Attachment #8371062 - Flags: feedback?(warner-bugzilla)
Comment on attachment 8371062 [details] [diff] [review]
Use onepw protocol in fxa client

Review of attachment 8371062 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/FxAccountsClient.jsm
@@ +225,5 @@
>     *        The promise resolves to true if the account exists, or false
>     *        if it doesn't. The promise is rejected on other errors.
>     */
>    accountExists: function (email) {
> +    return this.signIn(email, "").then(

I assume passing an empty string here doesn't cause anything to blow up?
I spoke with bsmith last night. He told me there was some magic we could perform using the PBKDF2 implementation in NSS that will allow us to get the resulting key out. This would yield a perf improvement, I believe.
Comment on attachment 8371062 [details] [diff] [review]
Use onepw protocol in fxa client

Review of attachment 8371062 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Jed, I hope you don't mind me coming along and doing a drive-by review. I didn't really check any of the behavior, I just have a few style suggestions from a quick glance on the code.

::: services/fxaccounts/Credentials.jsm
@@ +18,5 @@
> +Cu.import("resource://gre/modules/Promise.jsm");
> +Cu.import("resource://services-crypto/utils.js");
> +Cu.import("resource://services-common/utils.js");
> +
> +let {stringToBytes: s2b, hexToBytes: h2b} = CommonUtils;

Nit: I think it would be better to keep the original names here instead of shortening them. The current code makes grepping or using MXR just harder when searching of occurrences of these somewhat basic functions.

@@ +31,5 @@
> +
> +this.constants = {
> +  PROTOCOL_VERSION: PROTOCOL_VERSION,
> +  PBKDF2_ROUNDS: PBKDF2_ROUNDS,
> +  STRETCHED_PW_LENGTH_BYTES: STRETCHED_PW_LENGTH_BYTES,

Wouldn't it be nicer to define those in the "Credentials" object? Modules exporting only one symbol are a little easier to use and clients could just use |Credentials.HKDF_SALT| etc. It probably doesn't make sense to have the consts here and then maintain another list, just define them in the object once?

@@ +38,5 @@
> +  HMAC_ALGORITHM: HMAC_ALGORITHM,
> +  HMAC_LENGTH: HMAC_LENGTH,
> +};
> +
> +this.Credentials = {

Nit: this.Credentials = Object.freeze({

});

@@ +104,5 @@
> +      CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, this.keyWord("unwrapBkey"), hkdfLength);
> +
> +    deferred.resolve(result);
> +
> +    return deferred.promise;

Nit: you don't actually need the deferred here. You can just do |return Promise.resolve(result)|.

::: services/fxaccounts/FxAccountsClient.jsm
@@ +14,5 @@
>  Cu.import("resource://services-crypto/utils.js");
>  Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +Cu.import("resource://gre/modules/Credentials.jsm");
> +
> +let b2h = CommonUtils.bytesAsHex;

Nit: same comment about the shortened function name.

::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +8,5 @@
> +
> +let {hexToBytes: h2b,
> +     hexAsString: h2s,
> +     stringAsHex: s2h,
> +     bytesAsHex: b2h} = CommonUtils;

I don't mind so much doing that in a test but if the first thing in a file using those methods is to establish short names for them then maybe just rename the functions?
> Hey Jed, I hope you don't mind me coming along and doing a drive-by review.

Thanks Tim!
(In reply to Chris Karlof [:ckarlof] from comment #26)
> Comment on attachment 8371062 [details] [diff] [review]
> Use onepw protocol in fxa client
> 
> Review of attachment 8371062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +225,5 @@
> >     *        The promise resolves to true if the account exists, or false
> >     *        if it doesn't. The promise is rejected on other errors.
> >     */
> >    accountExists: function (email) {
> > +    return this.signIn(email, "").then(
> 
> I assume passing an empty string here doesn't cause anything to blow up?

It doesn't blow anything up in my accountExists tests in test_client.js.  I assume the server is just going to treat it as an ill-formed password?
(In reply to Tim Taubert [:ttaubert] from comment #28)
> Comment on attachment 8371062 [details] [diff] [review]
> Use onepw protocol in fxa client
> 
> Review of attachment 8371062 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Jed, I hope you don't mind me coming along and doing a drive-by review.
> I didn't really check any of the behavior, I just have a few style
> suggestions from a quick glance on the code.

I am always grateful for your feedback.  Thanks for taking a look.

> ::: services/fxaccounts/Credentials.jsm
> @@ +18,5 @@
> > +Cu.import("resource://gre/modules/Promise.jsm");
> > +Cu.import("resource://services-crypto/utils.js");
> > +Cu.import("resource://services-common/utils.js");
> > +
> > +let {stringToBytes: s2b, hexToBytes: h2b} = CommonUtils;
> 
> Nit: I think it would be better to keep the original names here instead of
> shortening them. The current code makes grepping or using MXR just harder
> when searching of occurrences of these somewhat basic functions.

That's a very good point.  I'm glad you pointed that out.  Fixed.

> @@ +31,5 @@
> > +
> > +this.constants = {
> > +  PROTOCOL_VERSION: PROTOCOL_VERSION,
> > +  PBKDF2_ROUNDS: PBKDF2_ROUNDS,
> > +  STRETCHED_PW_LENGTH_BYTES: STRETCHED_PW_LENGTH_BYTES,
> 
> Wouldn't it be nicer to define those in the "Credentials" object? Modules
> exporting only one symbol are a little easier to use and clients could just
> use |Credentials.HKDF_SALT| etc. It probably doesn't make sense to have the
> consts here and then maintain another list, just define them in the object
> once?

Agreed.  Fixed.

> @@ +38,5 @@
> > +  HMAC_ALGORITHM: HMAC_ALGORITHM,
> > +  HMAC_LENGTH: HMAC_LENGTH,
> > +};
> > +
> > +this.Credentials = {
> 
> Nit: this.Credentials = Object.freeze({
> 
> });

Thanks.  Done.

> @@ +104,5 @@
> > +      CryptoUtils.hkdf(quickStretchedPW, hkdfSalt, this.keyWord("unwrapBkey"), hkdfLength);
> > +
> > +    deferred.resolve(result);
> > +
> > +    return deferred.promise;
> 
> Nit: you don't actually need the deferred here. You can just do |return
> Promise.resolve(result)|.

Ah yes.  Fixed.

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +14,5 @@
> >  Cu.import("resource://services-crypto/utils.js");
> >  Cu.import("resource://gre/modules/FxAccountsCommon.js");
> > +Cu.import("resource://gre/modules/Credentials.jsm");
> > +
> > +let b2h = CommonUtils.bytesAsHex;
> 
> Nit: same comment about the shortened function name.

Fixed.

> ::: services/fxaccounts/tests/xpcshell/test_credentials.js
> @@ +8,5 @@
> > +
> > +let {hexToBytes: h2b,
> > +     hexAsString: h2s,
> > +     stringAsHex: s2h,
> > +     bytesAsHex: b2h} = CommonUtils;
> 
> I don't mind so much doing that in a test but if the first thing in a file
> using those methods is to establish short names for them then maybe just
> rename the functions?

My inclination is to let this stand as it is.  I think the substitute names keep the lines short and more legible in the tests.  But I don't feel comfortable just changing all the names in CommonUtils in this patch, as most of them are as I found them.  Still, they could be shorter, more consistent (fooAsBar vs fooToBar), and in cases clearer ("Bytes" are not what I'd think of as bytes).

Perhaps such a clean-up would be appropriate for a follow-up bug?  I'd be glad to do it.  I just didn't want to muddy the waters any more than I have with this patch!

Many thanks again for your feedback.
Cheers,
j
I rejoice in seeing so many people giving helpful feedback and criticism on the FxA bugs.  Thanks to all who have been watching this product and chipping in with advice.  It's a healthy sign, and it makes it a pleasure to work on it.
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
Updated according to feedback from :ttaubert
Attachment #8371062 - Attachment is obsolete: true
Attachment #8371062 - Flags: feedback?(warner-bugzilla)
Attachment #8371134 - Flags: feedback?(warner-bugzilla)
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #31)
> Perhaps such a clean-up would be appropriate for a follow-up bug?  I'd be
> glad to do it.  I just didn't want to muddy the waters any more than I have
> with this patch!

Yes, this would not warrant being handled here but rather in a follow-up. I actually don't think we should do that and keep the more legible function names. It's totally fine to shorten those in tests if it helps there. Thank you for addressing all the feedback!
Status: NEW → ASSIGNED
Testing this on a 2.3GHz laptop, with a CPU dedicated to the test, 1000 rounds of pbkdf2 take a whopping 3.5 seconds to compute (presumably due to the overhead of shoving data back and forth so many times across the XPCOM blood-brain barrier between the JavaScript PBKDF2 function on the one side and its C++ HMAC hasher on the other).  This is going to be horrid on b2g.

ckarlof has spoken to bsmith about this, and bsmith says he can implement PBKDF2 HMAC SHA256 in NSS for us next week.  I've opened Bug 968567 to track this.

Sam, given that this is a huge blocker for b2g, how do you want to proceed regarding the present patch?

a. Land it and just suck it up on b2g for a week?  (slow dev, but unblocked)

b. Revise the patch to include an all-js implementation of pbkdf2 (e.g., sjcl), not landing it, but using it in dev to tide us over until Bug 968567 lands?

c. Try it first on b2g before passing judgment?

d. Something else?
Flags: needinfo?(spenrose)
Land it.
Flags: needinfo?(spenrose)
Comment on attachment 8371134 [details] [diff] [review]
Use onepw protocol in fxa client

Review of attachment 8371134 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/Credentials.jsm
@@ +72,5 @@
> +   * Note that PROTOCOL_VERSION does not refer in any way to the version of the
> +   * Firefox Accounts API.
> +   */
> +  keyWordExtended: function(name, email) {
> +    return stringToBytes(PROTOCOL_VERSION + name + ':' + email);

s/stringToBytes/CommonUtils.stringToBytes
I am testing this patch in b2g and I cannot create an account.

With the dev server I am getting:

Gecko  I  1391692658109	FirefoxAccounts	DEBUG	Got content msg {"id":"9460b072-5032-3569-5db8-229eea1df6cf","data":{"method":"queryAccount","accountId":"ggggg@ggggg"}}
Gecko  I  1391692658203	FirefoxAccounts	DEBUG	queryAccount ggggg@ggggg
GeckoConsole  E  Content JS WARN at app://system.gaiamobile.org/shared/js/l10n.js:79 in consoleWarn: [l10n] #fxa-connecting-to-firefox is undefined.
Gecko  I  1391692660668	FirefoxAccounts	DEBUG	(Response) code: 400 - Status text: Bad Request
Gecko  I  1391692660673	FirefoxAccounts	DEBUG	Clock offset vs https://api-accounts.dev.lcip.org/v1: -673
Gecko  I  1391692660693	FirefoxAccounts	DEBUG
Gecko  I  1391692660694	FirefoxAccounts	DEBUG	Chrome event {"id":"9460b072-5032-3569-5db8-229eea1df6cf","data":{"registered":false}}
Gecko  I  1391692666680	FirefoxAccounts	DEBUG	Got content msg {"id":"b44667f5-c8ed-7495-a428-c9617e929cbd","data":{"method":"signUp","accountId":"ggggg@ggggg","password":"11111111"}}
GeckoConsole  E  Content JS WARN at app://system.gaiamobile.org/shared/js/l10n.js:79 in consoleWarn: [l10n] #fxa-registering is undefined.
Gecko  I  1391692669040	FirefoxAccounts	DEBUG	(Response) code: 200 - Status text: OK
Gecko  I  1391692669041	FirefoxAccounts	DEBUG	Clock offset vs https://api-accounts.dev.lcip.org/v1: -1041
Gecko  I  1391692669063	FirefoxAccounts	ERROR	INTERNAL_ERROR_INVALID_USER
Gecko  I  1391692669071	FirefoxAccounts	DEBUG	Chrome event {"id":"b44667f5-c8ed-7495-a428-c9617e929cbd","error":{"error":"INTERNAL_ERROR_INVALID_USER","details":{"user":{"uid":"e34599af6d1a4e9bab366a95d01f1675"}}}}



With the legacy server:

Gecko  I  1391693169403	FirefoxAccounts	DEBUG	Clock offset vs https://api-accounts-legacy.dev.lcip.org/v1: -403
Gecko  I  *************************
Gecko  I  A coding exception was thrown in a Promise rejection callback.
Gecko  I  Full message: ReferenceError: err is not defined
Gecko  I  See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Gecko  I  Full stack: this.FxAccountsClient.prototype.accountExists/<@resource://gre/modules/FxAccountsClient.jsm:240
Gecko  I  Handler.prototype.process@resource://gre/modules/Promise.jsm:770
Gecko  I  this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm:531
Gecko  I  *************************
GeckoConsole  E  Full Stack: JS frame :: resource://gre/modules/FxAccountsManager.jsm :: this.FxAccountsManager._serverError :: line 75
GeckoConsole  E  JS frame :: resource://gre/modules/FxAccountsManager.jsm :: this.FxAccountsManager.queryAccount/< :: line 306
GeckoConsole  E  JS frame :: resource://gre/modules/Promise.jsm :: Handler.prototype.process :: line 770
GeckoConsole  E  JS frame :: resource://gre/modules/Promise.jsm :: this.PromiseWalker.walkerLoop :: line 531
GeckoConsole  E  native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"]
Gecko  I  1391693206193	FirefoxAccounts	DEBUG	(Response) code: 400 - Status text: Bad Request
Gecko  I  1391693206194	FirefoxAccounts	DEBUG	Clock offset vs https://api-accounts-legacy.dev.lcip.org/v1: -194
Gecko  I  1391693206197	FirefoxAccounts	ERROR	UNKNOWN_ERROR
Gecko  I  1391693206204	FirefoxAccounts	DEBUG	Chrome event {"id":"31b3cec3-0869-b716-ab03-ab2c6313e336","error":{"error":"UNKNOWN_ERROR","details":{"code":400,"error":"Bad Request","message":"the value of email must match the RegExp &#x2f;&#x5e;&#x28;&#x3f;:&#x5b;a-fA-F0-9&#x5d;&#x7b;2&#x7d;&#x29;&#x2b;40&#x28;&#x3f;:&#x5b;a-fA_F0-9&#x5d;&#x7b;2&#x7d;&#x29;&#x2b;&#x24;&#x2f;, the value of srp is not allowed to be undefined, the value of passwordStretching is not allowed to be undefined, the key &#x28;authPW&#x29; is not allowed","info":"https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#response-format","validation":{"source":"payload","keys":["email","srp","passwordStretching","authPW"]},"errno":999}}}
GeckoConsole  E  [JavaScript Error: "TypeError: l10nKeys is undefined" {file: "app://system.gaiamobile.org/fxa/js/fxam_errors.js" line: 55}]
We *really* need mochitests :(

(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #35)
> Testing this on a 2.3GHz laptop, with a CPU dedicated to the test, 1000
> rounds of pbkdf2 take a whopping 3.5 seconds to compute (presumably due to
> the overhead of shoving data back and forth so many times across the XPCOM
> blood-brain barrier between the JavaScript PBKDF2 function on the one side
> and its C++ HMAC hasher on the other).  This is going to be horrid on b2g.
> 
> ckarlof has spoken to bsmith about this, and bsmith says he can implement
> PBKDF2 HMAC SHA256 in NSS for us next week.  I've opened Bug 968567 to track
> this.
>

Is this going to run in the main thread?
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #37)
> Comment on attachment 8371134 [details] [diff] [review]
> Use onepw protocol in fxa client
> 
> Review of attachment 8371134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/fxaccounts/Credentials.jsm
> @@ +72,5 @@
> > +   * Note that PROTOCOL_VERSION does not refer in any way to the version of the
> > +   * Firefox Accounts API.
> > +   */
> > +  keyWordExtended: function(name, email) {
> > +    return stringToBytes(PROTOCOL_VERSION + name + ':' + email);
> 
> s/stringToBytes/CommonUtils.stringToBytes

Holy cow.  How did that even pass any of the tests???  kwe is repeatedly tested.  Ugh.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #39)
> We *really* need mochitests :(

Yes, we do.

> (In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from
> comment #35)
> > Testing this on a 2.3GHz laptop, with a CPU dedicated to the test, 1000
> > rounds of pbkdf2 take a whopping 3.5 seconds to compute (presumably due to
> > the overhead of shoving data back and forth so many times across the XPCOM
> > blood-brain barrier between the JavaScript PBKDF2 function on the one side
> > and its C++ HMAC hasher on the other).  This is going to be horrid on b2g.
> > 
> > ckarlof has spoken to bsmith about this, and bsmith says he can implement
> > PBKDF2 HMAC SHA256 in NSS for us next week.  I've opened Bug 968567 to track
> > this.
> >
> 
> Is this going to run in the main thread?

Good point.  I will fix this.
ferjm, thanks for all this testing, and for catching the 'err' error in comment 38.  afaik, this isn't going to be used by desktop, only b2g.  does that mean we really want marionette tests?
Flags: needinfo?(ferjmoreno)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #38)
> I am testing this patch in b2g and I cannot create an account.
> 
> With the dev server I am getting:

Going forward, I would recommend using the prod FxA API server (https://api.accounts.firefox.com/v1). The role of the dev servers is being reconsidered.

-chris
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #42)
> ferjm, thanks for all this testing, and for catching the 'err' error in
> comment 38.  afaik, this isn't going to be used by desktop, only b2g.  does
> that mean we really want marionette tests?

I would start with mochitests for services/fxaccounts as suggested in Bug 967508. We should be able to mock the server behavior with server-side JavaScript [1]. Marionette tests would also be great for FxAccounts and the mozId API. We can ask the Gaia QA team to help with this [2]. We already have UI tests for the mozId API, we just need to update them again with the FxAccounts bits that were backed out.

[1] https://developer.mozilla.org/en-US/docs/Mochitest#How_do_I_write_tests_that_check_header_values.2C_method_types.2C_etc._of_HTTP_requests.3F
[2] https://groups.google.com/d/msg/mozilla.dev.b2g/f_LCBGv_InY/IO0sDFIZGv4J
Flags: needinfo?(ferjmoreno)
WIP - updating tests

- revised test_client.js (xpcshell)
  - separated all api tests
  - one test per client api call
  - exercise all error paths for each api call

working on mochitests now ...
Attachment #8371134 - Attachment is obsolete: true
Attachment #8371134 - Flags: feedback?(warner-bugzilla)
Oh - and all that pbkdf2 stuff happens on a separate thread now, per Comment 39
Verified with spenrose that the updated patch works on the production server, but using the desktop emulator.

I'm building for my hamachi now; if anyone has a build ready to go, and can throw this patch on, please pref identity.fxaccounts.loglevel="DEBUG" and try to sign in.

I want to know how long pbkdf2 takes on the device.  grep for the string "Credentials set up after" to see how many ms it took for 1000 rounds of pbkdf2 with the current (slow) implementation.

Thanks, j
Comment on attachment 8371751 [details] [diff] [review]
Use onepw protocol in fxa client (WIP)

Review of attachment 8371751 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Jed! I'll try to test this again.

::: services/fxaccounts/Credentials.jsm
@@ +129,5 @@
> +      deferred.resolve(result);
> +    }
> +
> +    Services.tm.currentThread.dispatch(runnable,
> +        Ci.nsIThread.DISPATCH_NORMAL);

If I am not wrong, 'currentThread' is still the main thread in this case, so you are just queuing the task on the main thread (i.e setTimeout). I am not sure if the ThreadManager can be used safely to create a new thread from JS :\, so we might need a chrome worker instead.
From [1]:

"Please note that as of Firefox 4, nsIThread.dispatch does not accept nsIRunnable-s implemented in Javascript and created on a different thread than the thread you're trying to dispatch the nsIRunnable to. This limitation was implemented to avoid crashes caused by changes especially to the Javascript strings implementation. This effectively means that javascript extensions cannot use the nsIThread API anymore to execute own jobs on different threads than the main thread. Consider Web/ChromeWorker as a replacement, which are severely limited in what you can do with them, or just don't use threads."

So, yeah, we need a chrome worker.

[1] https://wiki.mozilla.org/Performance/Addons/BestPractices#Consider_using_web_workers_and.2For_nsIThread
Can we break the chrome worker issue into a separate bug, or does it need to block the rest of the patch?
D'oh!  Thanks, ferjm.  Chrome worker it is.
(In reply to Sam Penrose from comment #50)
> Can we break the chrome worker issue into a separate bug, or does it need to
> block the rest of the patch?

I think it looks pretty straightforward to implement.  I'm grabbing breakfast and coffee, and then I'll take a stab at it.
I take that back.  This isn't straightforward at all.

XPCOM is not exposed to ChromeWorkers, so I see no way to use CryptoUtils in a worker.
Bug 608156 Comment 8 seems relevant:

> From the results outlined in bug 624485 comment 1, this is likely more trouble 
> than it's worth.  Because we decrypt one record per call, we're talking about 
> thousands of small calls, none of which are long enough on their own to block 
> the main thread for any meaningful amount of time.  Without a bulk API in NSS, 
> using ChromeWorker for these ops likely add even more overhead with little benefit.

> Instead, we're going to focus on making the crypto pieces much more efficient 
> with the intent of reducing overall time spent in crypto.
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #54)
> Bug 608156 Comment 8 seems relevant:

Quite so.

> Without a bulk API in NSS, 
> using ChromeWorker for these ops likely add even more overhead with little benefit.

Consider also serialization overhead from main thread to worker.

Workers only make sense inasmuch as you can put most of the feature in a worker -- reading directly from databases inside the worker, getting access to crypto from inside the worker, etc.

Otherwise you're just offloading a small chunk of work, and paying for it in messaging overhead.

See also Bug 649154 / Bug 721193, Bug 850711.
ferjm, spenrose, and I have spent the morning working on this.

Of this patch, we've concluded:
- Account creation and signin work properly on emulator and device
- Performance is ok, taking a little over a second on hamachi
  - that means hamachi is 3x faster than tests on my mac! xpcshell-test overhead?
- Crypto work doesn't block UI
- Crypto performance should be solved the right way with Bug 968567

Consensus is that we should land this to unblock progress towards b2g 1.4.
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
Added a few more tests for pbkdf2 results because I guess I just love hex strings.
Attachment #8371751 - Attachment is obsolete: true
In testing this patch, :ferjm discovered a problem with signing in while fxa was still polling for a different user's email.  Sadly, there is a test in test_accounts.js to test this very case, but it appears not to be sufficient.

I have opened Bug 969546 for this issue.
Thanks for all the work that you've done here Jed!

Let's wait for the try push before pushing to inbound: https://tbpl.mozilla.org/?tree=Try&rev=5c918162e609
And thanks for all your input and help, as always, Fernando.

I hope the try push won't be much different from when I tried on Comment 20 (the patch hasn't changed much, in essence, since then, and all local tests pass).  But you are wise to be cautious.

Awaiting the outcome.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/79b198be3e50
Flags: in-testsuite+
Whiteboard: [qa+] → [qa+][fixed-in-fx-team]
s/bang/whimper/
The report in comment 62 is:

14:32:06  WARNING -  TEST-UNEXPECTED-FAIL | C:/slave/test/build/tests/xpcshell/tests/services/fxaccounts/tests/xpcshell/test_client.js | null == 102 - See following stack:
14:32:06     INFO -  JS frame :: C:/slave/test/build/tests/xpcshell/tests/services/fxaccounts/tests/xpcshell/test_client.js :: test_accountExists :: line 481

Fantastic.  I can't reproduce this failure locally on my mac.
Whiteboard: [qa+][fixed-in-fx-team] → [qa+]
OS: Gonk (Firefox OS) → All
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #64)
> Fantastic.  I can't reproduce this failure locally on my mac.

Yeah, this failure is Windows-only.
Google search for "site:bugzilla.mozilla.org WindowsError: [Error 6] The handle is invalid." returns 23,000 results.  Just sayin.
A local run on my Windows VM seems to confirm that there might be too many open connections. It isn't tied to the specific 102 return code test, everything after those requests fails.
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
Refactored accountExists test so we don't have multiple connections to the same server.  Hoping this addresses the mysterious Windows failure.
Attachment #8372476 - Attachment is obsolete: true
Attached patch Use onepw protocol in fxa client (obsolete) — — Splinter Review 
I suppose it would have helped had I run 'hg qrefresh' first.
Attachment #8372691 - Attachment is obsolete: true
Yay, attachment 8372700 [details] [diff] [review] is looking good.

The "fix" was to break the accountExists test into three separate tests, each of which only uses a test http server once.

Profound thanks to ttaubert for helping debug a windows build at 3am localtime.

tbpl:

https://tbpl.mozilla.org/?tree=Try&rev=54f0fcfb301d
Oh bother.  Still some windows tests failing.
It looks like the same failure pattern in test_accountKeys, where the http server is used for four successive requests.
Refactored accountKeys test into four separate tests to try to fix intermittent orange on Windows that appears to occur when http servers get too many requests.

https://tbpl.mozilla.org/?tree=Try&rev=f5ef41601a2e
Attachment #8372700 - Attachment is obsolete: true
Bug 967372 reports an intermittent error with the test_hawk.js suite that looks like it's failing with the same pattern.  (A test file with lots of separate http servers, Windows eventually complaining NS_ERROR_NOT_AVAILABLE, and then the test failing.)

Unfortunately, that test suite looks very much like my "fixed" suite in this current patch.  That is, each server fields at most two requests.  So I'm wondering if whether the solution is instead to ensure that the tests run sequentially, not concurrently as they do now.
The test also fails on Mac for me, although it succeeds in 20% of all runs. To make it fail all I need is test_accountKeys() and test_accountExists(). Removing one of them make the test succeed, so it looks like they somehow interfere with each other...

Too many requests in any of those two tests doesn't seem to be the issue, I can e.g. increase the number of client.accountExists() calls by 100x and it still succeeds.
Oh, and test_client.js does fail in sequential mode. I think we don't split up sub-tests anyway, only by file.
This seems all caused by multiple instances of:

add_task(function () {
  yield deferredStop(server);
  run_next_test();
});

add_task() does already call run_next_test() [1] so calling it again probably messes up the test harness somehow.

[1] https://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#1364
Re-pushed the original patch with those minor fixes as described in comment #77:

https://hg.mozilla.org/integration/fx-team/rev/6e77dfcf5c6e

Running in parallel, sequential and standalone mode all succeeded on the Mac and Windows VM.
w00t!

Thanks for digging in more, running the tests so many times, and pushing the patch with the fixes, Tim.  This is great news!

Good catch in comment 77; I'll look for that in my other tests as well.

I noticed last night that the failure-case tests in this patch aren't all guaranteed to be executed, since they are in try-catch blocks, and if there's no exception, the test will just flow through them happily.  In fact one of them was broken in this way.  I fixed that in attachment 8372775 [details] [diff] [review], but forgot to mention it in comments.  I'll file a follow-up bug to fix them.

Finally, as ckarlof said in Comment 23, we should ask warner to take a look at this when he's back from PTO next week.

Thanks, everyone, for all your comments and feedback!
Flags: needinfo?(warner-bugzilla)
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #79)
> I noticed last night that the failure-case tests in this patch aren't all
> guaranteed to be executed, since they are in try-catch blocks, and if
> there's no exception, the test will just flow through them happily.  In fact
> one of them was broken in this way.  I fixed that in attachment 8372775 [details] [diff] [review]
> [details] [diff] [review], but forgot to mention it in comments.  I'll file
> a follow-up bug to fix them.

Oh, sorry. I didn't know that there's some more fixes in the patch than just splitting tests into multiple ones. Thanks for filing the follow-up!
I've opened Bug 969892 for the test suite adjustments
https://hg.mozilla.org/mozilla-central/rev/6e77dfcf5c6e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
No longer depends on: 955953
Component: Identity → FxA
Comment on attachment 8372775 [details] [diff] [review]
Use onepw protocol in fxa client

Review of attachment 8372775 [details] [diff] [review]:
-----------------------------------------------------------------

Crypto looks fine to me: it'd be nice to fix those two salt values, but it's not a blocker. So sorry I took so long on this!

::: services/fxaccounts/Credentials.jsm
@@ +23,5 @@
> +
> +const PROTOCOL_VERSION = "identity.mozilla.com/picl/v1/";
> +const PBKDF2_ROUNDS = 1000;
> +const STRETCHED_PW_LENGTH_BYTES = 32;
> +const HKDF_SALT = CommonUtils.hexToBytes("00");

Does this mean the default salt is a single NUL byte (length=1)? The spec calls for an empty string (length=0). It turns out this is fine (as verified by your test vectors), because HMAC pads short keys (anything shorter than the hash size) with zero bytes, so key="" and key="\x00" and key="\x00\x00\x00" will all behave the same way. But it might be nice to change this to hexToBytes(""), if that's legal, for the benefit of future readers.

@@ +88,5 @@
> +   *
> +   * Note that PROTOCOL_VERSION does not refer in any way to the version of the
> +   * Firefox Accounts API.
> +   */
> +  keyWordExtended: function(name, email) {

It doesn't matter, but the "E" in "KWE" actually stands for "email" :)

::: services/fxaccounts/tests/xpcshell/test_credentials.js
@@ +52,5 @@
> +  let authKeyInfo = Credentials.keyWord('authPW');
> +  do_check_eq(b2h(authKeyInfo), "6964656e746974792e6d6f7a696c6c612e636f6d2f7069636c2f76312f617574685057");
> +
> +  // derive auth password
> +  let hkdfSalt = h2b("00");

ditto: h2b("") would be more precise

@@ +100,5 @@
> +
> +  do_check_eq(expected.quickStretchedPW, b2h(results.quickStretchedPW),
> +      "quickStretchedPW is wrong");
> +
> +  do_check_eq(expected.authPW, b2h(results.authPW),

excellent test vectors!
Attachment #8372775 - Flags: review+
Comment on attachment 8372775 [details] [diff] [review]
Use onepw protocol in fxa client

oops, set the wrong flag
Attachment #8372775 - Flags: review+
Flags: needinfo?(warner-bugzilla)
This bug is blocking me from getting code changes related to TPS on bug 966434 landed on Aurora. Can we please get this backported?

I also request blocking 29.0 for that given that it will not allow us any testing.
status-firefox29: --- → affected
status-firefox30: --- → fixed
tracking-firefox29: --- → ?
Sorry, it looks like this is a complete change of the underlying protocol and which may not make it into Firefox 29.0. If that is the case I will have to update my patches to work with the older authentication protocol.
status-firefox29: affected → ---
tracking-firefox29: ? → ---
Flags: needinfo?(ckarlof)
Also asking info from jed. Would it be possible to backport or is it too dangerous and/or not wanted?
Flags: needinfo?(jparsons)
Henrik, the "old protocol" isn't supported anymore in the server, so "updating your patches to work with the older auth protocol isn't going to work". I had forgotten this patch wasn't uplifted to aurora. At the didn't impact the Sync 29 effort (but now it does).
Flags: needinfo?(ckarlof)
Thank you Chris. Given that both bugs I have referenced are blockers for Firefox 29, this should also become one. Asking for tracking firefox-29.
status-firefox29: --- → affected
tracking-firefox29: --- → ?
Comment on attachment 8372775 [details] [diff] [review]
Use onepw protocol in fxa client

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 943521
User impact if declined: 
  Firefox Accounts and Sync won't work
  Sorrow and gnashing of teeth
Testing completed (on m-c, etc.): fx-team, m-i, and m-c
Risk to taking this patch (and alternatives if risky): 
  Risk is low; it's been in use with active QA for a month
  No alternatives.  The Firefox Accounts auth servers have all
  been updated to use the protocol this patch supports
String or IDL/UUID changes made by this patch: none
Attachment #8372775 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jparsons)
This patch didn't apply for me on aurora: https://bugzilla.mozilla.org/show_bug.cgi?id=981921#c19
Drat.  Ok, getting a fresh aurora checkout.  Will upload a rebased patch.
Think I got it sorted out.  Building now in order to test.
Attachment #8372775 - Flags: approval-mozilla-aurora?
Comment on attachment 8391487 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 943521
User impact if declined: 
  Firefox Accounts and Sync won't work
  Sorrow and gnashing of teeth
Testing completed (on m-c, etc.): fx-team, m-i, and m-c
Risk to taking this patch (and alternatives if risky): 
  Risk is low; it's been in use with active QA for a month
  No alternatives.  The Firefox Accounts auth servers have all
  been updated to use the protocol this patch supports
String or IDL/UUID changes made by this patch: none

The attachment whose title begins with [aurora rebase] is the one we want
Attachment #8391487 - Flags: approval-mozilla-aurora?
Jed, this bug is marked as tracking firefox-29. So no approval request of the attachment is necessary. But you most likely should ask for review before getting it landed. Thanks for fixing it that quickly. If it doesn't regress anything I will backport my patches most likely on  Sunday.
(In reply to Henrik Skupin (:whimboo) from comment #96)
> Jed, this bug is marked as tracking firefox-29. So no approval request of
> the attachment is necessary.

That's not true. Requesting approval is the correct course of action.
Comment on attachment 8391487 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth

Oh!  Good to know.  I asked around and got the impression that this was enough.  :rnewman, can I ask you for a sanity-check review of this patch?

Thanks
j
Attachment #8391487 - Flags: review?(rnewman)
Comment on attachment 8391487 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth

Review of attachment 8391487 [details] [diff] [review]:
-----------------------------------------------------------------

Skimming, this looks fine; I recommend manual testing, though!

rs=me

::: services/fxaccounts/FxAccountsClient.jsm
@@ +154,5 @@
>     * @return Promise
>     *        Returns a promise that resolves to an object:
>     *        {
> +   *          kA: an encryption key for recevorable data (bytes)
> +   *          wrapKB: an encryption key that requires knowledge of the 

Pah trailing whitespace.

::: services/fxaccounts/tests/xpcshell/test_client.js
@@ +217,5 @@
> +    do_throw("Expected an error");
> +  } catch(expectedError) {
> +    do_check_eq(101, expectedError.errno);
> +  }
> + 

ws

@@ +221,5 @@
> + 
> +  yield deferredStop(server);
> +  run_next_test();
> +});
> + 

ws

@@ +509,5 @@
> +      response.bodyOutputStream.write(errorMessage, errorMessage.length);
> +      return;
> +    },
> +  });
> + 

ws
Attachment #8391487 - Flags: review?(rnewman) → review+
I agree with rnewman, this should get a manual test of FxA Sync. gavin had similar concerns.
r=rnewman
Attachment #8391487 - Attachment is obsolete: true
Attachment #8391487 - Flags: approval-mozilla-aurora?
(In reply to Richard Newman [:rnewman] from comment #99)
> Comment on attachment 8391487 [details] [diff] [review]
> [aurora rebase] use onepw protocol for auth
> 
> Review of attachment 8391487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Skimming, this looks fine; I recommend manual testing, though!
> 
> rs=me

Thanks, Richard,

Sorry about the whitespace.  My text editor is doing something wonky.

Testing manually now.
I tested as follows:
- Go to about:accounts and create a new account
- Verify account
- Manage sync
- Disconnect
- Sign in again

And a sync:
- Configure logOnSuccess to true
- Open some new tabs
- Choose Tools -> Sync Now
- See success logs in about:sync-log as expected
Comment on attachment 8391600 [details] [diff] [review]
[aurora rebase] use onepw protocol for auth

[Approval Request Comment]

Bug caused by (feature/regressing bug #): 943521

User impact if declined: 
  Firefox Accounts and Sync won't work
  Sorrow and gnashing of teeth

Testing completed (on m-c, etc.):
  fx-team, m-i, and m-c
  Confirmed with manual testing as described in Comment 103

Risk to taking this patch (and alternatives if risky): 
  Risk is low; it's been in use with active QA for a month
  No alternatives.  The Firefox Accounts auth servers have all
  been updated to use the protocol this patch supports

String or IDL/UUID changes made by this patch: 
  none

The attachment whose title begins with [aurora rebase] is the one we want
rs=rnewman
Attachment #8391600 - Flags: approval-mozilla-aurora?
Attachment #8391600 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Jed, I hope you are ok that I pushed this for you. We have a hard time-line and I want to get my stuff landed today before the merge.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e85435556aaa
status-firefox29: affected → fixed
Hm, Jed any reason why the whole retry logic for SignIn() hasn't been implemented in the backport patch? I have seen this now while trying to get my patches to work.

https://hg.mozilla.org/mozilla-central/file/25cfa01ba054/services/fxaccounts/FxAccountsClient.jsm#l105
https://hg.mozilla.org/releases/mozilla-aurora/file/default/services/fxaccounts/FxAccountsClient.jsm#l103

Is that really wanted? I see loss of functionality here.
Flags: needinfo?(jparsons)
Also when you apply attachment 8387572 [details] [diff] [review] on top of your patch, it will fail with:

 "ASSERTION FAILED! Weave status OK; expected "success.status_ok", got "service.client_not_configured"

And this *after* the 'fxaccounts:onlogin' and 'weave:service:setup-complete' observer notifications have been sent. On m-c it is working fine. Do we still have a core bug in here?
Revoking tracking flag for now, given that a follow-up or back-out and new fix seem to be necessary.
status-firefox29: fixed → affected
As result the 'weave:engine:start-tracking' observer notification is not send, which we are waiting for in tps to start the tests.
So the problem here is that result.email is undefined (not returned by the server) and on m-c we set it to data.email. To keep the API definition we should get this updated. I have a patch in a bit for review.
This updates the signIn() method and makes it conform to our API as used on m-c. Tests for that change, and the jsdoc updates are included in my patch on bug 981921 which is waiting for this patch to be landed first.
Attachment #8392184 - Flags: review?(ckarlof)
(In reply to Henrik Skupin (:whimboo) from comment #106)
> Hm, Jed any reason why the whole retry logic for SignIn() hasn't been
> implemented in the backport patch? I have seen this now while trying to get
> my patches to work.
>
> https://hg.mozilla.org/mozilla-central/file/25cfa01ba054/services/fxaccounts/
> FxAccountsClient.jsm#l105
> https://hg.mozilla.org/releases/mozilla-aurora/file/default/services/
> fxaccounts/FxAccountsClient.jsm#l103

Yes, I asked :ckarlof about this in IRC on Friday, namely whether we should push for uplift of Bug 963835, which implements the retry logic on incorrect email case capitalization.  He said no, it's not needed for Firefox 29, adding that this patch really benefits FirefoxOS anyway.
Flags: needinfo?(jparsons)
(In reply to Henrik Skupin (:whimboo) from comment #110)
> So the problem here is that result.email is undefined (not returned by the
> server) and on m-c we set it to data.email. To keep the API definition we
> should get this updated. I have a patch in a bit for review.

I see
Ok, this implements only the updating piece for the email address, but leaves out the capitalization correction, which is too large to be backported at this stage. Please notice that the jsdoc will be updated with my other patch.
Attachment #8392184 - Attachment is obsolete: true
Attachment #8392184 - Flags: review?(ckarlof)
Attachment #8392334 - Flags: review?(jparsons)
Attachment #8392334 - Flags: feedback?(ckarlof)
Attachment #8392334 - Flags: feedback?(ckarlof) → feedback+
Comment on attachment 8392334 [details] [diff] [review]
[beta] Make signIn() method API conform v2

Looks good!

I also did manual testing on latest desktop aurora, just out of extra paranoia :)
Attachment #8392334 - Flags: review?(jparsons) → review+
Comment on attachment 8392334 [details] [diff] [review]
[beta] Make signIn() method API conform v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Code missed in last backport patch
User impact if declined: Only automation is affected
Testing completed (on m-c, etc.): yes, for a long time.
Risk to taking this patch (and alternatives if risky): None, fixes the API compared to m-c
String or IDL/UUID changes made by this patch: None
Attachment #8392334 - Flags: approval-mozilla-aurora?
Comment on attachment 8392334 [details] [diff] [review]
[beta] Make signIn() method API conform v2

[Triage Comment]
Post-merge, we're now needing this on Beta for 29.
Attachment #8392334 - Flags: approval-mozilla-beta+
Attachment #8392334 - Flags: approval-mozilla-aurora?
Attachment #8392334 - Flags: approval-mozilla-aurora-
Attachment #8392334 - Attachment description: [aurora] Make signIn() method API conform v2 → [beta] Make signIn() method API conform v2
I'm not around today, so could someone please land this on beta? Thanks.
Keywords: checkin-needed
Is any additional manual testing required here? If that is the case, could you please let me know if the steps specified in Comment 103 still represent a viable way of verifying the fix?
Flags: needinfo?(jparsons)
This code is in use in our tps framework to run the sync tests. All is working fine across branches. So I don't think we need manual testing here.
(In reply to Henrik Skupin (:whimboo) from comment #121)
> This code is in use in our tps framework to run the sync tests. All is
> working fine across branches. So I don't think we need manual testing here.
Thank you for following up on this, Henrik! Marking it as [qa-] based on Comment 121 - Jed, please flip this back if you disagree.
Whiteboard: [qa+] → [qa-]
Thanks, Andrei!
Flags: needinfo?(jparsons)
No longer depends on: 968567
(In reply to Jed Parsons (use needinfo, please) [:jedp, :jparsons] from comment #46)
> Oh - and all that pbkdf2 stuff happens on a separate thread now, per Comment
> 39

I'm looking at bug 1073639 and found myself here.  Note that there isn't a different thread here - Services.tm.currentThread.dispatch simply waits for an event-loop spin before running the function on the same thread.  AFAIK, the only way for JS to get a new thread is to use a worker.  The log message:

0:05.75 Identity.FxAccounts	DEBUG	Dispatched thread for credentials setup crypto work

is also misleading in this case.

It sounds like a new bug is warranted - but I'm not sure if it is "*really* use a thread" or "make the log message less misleading" :)  JedP, do you mind finding out what followup makes sense?
Flags: needinfo?(jed+bmo)
Hey Mark --

Jed has left the building. I have inherited responsibility for this code, and honestly I have no idea what the cross-platform implications of introducing a thread here are. My guess is that we should try it but it would be nice to get insight from someone who has handled memory- and CPU-intensive work on platform before. Any nominations?
Flags: needinfo?(jed+bmo)
Product: Core → Firefox
Target Milestone: mozilla30 → Firefox 30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: