753238 - Create a shared Identity.jsm module

Status: RESOLVED FIXED

(Core Graveyard :: Identity, enhancement)

Description

[Anant Narayanan [:anant]]

This shared JS module will implement several functions that will be needed by any code in Firefox dealing with Identity. This includes features like Sign in to the Browser, Apps in the Cloud, and other services that are planned in the future.

This module is the promised evolution of the hack in bug 745345. It will use code from the WIP patch in bug 664614 and depend on the crypto API being built in bug 665057.

Comment 1

[David Dahl :ddahl]

I am thinking we should not call this "DOMIdentity.jsm" as it will be consumed by non-DOM-specific (Sync, etc.) code. We should just drop the "DOM" prefix.

Comment 2

[David Dahl :ddahl]

Anant: perhaps I misunderstand what DOMIdentity.jsm is for as you have comments in there that talk about Identity.jsm as another module. We should connect and go through this.

Comment 3

[Anant Narayanan [:anant]]

We should definitely call this Identity.jsm. DOMIdentity.jsm is simply the child process for nsDOMIdentity.js which are both part of the thin DOM shim for navigator.id, and they're both being developed in bug 753239 rather than here.

I'm open to better names for either :)

Comment 4

[David Dahl :ddahl]

Attachment: Patch 1

Still need to test this against pine branch

Comment 5

[David Dahl :ddahl]

Attachment: DOM 2 Toolkit communication

Ben:

Take a look at this patch to see how we will communication between the DOM component and the Identity.jsm in toolkit

Comment 6

[Dietrich Ayala (:dietrich)]

Ben, is this code required to ship in B2G as well?

Comment 7

[Ben Adida [:benadida]]

Dietrich: a version of this code. It's gone through many iterations and a patch is coming soon. This is the code I demo'ed a couple of days ago.

Comment 8

[Dietrich Ayala (:dietrich)]

Thanks. If this bug is the tracking manifestation of the code that will eventually ship on a phone providing BrowserId functionality then this blocks. #flagflipped

Comment 10

[Ben Adida [:benadida]]

Attachment: WIP: Identity.jsm and supporting crypto

Comment 12

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

Comment on attachment 632457 [details] [diff] [review]
WIP: Identity.jsm and supporting crypto

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

::: toolkit/identity/IdentityCryptoService.cpp
@@ +94,5 @@
> +      destructorSafeDestroyNSSReference();
> +      shutdown(calledFromObject);
> +    }
> +
> +    void virtualDestroyNSSReference() MOZ_OVERRIDE {

brace needs to be on its own line.

@@ +100,5 @@
> +    }
> +
> +    void destructorSafeDestroyNSSReference()
> +    {
> +      if (isAlreadyShutDown()) // XXX: too many places

// XXX comment needs to be removed.

@@ +199,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    return NS_OK;
> +  }
> +private:

nix this line.

@@ +237,5 @@
> +KeyPair::GetHexRSAPublicKeyExponent(nsACString & result)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_ENSURE_TRUE(mPublicKey, NS_ERROR_NOT_AVAILABLE);
> +  NS_ENSURE_TRUE(mPublicKey->keyType == rsaKey, NS_ERROR_UNEXPECTED);

NS_ERROR_UNEXPECTED is not the best error code here; NS_ERROR_NOT_AVAILABLE would be better. (Note: this applies to all these accessors.)

@@ +346,5 @@
> +                                     PR_TRUE /*isSensitive*/,
> +                                     NULL /*&pwdata*/);
> +
> +  NS_ENSURE_TRUE(*privateKey, NS_ERROR_FAILURE); // XXX: PRErrorCode -> nsresult
> +  NS_ENSURE_TRUE(*privateKey, NS_ERROR_UNEXPECTED);

This line should be NS_ENSURE_TRUE(*publicKey, NS_ERROR_UNEXPECTED);

@@ +363,5 @@
> +  // XXX: These could probably be static const arrays, but this way we avoid
> +  // compiler warnings and also we avoid having to worry much about whether the
> +  // functions that take these inputs will (unexpectedly) modify them.
> +
> +  // Generated using openssl dsaparam -C 1024

This comment should be removed.

@@ +364,5 @@
> +  // compiler warnings and also we avoid having to worry much about whether the
> +  // functions that take these inputs will (unexpectedly) modify them.
> +
> +  // Generated using openssl dsaparam -C 1024
> +  // NOT ANYMORE: using NIST-based parameters

This comment should be clarified to say that these parameters come from NIST document XXX and that some Browser ID components external to Gecko require us to use these exact parameters.

@@ +380,5 @@
> +    0x2E,0xF7,0x17,0xFC,0x26,0xD0,0x2E,0x17
> +  };
> +
> +  /*
> +  PRUint8 P[] = {

These commented-out P, Q, G values should be removed.

@@ +452,5 @@
> +                                     PR_TRUE /*isSensitive*/,
> +                                     NULL /*&pwdata*/);
> +
> +  NS_ENSURE_TRUE(*privateKey, NS_ERROR_FAILURE); // XXX: PRErrorCode -> nsresult
> +  NS_ENSURE_TRUE(*publicKey, NS_ERROR_UNEXPECTED);

Add MOZ_ASSERT(*publicKey) like the above function.

@@ +463,5 @@
> +{
> +  if (!NS_IsMainThread()) {
> +    nsNSSShutDownPreventionLock locker;
> +    if (isAlreadyShutDown()) {
> +      mRv = NS_ERROR_ABORT;

Maybe NS_ERROR_NOT_AVAILABLE would be better?

@@ +523,5 @@
> +{
> +  if (!NS_IsMainThread()) {
> +    nsNSSShutDownPreventionLock locker;
> +    if (isAlreadyShutDown()) {
> +      mRv = NS_ERROR_ABORT;

Maybe NS_ERROR_NOT_AVAILABLE would be a better error code.

@@ +530,5 @@
> +      SECItem signature = { siBuffer, buffer, sizeof buffer };
> +
> +      switch (mPrivateKey->keyType) {
> +      case rsaKey:
> +        // XXX: is this in the right format

This is not in the right format, because it is DER-encoded. I think we need to use PK11_Sign directly like we do for the DSA case, and/or remove the RSA code since it isn't used by anything.

@@ +578,5 @@
> +// XPCOM module registration
> +
> +namespace mozilla {
> +
> +NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(IdentityCryptoService, Init)

I remember that I got linker errors when I put this in the unnamed namespace, but maybe I was doing something wrong. We should try it in the unnamed namespace, and if that doesn't work, then we should add a comment explaining why this must be in a named namespace.

@@ +584,5 @@
> +} // namespace mozilla
> +
> +namespace {
> +
> +

Remove extra blank line.

Comment 13

[Ben Adida [:benadida]]

bsmith(In reply to Brian Smith (:bsmith) from comment #12)
> Comment on attachment 632457 [details] [diff] [review]

would you like me to own and modify this code now, or would you like to take a whack at it?

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 632457 [details] [diff] [review]
WIP: Identity.jsm and supporting crypto

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

Posting partial-review comments... I've read through everything once (in varying detail). I mostly just skimmed IdentityCryptoService.cpp, will definitely need to do another pass on it. Nothing much jumped out at me in Identity.jsm, but I'll do a closer look next. But, overall, this is looking very very nice.

There's enough here so far that an updated patch would be helpful, I'll move on to other patches that are up for review in the meantime.

Since this is the first time through review for some of you... My review comments fall on a spectrum from "must fix" to "just a suggestion". But no matter where a particular comment falls, do feel free to push back (ideally with explanation :) if I've asked for something wrong, dumb, or with a terrible effort-to-benefit ratio.

::: toolkit/identity/Identity.jsm
@@ +36,5 @@
> + *
> + * Enable with about:config pref toolkit.identity.debug
> + */
> +function log(args) {
> +  if (! IdentityService._debugMode) return;

This is super-useful to have, and I'd highly encourage adding sufficient logging throughout.

https://wiki.mozilla.org/Firefox:Password_Manager_Debugging has been a huge help over the years, and lets us help diagnose user problems remotely even if they're not very tech-savvy.

@@ +55,5 @@
> +        strings.push("<<something>>");
> +      }
> +    }
> +  });
> +  dump("@@ Identity.jsm: " + strings.join(' ') + "\n");

I'd suggest adding a Services.console.logStringMessage() in addition to the dump, so that output is visible in the Error Console.

@@ +61,5 @@
> +
> +// the data store for IDService
> +// written as a separate thing so it can easily be mocked
> +function IDServiceStore() {
> +  this.reset();

Seems like .reset() should really just be named .init()? That's a more common pattern.

@@ +113,5 @@
> +  },
> +
> +  reset: function reset() {
> +    this._identities = {};
> +    this._loginStates = {};

Might consider adding null properties for _identities and _loginStates in the prototype, just to help make it obvious that they're an important part of the object.

@@ +126,5 @@
> +  Services.prefs.addObserver(PREF_DEBUG, this, false);
> +
> +  this._debugMode = Services.prefs.getBoolPref(PREF_DEBUG);
> +
> +  this.reset();

Also better named as .init()?

@@ +1045,5 @@
> +      case "nsPref:changed":
> +        this._debugMode = Services.prefs.getBoolPref(PREF_DEBUG);
> +        break;
> +    }
> +  }.bind(this),

Doesn't seem like the .bind() should be needed here.

::: toolkit/identity/IdentityCryptoService.cpp
@@ +23,5 @@
> +
> +namespace {
> +
> +void
> +HexEncode(const SECItem * it, nsACString & result)

Do we really not have some existing impl of this in the tree?

@@ +36,5 @@
> +  }
> +}
> +
> +// Stolen from /toolkit/components/places/Helpers.cpp
> +// TODO: use the new base64 encoder khuey wrote

Ditto -- yes please. Multiple implementations lead to multiple bugs. :/

::: toolkit/identity/jwcrypto.jsm
@@ +1,5 @@
> +/* -*- Mode: js2; js2-basic-offset: 2; indent-tabs-mode: nil; -*- */
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */
> +/* 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/. */

Is this actually Mozilla-originated code?

For some reason I'm thinking it's 3rd party. Maybe I'm thinking of jwplayer.

OOC, what's the "jw" indicate here?

@@ +47,5 @@
> +  // private property
> +  _keyStr : "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=",
> +  
> +  // public method for encoding
> +  encode : function (input) {

Isn't this just atob/btoa? I kinda hate to have yet another implementation of these in the tree.

@@ +118,5 @@
> +    
> +  },
> +  
> +  // private method for UTF-8 encoding
> +  _utf8_encode : function (string) {

This almost certainly should use nsIScriptableUnicodeConverter... See encrypt/decrypt in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/crypto-SDR.js

@@ +175,5 @@
> +    return string;
> +  }
> +};
> +
> +function base64urlencode(arg) {

encodeURI() / encodeURIComponent()?

See https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/encodeURI

@@ +317,5 @@
> +    var headerBytes = base64urlencode(JSON.stringify(header));
> +    
> +    var payload = {
> +      // expires in 2 minutes
> +      exp: new Date(new Date().valueOf() + (2 * 60 * 1000)).valueOf(),

This would be more readable with Date.now() instead of |new Date()|...

  exp: Date.now() + (2 * 60 * 1000);

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +2,5 @@
> + * 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/. */
> +
> +#include "nsISupports.idl"
> +#include "nsIURI.idl"

This should just be "interface nsIURI;"

@@ +11,5 @@
> +
> +%{ C++
> +#define NS_IDENTITYCRYPTOSERVICE_CONTRACTID \
> +        "@mozilla.org/identity/crypto-service;1"
> +%}

I'd suggest just getting rid of the #define, and directly using the actual string instead. A bit less indirect, and since JS uses the string anyway the #defines don't add much. Older Mozilla code is littered with these, I've been slowing them if I'm touching the IDL anyway.

@@ +36,5 @@
> + *
> + * "DS160": DSA with SHA-1. A 1024-bit prime and a 160-bit subprime with SHA-1.
> + */
> +
> +[scriptable, uuid(984f31ba-eaef-4824-b763-9637f87c1956)]

I presume all the uuids in this .idl were randomly generated, and not cut'n'pasted from elsewhere? (Hopefully this sounds like a dumb question, but it's happened. ;)

@@ +73,5 @@
> +/**
> + * This interface provides a JavaScript callback object used to collect the 
> + * nsIIdentityServeKeyPair when the keygen operation is complete 
> + */
> +[scriptable, uuid(f38100a5-94f1-415c-ac05-7e8351345dec)]

You probably want the "function" keyword here. This allows JS code to just pass in a plain ol' JS Function, instead of an object with XPCOM goop and a function named "generateKeyPairFinished". 

See https://developer.mozilla.org/en/XPIDL/Function_modifier

@@ +77,5 @@
> +[scriptable, uuid(f38100a5-94f1-415c-ac05-7e8351345dec)]
> +interface nsIIdentityKeyGenCallback : nsISupports
> +{
> +  void generateKeyPairFinished(in nsresult rv,
> +                               in nsIIdentityKeyPair keyPair);

An nsresult as an |in|?!

This should probably just be "void genKeyPairBlahblah(in nsIIDKP keyPair)"

@@ +84,5 @@
> +/**
> + * This interface provides a JavaScript callback object used to collect the 
> + * AUTF8String signature 
> + */
> +[scriptable, uuid(52992987-2725-4f71-94f4-ec40a141388b)]

Ditto, "function" keyword here.

@@ +94,5 @@
> +   *
> +   * For DSA128 signatures, the signature is the r value concatenated with the
> +   * s value, each component padded with leading zeroes as necessary.
> +   */
> +  void signFinished(in nsresult rv, in ACString base64urlSignature);

Ditto, remove "in nsresult rv".

::: toolkit/identity/tests/unit/head_global.js
@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +let XULAppInfo = {

Is this and the XULAppInfoFactory goop actually needed, or just cut'n'paste flotsam?

xpcshell provides a do_get_profile() now, which might be better if you actually need a profile dir to poke at?

See https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests

@@ +47,5 @@
> +  dump("ID Tests: " + aMsg + "\n");
> +}
> +
> +// Switch debug messages on by default
> +Services.prefs.setBoolPref("toolkit.identity.debug", true);

Not a big deal here, but you probably should use do_register_cleanup() to ensure the pref is reset at the end of the test.

See https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests

::: toolkit/identity/tests/unit/tail_global.js
@@ +2,5 @@
> +// pre-emptively shut down to clear resources
> +if (typeof IdentityService !== "undefined")
> +  IdentityService.shutdown();
> +if (typeof IDService !== "undefined")
> +  IDService.shutdown();

These might also be good as do_register_cleanup() callbacks, to make sure each xpcshell test gets a clean environment.

::: toolkit/identity/tests/unit/test_crypto_service.js
@@ +6,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const idService = Cc["@mozilla.org/identity/crypto-service;1"]
> +                    .getService(Ci.nsIIdentityCryptoService);

Nit: usual style for Cc+.getService calls is to format as:

var foo = Cc["blahblah"].
          getService(Ci.blah);

But it's pretty loosely adhered to, and not that big of a deal. So just FYI. :)

@@ +22,5 @@
> +        iid.equals(Components.interfaces.nsISupports))
> +      return this;
> +
> +    throw Components.results.NS_ERROR_NO_INTERFACE;
> +  },

Preferable to use:

  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports, ...]),

But I think the |function| keyword additions I suggested in the .idl comments will make having a QI completely unneeded.

::: toolkit/identity/tests/unit/test_identity_jsm.js
@@ +3,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +// delay the loading of the IDService for performance purposes

These are just tests, so not much win from lazy loading. Fine either way.

@@ +50,5 @@
> +  dump("@@ test_identity_jsm: " + strings.join(' ') + "\n");
> +}
> +
> +const TEST_URL = "https://myfavoritebacon.com";
> +const TEST_URL2 = "https://myfavoritebaconinacan.com";

Bacon in a can is a fantastic way to store bacon. Although I prefer the plastic vacuum-packed packages for convenience-bacon.

::: toolkit/identity/tests/unit/test_jwcrypto_jsm.js
@@ +1,3 @@
> +"use strict"
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

The strict-mode invocation can be placed after comments.

::: toolkit/identity/tests/unit/xpcshell.ini
@@ +3,5 @@
> +tail = tail_global.js
> +
> +# Test load modules first so syntax failures are caught early.
> +[test_crypto_service.js]
> +[test_load_modules.js]

Comment does not match reality? :)

Comment 15

[Ben Adida [:benadida]]

Justin: most of your comments sound great, we'll move on fixing them right away.

Questions:

- regarding encoders: where can we access stable base64/hex encoders, ideally not tied to a window object?

> ::: toolkit/identity/jwcrypto.jsm
> 
> Is this actually Mozilla-originated code?

The Base64 encoding isn't, the rest is. Where should we get base64 encoding from? Happy to reuse!

> OOC, what's the "jw" indicate here?

derived from JWT/JWS specs, which stand for JSON Web Token / Signature, so this is JSON Web Crypto.


> > +function base64urlencode(arg) {
> 
> encodeURI() / encodeURIComponent()?

That's actually different, it's a type of b64 encoding that we have to comply to as per JWT/JWS IETF spec. Thankfully, the implementation is just a tiny bit of code on top of normal base64 encoding, so once we know how to do that right, this is not much more code.

> I presume all the uuids in this .idl were randomly generated, and not
> cut'n'pasted from elsewhere? (Hopefully this sounds like a dumb question,
> but it's happened. ;)

Let's regenerate all of them to make sure. Jed, can you take that on?

> Is this and the XULAppInfoFactory goop actually needed, or just cut'n'paste
> flotsam?
> 
> xpcshell provides a do_get_profile() now, which might be better if you
> actually need a profile dir to poke at?

Nice, we'll try that. Yes, it was cut-and-paste.

> Bacon in a can is a fantastic way to store bacon. Although I prefer the
> plastic vacuum-packed packages for convenience-bacon.

what about turkey bacon? /me ducks.

Comment 16

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Justin: thank you, yes this is my first time through this process.  I appreciate the clarity of your comments.

Ben: I'm working on a new patch right now

Is sounds like we should consider making a pref for bacon ...

Comment 17

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

(In reply to Ben Adida [:benadida] from comment #13)
> bsmith(In reply to Brian Smith (:bsmith) from comment #12)
> > Comment on attachment 632457 [details] [diff] [review]
> 
> would you like me to own and modify this code now, or would you like to take
> a whack at it?

I will do (have done, mostly) so.

Comment 18

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

Comment on attachment 632457 [details] [diff] [review]
WIP: Identity.jsm and supporting crypto

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

::: toolkit/identity/Identity.jsm
@@ +634,5 @@
> +      return provFlow.callback("Cannot genKeyPair before beginProvisioning");
> +    }
> +
> +    // Ok generate a keypair
> +    jwcrypto.generateKeyPair(jwcrypto.ALGORITHMS.DS160, function(err, kp) {

Why did we switch back to jwcrypto from the native crypto service? Is this really an intentional change, or did the splitting of Identity.jsm and jwcrypto.jsm get messed up?

Comment 19

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Brian Smith (:bsmith) from comment #18)

> Why did we switch back to jwcrypto from the native crypto service? Is this
> really an intentional change, or did the splitting of Identity.jsm and
> jwcrypto.jsm get messed up?

It actually is using the native crypto.  jwcrypto.jsm is now basically an adapter that's wrapped around the native interface and callbacks.

Comment 20

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: v2 updates to Identity.jsm and crypto after dolske's first comments

Comment 21

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

Attachment: Do the base64-url-encoding in native code

Here is a patch that re-uses the native-code base64-URL-encoder.

We could do something very similar for the base64-URL-decoder, but since it isn't actually used, I didn't bother.

I did not test this very well. Also, I am not quite sure how the automatic Javascript<->Native Code UTF-8 encoding/decoding works when you use the AUTF8String XPIDL type; I suppose you must get some kind of exception in the Javascript code if the encoded data isn't valid UTF-8.

Comment 22

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

(In reply to Jed Parsons from comment #20)
> Created attachment 633415 [details] [diff] [review]
> v2 updates to Identity.jsm and crypto after dolske's first comments

Thanks Jed. FYI, I did add the [function] keyword to the XPCOM interfaces so that you can change jwcrypto to pass functions to generateKeyPair and sign, instead of doing the XPCOM goop. I did not change jwcrypto.jsm to do that because of that misunderstanding. Feel free to do so if you think it makes the code clearer.

Comment 23

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Brian Smith (:bsmith) from comment #22)

> FYI, I did add the [function] keyword to the XPCOM interfaces 

Awesomesauce.  Thank you, I've updated jwcrypto.  And I've also taken the liberty of pushing your last patch into our pine branch before it (commit 9297f0c3f0d1).

Comment 24

[Matthew N. [:MattN]]

Attachment: v.2.1 address review comments and include native base64 code

Latest code from pine for review.

Comment 25

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

Comment on attachment 634692 [details] [diff] [review]
v.2.1 address review comments and include native base64 code

Bob, could you please look at IdentityCryptoService.cpp and review the use of the NSS (pk11wrap) functions? This is what I was asking you about in last week's NSS teleconference.

This code must generate RSA 2048 keypairs and DSA 1024 keypairs, and it must generate RSA2048-SHA256 and DSA1024-SHA1 signatures.

The reason for the small DSA keys is that we're trying to be compatible with the DSA implementation in JavaScript, which is too slow when we use larger keys on devices like iPhones. Also, these signatures are only valid for a few hours (6 hours, I think), so it is extremely unlikely they will be broken by the time it expires.

Comment 26

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

Comment on attachment 634692 [details] [diff] [review]
v.2.1 address review comments and include native base64 code

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

I just noticed that this patch did not include the change to security/manager/ssl/src/Makefile.in that exports the nsNSSShutdown.h header from PSM so that IdentityCryptoService can use it. I filed bug 766683 for that change and moved Kai's review request to that bug.

Comment 27

[Robert Relyea]

Hi brian, I may not get to this today, but I should get to it by tomorrow.

I did have a comment on your DSA logic. I don't have a huge headache supporting 1024 bit DSA for now, but I wouldn't depend on the idea that the keys and signatures are only good for 6 hours. The security of DSA is based on the difficulty of calculating the discrete logarithms over prime fields, in particular calculating log_g(Y) mod p to get x. Our best algorithms usually have 2 steps, creating a factor base with known discrete logs for small primes (log_g(2), log_g(3), etc.),then finding g^2*Y that can be factored with these small factors. The first step is the most expensive, and is independent of Y, and therefore your key pair. What is being attacked is really your choice of p and g (and primarily the former, once you've broken p for and g, you can break it for any other g by using the identity log_g1 (Y) = log_g(Y)/log_g(g1)).

Anyway my upshot is that the choice of 1024 bits for your DSA keys isn't any more secure because the key and signature is only good for 6 hours. All is not lost, however. If you are willing to change out your pqg parameters periodically (every 6 months or so), you are probably still reasonably safe as long as you aren't trying to protect against large governments. (If you then I wouldn't trust the 6 hour number).

bob

Comment 28

[Robert Relyea]

Comment on attachment 634692 [details] [diff] [review]
v.2.1 address review comments and include native base64 code

r+ for the C C++ portions. except the XPCOM bindings. The excluded portions are not because of bugs, only because I did not review them.

It looks the the DSA issue is bigger than I first thought. If you are tied to the given parameters, the you can't change them to mitigate against any attacks.

Also, are the public keys validated in any way? That is would your system break if some one sent 'cooked' PQG parameters or a 'cooked' DSA key on the wire?

bob

Comment 29

[Robert Relyea]

Actually, the cooked case is not an issue since you are only verifying a signature (you already have to be strong against someone replacing a key). You are only generating your key from your own set of presumably trusted pqg parameters.

bob

Comment 30

[Ben Adida [:benadida]]

(In reply to Robert Relyea from comment #28)
> It looks the the DSA issue is bigger than I first thought. If you are tied
> to the given parameters, the you can't change them to mitigate against any
> attacks.

Hi Bob, thanks for these comments.

I'm not too worried about the crypto attack because it seems to me the task of finding the discrete log for a particular user is still going to be extremely expensive at 1024 bit. Do you have data indicating otherwise?

> Also, are the public keys validated in any way? That is would your system
> break if some one sent 'cooked' PQG parameters or a 'cooked' DSA key on the
> wire?

Right now the parameters are hard-wired. Our next step would be for the parameters to be dynamically generated locally. In both cases I believe this gets around the concerns of cooked PQG parameters.

Comment 31

[Ben Adida [:benadida]]

(In reply to Robert Relyea from comment #29)
> Actually, the cooked case is not an issue since you are only verifying a
> signature (you already have to be strong against someone replacing a key).
> You are only generating your key from your own set of presumably trusted pqg
> parameters.


Yes, exactly, sorry I missed this comment earlier.

Thanks for the thorough review!

Comment 32

[Robert Relyea]

> I'm not too worried about the crypto attack because it seems to me the task of finding the
> discrete log for a particular user is still going to be extremely expensive at 1024 bit. Do you
> have data indicating otherwise?

The tools for finding discrete logs are very similar to the tools for factoring large primes. 1024 is currently considered "weak", though there have been no published attacks to date.

Basically consider the strength of your PQG's as if they were a single 1024 bit RSA key. If you are worried about attacks from major governments, I wouldn't trust 1024 bit. If you are worried about "normal" players, corporations or hackers with bot nets, it's probably somewhat safe, certainly if you rotate them every 6 months to a year. I don't know what the target is for this product, so I can't advise you on whether there's a problem with your choice or not.

My point, however, is that the DSA's security is based on PQG. Once someone has done the hard work of calculating log_g {list of small primes}, PQG is no longer secure (actually P and Q are not longer secure, a new G can be broken in the same time it takes to break a key). You can't, therefore, depend on the sort life of Y and the signature for security, but you need to keep the life time of PQG low as well (generating on the fly would solve this issue).

bob

Comment 33

[Justin Dolske [:Dolske]]

Comment on attachment 634692 [details] [diff] [review]
v.2.1 address review comments and include native base64 code

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

Gah. Sorry this has taken so long to complete. I've a bunch of review comments from going through this closely, but they're all relatively small issues that should be fixable easily.

A couple general comments:

* For all "XXX" and "TODO" type comments, please either resolve them or file bugs so they can become "XXX bug 12345: need kittens here" comments.

* Was any review (from me) of IdentityCryptoService.cpp still needed? I didn't really look at it since rrelya did.

::: toolkit/identity/Identity.jsm
@@ +76,5 @@
> +  init: function init() {
> +    XPCOMUtils.defineLazyModuleGetter(this,
> +                                      "_store",
> +                                      "resource://gre/modules/identity/IdentityStore.jsm",
> +                                      "IdentityStore");

No point in using lazy getters when they're immediately called. Just Cu.import().

IDService seems to be a singleton. Any reason to store them as properties instead of just using them as globals for the module?

@@ +95,5 @@
> +    // Clear RP state
> +    this.RP.init();
> +
> +    // Clear IDP state
> +    this.IDP.init();

RAII would seem preferable here, unless there's a need to explicitly call init() separately/again elsewhere.

@@ +124,5 @@
> +  shutdown: function shutdown() {
> +    log("shutdown");
> +    this.init();
> +    try {
> +      Services.obs.removeObserver(this, "nsPref:changed");

This ought to be Services.prefs.removeObserer().

@@ +153,5 @@
> +   *
> +   * @param aIdentity
> +   *        (string) the email chosen for login
> +   */
> +  selectIdentity: function selectIdentity(aRPId, aIdentity) {

This has a lot of (nested) anonymous callbacks, I'd suggest naming each so that debugging is easier when looking at call stacks.

@@ +168,5 @@
> +    }
> +
> +    // It's possible that we are in the process of provisioning an
> +    // identity.
> +    let provId = rp.provId || null;

This seems to be the only place that's checking for a null, is it really needed? (Alternatively, seems like it should be initialized/set to null by underlying code when it's... undefined?)

@@ +170,5 @@
> +    // It's possible that we are in the process of provisioning an
> +    // identity.
> +    let provId = rp.provId || null;
> +
> +    // XXX consolidate rp, flows, etc. ?

Not sure what this means.

@@ +179,5 @@
> +    log("selectIdentity: provId:", provId, "origin:", rp.origin);
> +
> +    // Once we have a cert, and once the user is authenticated with the
> +    // IdP, we can generate an assertion and deliver it to the doc.
> +    self.RP._generateAssertion(rp.origin, aIdentity, function(err, assertion) {

nit: this.RP, and might suggest moving the |let self=this| down to here since it's the first use.

@@ +184,5 @@
> +      if (!err && assertion) {
> +        self.RP._doLogin(rp, rpLoginOptions, assertion);
> +        return;
> +
> +      } else {

Please avoid else-after-return, all it gives you is more indenting. :(

@@ +192,5 @@
> +          if (err) {
> +            rp.doError(err);
> +            return;
> +          }
> +

See, you avoided it right here. ;-)

@@ +206,5 @@
> +
> +            // At this point, we already have a cert.  If the user is also
> +            // already authenticated with the IdP, then we can try again
> +            // to generate an assertion and login.
> +            if (!err) {

Sometimes you're checking |if (err)|, other times |if (!err)|, etc. Would be nice to be consistent.

@@ +209,5 @@
> +            // to generate an assertion and login.
> +            if (!err) {
> +              // XXX quick hack - cleanup is done by registerCertificate
> +              // XXX order of callbacks and signals is a little tricky
> +              //self._cleanUpProvisionFlow(aProvId);

I don't understand these comments.

@@ +262,5 @@
> +   *        (function) callback to invoke on completion
> +   *                   with first-positional parameter the error.
> +   */
> +  _discoverIdentityProvider: function _discoverIdentityProvider(aIdentity, aCallback) {
> +    let domain = aIdentity.split('@')[1];

Should have a check here to ensure the identity has a sane (user@host) format.

We should explicitly fail early on for a user fatfingering (or deliberately sniffing around with) weird input like "me@", "@host", "me@foo@bar", etc.

Ditto for malformed domains -- "user@gmail" or "[e-mail]/accounts/resetpassword.html?".

May just want a spinoff bug to explicitly go through and add defensive checks for all non-trusted callers.

@@ +287,5 @@
> +   *
> +   * @param aScheme
> +   *        (string) (optional) Protocol to use.  Default is https.
> +   *                 This is necessary because we are unable to test
> +   *                 https.

Hmm? The test frameworks support https requests.

@@ +296,5 @@
> +  _fetchWellKnownFile: function _fetchWellKnownFile(aDomain, aScheme, aCallback) {
> +    if (arguments.length <= 2) {
> +      aCallback = aScheme;
> +      aScheme = "https";
> +    }

If you need to keep the optional arg, make it the last arg to avoid this magic switcharoo.

Could then just do: aScheme = aScheme || "https"

@@ +311,5 @@
> +    req.responseType = "json";
> +    req.mozBackgroundRequest = true;
> +    req.onload = function _fetchWellKnownFile_onload() {
> +      if (req.status < 200 || req.status >= 400)
> +        return aCallback(req.status);

We shouldn't expose the status to the callback. Doesn't seem used, and is bad form to expose it to untrusted content because it leaks info about what resources exist. Just log() it.

@@ +316,5 @@
> +      try {
> +        let idpParams = req.response;
> +
> +        // Verify that the IdP returned a valid configuration
> +        if (! (idpParams.provisioning &&

I wonder a bit if there should be more verification here, but it seems reasonable as-is.

@@ +331,5 @@
> +        };
> +        // Yay.  Valid IdP configuration for the domain.
> +        return aCallback(null, callbackObj);
> +
> +      } catch (err) {

Consider just throwing exceptions within the try-block instead of explicit error-returns... That way real exceptions are already well-tested by going through a normal path. :)

@@ +347,5 @@
> +      if (req.statusText) {
> +        err += " " + req.statusText;
> +      }
> +      log("ERROR: _fetchWellKnownFile:", err);
> +      return aCallback(err);

Ditto about not exposing the error status.

::: toolkit/identity/IdentityProvider.jsm
@@ +108,5 @@
> +    }
> +  },
> +
> +  shutdown: function RP_shutdown() {
> +    this.init();

Why are you initing for shutdown?

@@ +110,5 @@
> +
> +  shutdown: function RP_shutdown() {
> +    this.init();
> +    try {
> +      Services.obs.removeObserver(this, "nsPref:changed");

try/catch shouldn't be needed here, as noted elsewhere you want Services.prefs.removeObserver.

Might be followup worthy to look at unifying this stuff, seems like overkill to have each submodule doing all this init/observing/shutdown stuff.

@@ +115,5 @@
> +    } catch (ex) {}
> +  },
> +
> +  get securityLevel() {
> +    return 1;

Is this documented somewhere? Add a comment pointing there?

@@ +208,5 @@
> +    // Determine recommended length of cert.
> +    let duration = this.certDuration;
> +    if (!duration) {
> +      throw "invalid duration";
> +    }

The certDuration getter seems infallible, is this check really needed? Shouldn't it call aCaller.doError() if so?

@@ +229,5 @@
> +   * @param aReason
> +   */
> +  raiseProvisioningFailure: function raiseProvisioningFailure(aProvId, aReason) {
> +    log("ERROR: Provisioning failure:", aReason);
> +    Cu.reportError("Provisioning failure: " + aReason);

Bonus points for checking that all .reportError() calls mention "BrowserID: ..." so that there's some context for the error console line showing all this.

@@ +234,5 @@
> +
> +    // look up the provisioning caller and its callback
> +    let provFlow = this._provisionFlows[aProvId];
> +    if (!provFlow) {
> +      let errStr = "No provisioning flow found with id:" + aProvId;

This pattern happens a lot -- maybe implement a getProvFlow(id) method that throws+logs when the flow for the ID does not exist?

@@ +335,5 @@
> +
> +    // store the keypair and certificate just provided in IDStore.
> +    this._store.addIdentity(provFlow.identity, provFlow.kp, aCert);
> +
> +    // Great success!

Is nice!

@@ +439,5 @@
> +    let subject = {
> +      rpId: provFlow.rpId,
> +      identity: provFlow.identity,
> +    };
> +    Services.obs.notifyObservers({ wrappedJSObject: subject }, "identity-auth-complete", aAuthId);

Huh, I guess the wrappedJSObject bit is actually needed here? Curious.

::: toolkit/identity/IdentityStore.jsm
@@ +27,5 @@
> +
> +// _loginStates will associate remote origins with a login status and
> +// the email the user has chosen as his or her identity when logging
> +// into that origin.
> +IDServiceStore.prototype._loginStates = null;

Could just move these comments into init().

::: toolkit/identity/jwcrypto.jsm
@@ +15,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const IdentityCryptoService
> +  = Cc["@mozilla.org/identity/crypto-service;1"]
> +      .getService(Ci.nsIIdentityCryptoService);

XPCOMUtils.defineLazyServiceGetter()

@@ +45,5 @@
> +  // Additionally, make the output visible in the Error Console
> +  Services.console.logStringMessage(output);
> +}
> +
> +function keygenerator() {}

For keygenerator() and signer(), there doesn't seem to be any reason to use object protos or |new Foo()|... They're not holdinging any state, other than what closures will provide.

I'd suggest simplifying these. eg
var keygenerator = {
  genKeyPair : function() { ... },
  _genKPFinished : function() { ... },
}

Or even just make them all global functions, and skip the object methodification.

@@ +99,5 @@
> +
> +signer.prototype = {
> +  sign: function(aPayload, aKeypair, aCallback) {
> +    this.payload = aPayload;
> +    this.callback = aCallback;

These could just be simple closures. In fact the anonymous callback can already just refer to aPayload / aCallback, so they're entirely unneeded.

@@ +117,5 @@
> +}
> +
> +jwcryptoClass.prototype = {
> +  isCertValid: function(aCert, aCallback) {
> +    // XXX check expiration

Yeah, definitely gonna need a followup bug here. :D

@@ +128,5 @@
> +    the_keygenerator.generateKeyPair(aAlgorithmName, aCallback);
> +  },
> +
> +  generateAssertion: function(aCert, aKeyPair, aAudience, aCallback) {
> +    // for now, we hack the algorithm name

Need a followup bug here too.

@@ +134,5 @@
> +    var headerBytes = IdentityCryptoService.base64UrlEncode(
> +                          JSON.stringify(header));
> +
> +    var payload = {
> +      // expires in 2 minutes

Was there ever a conclusion to dealing with clock-skew between client and server? Followup bug?

::: toolkit/identity/nsIIdentityCryptoService.idl
@@ +6,5 @@
> +
> +interface nsIURI;
> +interface nsIIdentityKeyGenCallback;
> +interface nsIIdentityKeyPair;
> +interface nsIIdentitySignCallback;

These 3 are defined in this file, so these lines shouldn't be needed.

@@ +13,5 @@
> + *
> + * A"hex" prefix means "hex-encoded string representation of a byte sequence"
> + *
> + * A "base64url" prefix means "base-64-URL-encoded string repressentation of a
> + * byte sequence. XXX: what about the base64 padding?

A brief example of each might be useful? Along with a more precise definition, I can't tell just from these descriptions what either really means.

@@ +28,5 @@
> + *
> + * "RS256": RSA + SHA-256.
> + *
> + * "DS160": DSA with SHA-1. A 1024-bit prime and a 160-bit subprime with SHA-1.
> + */

I'd strongly consider being verbose here. EG "RSA-SHA256" is only a few extra bytes, and is much more self-explanitory.

@@ +62,5 @@
> +
> +  void sign(in AUTF8String aText,
> +            in nsIIdentitySignCallback callback);
> +
> +  // AUTF8String verify(in AUTF8String aSignature, in AUTF8String encodedPublicKey);

This needs to be removed, implemented, or marked as "// XXX Bug 12345 implement foo".

@@ +74,5 @@
> +[scriptable, function, uuid(90f24ca2-2b05-4ca9-8aec-89d38e2f905a)]
> +interface nsIIdentityKeyGenCallback : nsISupports
> +{
> +  void generateKeyPairFinished(in nsresult rv,
> +                               in nsIIdentityKeyPair keyPair);

Ok, now I see why you're using "in nsresult" here.

It doesn't really seem useful, as nothing cares about the specific type of success/failure. It also leads to confusing code like:

  function callback(rv, signature) {
    if (!Components.isSuccessCode(rv)) {
      return aCallback("Sign failed");
    }
    return aCallback(null, signature);
  }

where aCallback repeats that pattern yet again.


It would be a cleaner API (and code) to remove this argument, and do one of:
  (1) return a null |keyPair| upon failure
  (2) return a keyPair of type "error" with an error code property
  (3) throw an exception.

1 seems preferable to me.

@@ +86,5 @@
> +interface nsIIdentitySignCallback : nsISupports
> +{
> +  /** On success, base64urlSignature is the base-64-URL-encoded signature
> +   *
> +   * For RS256 signatures, XXXX.

...yes?

@@ +91,5 @@
> +   *
> +   * For DSA128 signatures, the signature is the r value concatenated with the
> +   * s value, each component padded with leading zeroes as necessary.
> +   */
> +  void signFinished(in nsresult rv, in ACString base64urlSignature);

Ditto as generateKeyPairFinished().

Comment 34

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

(In reply to Justin Dolske [:Dolske] from comment #33)
> * Was any review (from me) of IdentityCryptoService.cpp still needed? I
> didn't really look at it since rrelya did.

Yes, please re-review it. I could have sworn that I responded to your review comments from comment 14 with questions but I cannot see my comment above, so I guess it is lost in the ether. (Actually, I think that discussion may have happened on IRC). Here are my questions:

0. Do my XPCOM methods that take AUTF8Strings as parameters do the correct thing as far as error handling goes with values that aren't really UTF-8? I assumed that when xpconnect translates an AUTF8String across a boundary, it verifies that the UTF-8 is valid and throws an exception if not.

1. You recommended that we re-use some existing HexEncode function and base64-URL-encoding. I couldn't find an existing one and I asked on IRC with negative results. I agree it would be a good idea to have a standard version of each function in xpcom/ but I think we should do that in a follow-up bug. I filed bug 769519 and bug 769521 for this.

> @@ +287,5 @@
> > +   *
> > +   * @param aScheme
> > +   *        (string) (optional) Protocol to use.  Default is https.
> > +   *                 This is necessary because we are unable to test
> > +   *                 https.
> 
> Hmm? The test frameworks support https requests.

xpcshell doesn't have proper HTTPS support yet. See bug 466524.

> Might be followup worthy to look at unifying this stuff, seems like overkill
> to have each submodule doing all this init/observing/shutdown stuff.

+1

> > +signer.prototype = {
> > +  sign: function(aPayload, aKeypair, aCallback) {
> > +    this.payload = aPayload;
> > +    this.callback = aCallback;
> 
> These could just be simple closures. In fact the anonymous callback can
> already just refer to aPayload / aCallback, so they're entirely unneeded.

+1. This is a result of an incomplete refactoring after the [function] keyword was added to the callback interfaces.

> @@ +28,5 @@
> > + *
> > + * "RS256": RSA + SHA-256.
> > + *
> > + * "DS160": DSA with SHA-1. A 1024-bit prime and a 160-bit subprime with SHA-1.
> > + */
> 
> I'd strongly consider being verbose here. EG "RSA-SHA256" is only a few
> extra bytes, and is much more self-explanitory.

These names actually come from the JavaScript Web Token specification. I hate them too.

> > +  void sign(in AUTF8String aText,
> > +            in nsIIdentitySignCallback callback);
> > +
> > +  // AUTF8String verify(in AUTF8String aSignature, in AUTF8String encodedPublicKey);
> 
> This needs to be removed, implemented, or marked as "// XXX Bug 12345
> implement foo".

Up to Ben which path he wants to choose, though I highly advise against "implemented" unless someone not named Brian Smith is going to do it Friday.

> Ok, now I see why you're using "in nsresult" here.
> 
> It doesn't really seem useful, as nothing cares about the specific type of
> success/failure.

I'd rather leave it up to the JS code to decide whether it wants to inspect the return value and/or log it or whatever; the people that maintain the Identity module are generally JS hackers so I tried to optimize this to give them flexibility without them having to deal with the C++ code too much.

> It also leads to confusing code like:
> 
>   function callback(rv, signature) {
>     if (!Components.isSuccessCode(rv)) {
>       return aCallback("Sign failed");
>     }
>     return aCallback(null, signature);
>   }

> It would be a cleaner API (and code) to remove this argument, and do one of:
>   (1) return a null |keyPair| upon failure
>   (2) return a keyPair of type "error" with an error code property
>   (3) throw an exception.
> 
> 1 seems preferable to me.

I think the code can just be rewritten to adapt what the C++ component does to (1) or (2):

   function ConvertErrorToString(errorString, f) {
      return function(rv, value) { 
        f(Components.isSuccessCode(rv) ? null : errorString, value);
      };
   }

   ...

   return cryptoService.sign(...,
                             ConvertErrorToString("Sign Failed", aCallback));

or

   function DropErrorCode(f) {
      return function(rv, value) { 
        f(value);
      };
   }

   ...

   return cryptoService.sign(..., DropErrorCode(aCallback));

> @@ +86,5 @@
> > +interface nsIIdentitySignCallback : nsISupports
> > +{
> > +  /** On success, base64urlSignature is the base-64-URL-encoded signature
> > +   *
> > +   * For RS256 signatures, XXXX.
> 
> ...yes?

XXXX obviously means "The signature is in PKCS#1.5 format." :)

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to Brian Smith (:bsmith) from comment #34)
> > Hmm? The test frameworks support https requests.
> 
> xpcshell doesn't have proper HTTPS support yet. See bug 466524.

mochitest-based harnesses do, though (e.g. mochitest-browser-chrome. Can you use that to test this?

Comment 36

[Ben Adida [:benadida]]

Patch forthcoming, some comments here.

All of Dolske's comments re: jwcrypto and crypto service addressed except:

> > +interface nsIURI;
> > +interface nsIIdentityKeyGenCallback;
> > +interface nsIIdentityKeyPair;
> > +interface nsIIdentitySignCallback;
> 
> These 3 are defined in this file, so these lines shouldn't be needed.

The callback declarations are necessary because of circular dependencies, but I removed the nsIIdentityKeyPair one since that one is not necessary.

> @@ +28,5 @@
> > + *
> > + * "RS256": RSA + SHA-256.
> > + *
> > + * "DS160": DSA with SHA-1. A 1024-bit prime and a 160-bit subprime with SHA-1.
> > + */
> 
> I'd strongly consider being verbose here. EG "RSA-SHA256" is only a few
> extra bytes, and is much more self-explanitory.

As per bsmith's point, this is part of the spec, I added a link to it in the code. In our experience with the shim, it's actually not a problem so far.

> > +  // AUTF8String verify(in AUTF8String aSignature, in AUTF8String encodedPublicKey);
> 
> This needs to be removed, implemented, or marked as "// XXX Bug 12345
> implement foo".

so bsmith can sleep at night: to be implemented, bug filed and referenced.

> Ok, now I see why you're using "in nsresult" here.
> 
> It doesn't really seem useful, as nothing cares about the specific type of
> success/failure.

For now... but I'd rather keep that in there so we have access to it if we need to.

Comment 37

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Justin Dolske [:Dolske] from comment #33)
> Comment on attachment 634692 [details] [diff] [review]
> v.2.1 address review comments and include native base64 code
> 
> Review of attachment 634692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Gah. Sorry this has taken so long to complete. I've a bunch of review
> comments from going through this closely, but they're all relatively small
> issues that should be fixable easily.

Thank you for your thorough review, Justin!

I'm responding to your points on Identity.jsm here.  Brian and Ben have separately addressed the crypto.

> A couple general comments:
> 
> * For all "XXX" and "TODO" type comments, please either resolve them or file
> bugs so they can become "XXX bug 12345: need kittens here" comments.

This has been done.

> ::: toolkit/identity/Identity.jsm
> @@ +76,5 @@
> > +  init: function init() {
> > +    XPCOMUtils.defineLazyModuleGetter(this,
> > +                                      "_store",
> > +                                      "resource://gre/modules/identity/IdentityStore.jsm",
> > +                                      "IdentityStore");
> 
> No point in using lazy getters when they're immediately called. Just
> Cu.import().

Good point.  Fixed @4c4eea

> IDService seems to be a singleton. Any reason to store them as properties
> instead of just using them as globals for the module?

Ben and I talked this over, and we actually like the encapsulation.  If it's not too out of the ordinary, may we leave them as they are?

> @@ +95,5 @@
> > +    // Clear RP state
> > +    this.RP.init();
> > +
> > +    // Clear IDP state
> > +    this.IDP.init();
> 
> RAII would seem preferable here, unless there's a need to explicitly call
> init() separately/again elsewhere.

+1

We've renamed init() to reset()

RP, IDP, Identity, and the IdentityStore now listed for quit-application-granted and reset themselves (in order to not leak memory).  I chose to leave a reset() function on Identity.jsm that calls reset on the other three just so we can make testing easier.  I've commented it so that anyone using the module will know not to use it.  does that seem ok?  (fixed @b293c9)

> @@ +124,5 @@
> > +  shutdown: function shutdown() {
> > +    log("shutdown");
> > +    this.init();
> > +    try {
> > +      Services.obs.removeObserver(this, "nsPref:changed");
> 
> This ought to be Services.prefs.removeObserer().

Oops.  Thanks!  (@fc63b7)

> @@ +153,5 @@
> > +   *
> > +   * @param aIdentity
> > +   *        (string) the email chosen for login
> > +   */
> > +  selectIdentity: function selectIdentity(aRPId, aIdentity) {
> 
> This has a lot of (nested) anonymous callbacks, I'd suggest naming each so
> that debugging is easier when looking at call stacks.

Quite true.  I've gone through and named all anonymous functions in all modules  (@fa4178)

> @@ +168,5 @@
> > +    }
> > +
> > +    // It's possible that we are in the process of provisioning an
> > +    // identity.
> > +    let provId = rp.provId || null;
> 
> This seems to be the only place that's checking for a null, is it really
> needed? (Alternatively, seems like it should be initialized/set to null by
> underlying code when it's... undefined?)

Good point.  I've changed it here and updated a test for it elsewhere  (@ac4e91)

> @@ +170,5 @@
> > +    // It's possible that we are in the process of provisioning an
> > +    // identity.
> > +    let provId = rp.provId || null;
> > +
> > +    // XXX consolidate rp, flows, etc. ?
> 
> Not sure what this means.

Neither am I :)  I'm sure it meant something at one point.  Comment removed.

> @@ +179,5 @@
> > +    log("selectIdentity: provId:", provId, "origin:", rp.origin);
> > +
> > +    // Once we have a cert, and once the user is authenticated with the
> > +    // IdP, we can generate an assertion and deliver it to the doc.
> > +    self.RP._generateAssertion(rp.origin, aIdentity, function(err, assertion) {
> 
> nit: this.RP, and might suggest moving the |let self=this| down to here
> since it's the first use.

Yes, that's clearer and cleaner.  Fixed @ac4e91

> @@ +184,5 @@
> > +      if (!err && assertion) {
> > +        self.RP._doLogin(rp, rpLoginOptions, assertion);
> > +        return;
> > +
> > +      } else {
> 
> Please avoid else-after-return, all it gives you is more indenting. :(

I've gone through and eradicated all else-after-returns.  (@ac4e91)

> @@ +192,5 @@
> > +          if (err) {
> > +            rp.doError(err);
> > +            return;
> > +          }
> > +
> 
> See, you avoided it right here. ;-)

I was just flirting with it.  But now I'm in love.

> @@ +206,5 @@
> > +
> > +            // At this point, we already have a cert.  If the user is also
> > +            // already authenticated with the IdP, then we can try again
> > +            // to generate an assertion and login.
> > +            if (!err) {
> 
> Sometimes you're checking |if (err)|, other times |if (!err)|, etc. Would be
> nice to be consistent.

I have updated them all to be test for |if (err)|, which feels more natural.  @ac4e91

> @@ +209,5 @@
> > +            // to generate an assertion and login.
> > +            if (!err) {
> > +              // XXX quick hack - cleanup is done by registerCertificate
> > +              // XXX order of callbacks and signals is a little tricky
> > +              //self._cleanUpProvisionFlow(aProvId);
> 
> I don't understand these comments.

I've rephrased and clarified.  It was a question we had at one point, but we believe we've got it right now.  @ac4e91

> @@ +262,5 @@
> > +   *        (function) callback to invoke on completion
> > +   *                   with first-positional parameter the error.
> > +   */
> > +  _discoverIdentityProvider: function _discoverIdentityProvider(aIdentity, aCallback) {
> > +    let domain = aIdentity.split('@')[1];
> 
> Should have a check here to ensure the identity has a sane (user@host)
> format.
> 
> We should explicitly fail early on for a user fatfingering (or deliberately
> sniffing around with) weird input like "me@", "@host", "me@foo@bar", etc.
> 
> Ditto for malformed domains -- "user@gmail" or
> "[e-mail]/accounts/resetpassword.html?".
> 
> May just want a spinoff bug to explicitly go through and add defensive
> checks for all non-trusted callers.

Yes, this was a big hole.  I've created a parseEmail function for now and added tests for it (@c271e9)

I've also attached a link to follow-up bug 767610, which specifies a need for a proper parser and verifier that can be shared with other js modules.

I hope my short-term fix is ok until we can get a longer-term solution.

> @@ +287,5 @@
> > +   *
> > +   * @param aScheme
> > +   *        (string) (optional) Protocol to use.  Default is https.
> > +   *                 This is necessary because we are unable to test
> > +   *                 https.
> 
> Hmm? The test frameworks support https requests.

I've filed a follow-up bug (769854) to move this test to mochi so we can do https.  For now, xpcshell requires http.

> @@ +296,5 @@
> > +  _fetchWellKnownFile: function _fetchWellKnownFile(aDomain, aScheme, aCallback) {
> > +    if (arguments.length <= 2) {
> > +      aCallback = aScheme;
> > +      aScheme = "https";
> > +    }
> 
> If you need to keep the optional arg, make it the last arg to avoid this
> magic switcharoo.
> 
> Could then just do: aScheme = aScheme || "https"

I've made it an optional last argument, as you suggest.  @a57656

> @@ +311,5 @@
> > +    req.responseType = "json";
> > +    req.mozBackgroundRequest = true;
> > +    req.onload = function _fetchWellKnownFile_onload() {
> > +      if (req.status < 200 || req.status >= 400)
> > +        return aCallback(req.status);
> 
> We shouldn't expose the status to the callback. Doesn't seem used, and is
> bad form to expose it to untrusted content because it leaks info about what
> resources exist. Just log() it.

+1  We're just returning "error" so the caller knows it failed.  And we're logging the code (@6ce5e5)

> @@ +316,5 @@
> > +      try {
> > +        let idpParams = req.response;
> > +
> > +        // Verify that the IdP returned a valid configuration
> > +        if (! (idpParams.provisioning &&
> 
> I wonder a bit if there should be more verification here, but it seems
> reasonable as-is.

I checked with Ben on this, and he believes it's ok as it is.  I also checked with Matthew, who pointed out that nsIUri will fail if any bogus urls are used.  I believe that's sufficient; or do you think I should file a follow-up bug for it?

> @@ +347,5 @@
> > +      if (req.statusText) {
> > +        err += " " + req.statusText;
> > +      }
> > +      log("ERROR: _fetchWellKnownFile:", err);
> > +      return aCallback(err);
> 
> Ditto about not exposing the error status.

Yes.  Fixed this one, too.

> ::: toolkit/identity/IdentityProvider.jsm
> @@ +108,5 @@
> > +    }
> > +  },
> > +
> > +  shutdown: function RP_shutdown() {
> > +    this.init();
> 
> Why are you initing for shutdown?

changed the name to reset() to be clearer.  The reason is that we want to destroy any lingering references and listeners on quit.  I believe otherwise we may leak?  @306d58

> @@ +110,5 @@
> > +
> > +  shutdown: function RP_shutdown() {
> > +    this.init();
> > +    try {
> > +      Services.obs.removeObserver(this, "nsPref:changed");
> 
> try/catch shouldn't be needed here, as noted elsewhere you want
> Services.prefs.removeObserver.

Yes, thank you.  Fixed.  @fc63b7 and @a5a719

> Might be followup worthy to look at unifying this stuff, seems like overkill
> to have each submodule doing all this init/observing/shutdown stuff.

I think I took this too far in the wrong direction in response to a different issue!  I've filed follow-up bug 769873 for us to take a closer look at this.

> @@ +115,5 @@
> > +    } catch (ex) {}
> > +  },
> > +
> > +  get securityLevel() {
> > +    return 1;
> 
> Is this documented somewhere? Add a comment pointing there?

Yes, this is just a temporary hack.  Matthew observed that this is described in the spec, but it's not exactly clear how to implement it.  So this keeps us as spec-compliant as possible on this point :)

> @@ +208,5 @@
> > +    // Determine recommended length of cert.
> > +    let duration = this.certDuration;
> > +    if (!duration) {
> > +      throw "invalid duration";
> > +    }
> 
> The certDuration getter seems infallible, is this check really needed?
> Shouldn't it call aCaller.doError() if so?

You're right.  I've removed the test.

> @@ +229,5 @@
> > +   * @param aReason
> > +   */
> > +  raiseProvisioningFailure: function raiseProvisioningFailure(aProvId, aReason) {
> > +    log("ERROR: Provisioning failure:", aReason);
> > +    Cu.reportError("Provisioning failure: " + aReason);
> 
> Bonus points for checking that all .reportError() calls mention "BrowserID:
> ..." so that there's some context for the error console line showing all
> this.

+1  I've refactored out the logging system into its own module and made a wrapper for recordError that always mentions "Identity".  @5437ab

> @@ +234,5 @@
> > +
> > +    // look up the provisioning caller and its callback
> > +    let provFlow = this._provisionFlows[aProvId];
> > +    if (!provFlow) {
> > +      let errStr = "No provisioning flow found with id:" + aProvId;
> 
> This pattern happens a lot -- maybe implement a getProvFlow(id) method that
> throws+logs when the flow for the ID does not exist?

True.  I've followed your suggestion and created a getter that takes an optional errback.  The code is much cleaner now.  @689e3c

> @@ +335,5 @@
> > +
> > +    // store the keypair and certificate just provided in IDStore.
> > +    this._store.addIdentity(provFlow.identity, provFlow.kp, aCert);
> > +
> > +    // Great success!
> 
> Is nice!

yessss!!!

> ::: toolkit/identity/IdentityStore.jsm
> @@ +27,5 @@
> > +
> > +// _loginStates will associate remote origins with a login status and
> > +// the email the user has chosen as his or her identity when logging
> > +// into that origin.
> > +IDServiceStore.prototype._loginStates = null;
> 
> Could just move these comments into init().

Done and done.  @ac4e91

Matthew is heroically preparing the patch now and will submit it shortly

Again, many thanks for your time and close reading.
Cheers,
Jed

Comment 38

[Justin Dolske [:Dolske]]

Comment on attachment 634692 [details] [diff] [review]
v.2.1 address review comments and include native base64 code

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

Comments solely on IdentityCryptoService.cpp. Nothing major, just some things to consider/look at. Let's call it a "r+ for this file with any changes you make".

::: toolkit/identity/IdentityCryptoService.cpp
@@ +74,5 @@
> +}
> +
> +// IMPORTANT: This must be called immediately after the function returning the
> +// SECStatus result. The recommended usage is:
> +//    nsresult rv = MapSECStatus(f(x, y, z));

Really? That's terrible for readability. SignRunnable::Run(), in particular, is tough.

Seems nicer as:

 s = PK11_Foo();
 mRv = MapSECStatus(s);

Maybe it's just me.

@@ +218,5 @@
> +  nsresult Init()
> +  {
> +    nsresult rv;
> +    nsCOMPtr<nsISupports> dummyUsedToEnsureNSSIsInitialized
> +      = do_CreateInstance("@mozilla.org/security/keyobjectfactory;1", &rv);

Could do_GetService("@mozilla.org/psm;1") to make it a little more clear about PSM ownership, as WeaveCrypto does...

@@ +376,5 @@
> +  if (!*privateKey) {
> +    MOZ_ASSERT(!*publicKey);
> +    return PRErrorCode_to_nsresult(PR_GetError());
> +  }
> +  if (!*publicKey) {

Nuke the privateKey so it doesn't leak? (Not sure how possible it is to generate one but not the other...)

@@ +468,5 @@
> +  if (!NS_IsMainThread()) {
> +    nsNSSShutDownPreventionLock locker;
> +    if (isAlreadyShutDown()) {
> +      mRv = NS_ERROR_NOT_AVAILABLE;
> +    } else {

You could convert these to early returns to eliminate a bunch of nesting... I feel less strongly about it in C, though.

@@ +499,5 @@
> +          mKeyPair = new KeyPair(privk, pubk);
> +        }
> +
> +        SECKEY_DestroyPrivateKey(privk);
> +        SECKEY_DestroyPublicKey(pubk);

Aroo? These seem to actually deallocate, so shouldn't they only be in the KeyPair destructor?

...Oh, I see there are SECKEY_CopyPublicKey / SECKEY_CopyPrivateKey calls in the constructor.

Why bother copying at all? Just let the KeyPair take ownership.

@@ +506,5 @@
> +
> +    NS_DispatchToMainThread(this);
> +  } else {
> +    // Back on Main Thread
> +    (void) mCallback->GenerateKeyPairFinished(mRv, mKeyPair);

Oh, so that's kind of a neat trick. You redispatch yourself upon completion to mainthread to trigger the JS callback.

@@ +530,5 @@
> +    if (isAlreadyShutDown()) {
> +      mRv = NS_ERROR_NOT_AVAILABLE;
> +    } else {
> +      // We need the output in PKCS#11 format, not DER encoding, so we must use
> +      // PK11_HashBuf & PK11_Sign instead of SEC_SignData.

s/&/&&/ (too much code for me today ;-)

@@ +533,5 @@
> +      // We need the output in PKCS#11 format, not DER encoding, so we must use
> +      // PK11_HashBuf & PK11_Sign instead of SEC_SignData.
> +
> +      SECItem sig = { siBuffer, NULL, 0 };
> +      if (!SECITEM_AllocItem(NULL, &sig, PK11_SignatureLen(mPrivateKey))) {

Consider breaking out the PK11_SignatureLen call; lest it fail you'll know it was a bad key and not a bad alloc.

@@ +536,5 @@
> +      SECItem sig = { siBuffer, NULL, 0 };
> +      if (!SECITEM_AllocItem(NULL, &sig, PK11_SignatureLen(mPrivateKey))) {
> +        mRv = PRErrorCode_to_nsresult(PR_GetError());
> +      } else {
> +        PRUint8 hash[256 / BITS_PER_BYTE]; // big enough for SHA-1 or SHA-256

This will be incorrect on a GE-600 Multics system. Followup bug?

@@ +540,5 @@
> +        PRUint8 hash[256 / BITS_PER_BYTE]; // big enough for SHA-1 or SHA-256
> +        SECOidTag hashAlg = mPrivateKey->keyType == dsaKey ? SEC_OID_SHA1
> +                                                           : SEC_OID_SHA256;
> +        SECItem hashItem = { siBuffer, hash,
> +                             hashAlg == SEC_OID_SHA1 ? 20 : 32 };

I'm vaguely remembering there being a helper to map the hash/key type to a size?

Probably not important here, so long as you're sure any future change to supported keys/hashes will update this. Oh, I guess they really will because PK11_HashBuf should complain if the provided size is too small. Carry on!

@@ +545,5 @@
> +
> +        mRv = MapSECStatus(PK11_HashBuf(hashAlg, hash,
> +                    const_cast<PRUint8*>(reinterpret_cast<const PRUint8 *>(
> +                                            mTextToSign.BeginReading())),
> +                                      mTextToSign.Length()));

*>:( <3 JS's lack of typechecking sometimes.

My C++ is a little rusty, but isn't this typically simply written as:

  PromiseFlatCString(mTextToSign).get() ?

Comment 39

[Matthew N. [:MattN]]

Attachment: v.3 modules

Comment 40

[Matthew N. [:MattN]]

Attachment: v.3 tests

View the results of the try push at http://tbpl.mozilla.org/?tree=Try&rev=856caeb2166f

Comment 41

[Justin Dolske [:Dolske]]

Comment on attachment 638100 [details] [diff] [review]
v.3 modules

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

Responses in comment 36 and comment 37 are fine. Basically just gave this a light review pass, focusing on the parts that changed. Only a few minor comments.

I'd call this r+, but I think we're still waiting for an IdentityCryptoService.cpp update/reply to my review pass in comment 38?

So just marking f+, pending resolution of that one last piece. Woot!

::: toolkit/identity/Identity.jsm
@@ +28,5 @@
> +  Logger.log([null].concat(aMessageArgs));
> +}
> +function reportError(...aMessageArgs) {
> +  Logger.reportError([null].concat(aMessageArgs));
> +}

These should have s/null/"Identity"/, or else logged messages have no identifying prefix to tell what the heck is outputting the string.

I'd also suggest moving the concat bits into Logger, so that these are essentially just calling with a prefix...

Note sure how that works with the fancy new syntax, is it as simple as this?

  function log(...aMessageArgs) {
    Logger.log("Identity", aMessageArgs);
  }

@@ +89,5 @@
> +        domain: match[2]
> +      };
> +    }
> +    return null;
> +  },

Would be good to add a few lines in a test to ensure that parseEmail() accepts/rejects as expected.

@@ +143,5 @@
> +      if (!err && assertion) {
> +        self.RP._doLogin(rp, rpLoginOptions, assertion);
> +        return;
> +
> +      } else {

else-after-return?

::: toolkit/identity/IdentityStore.jsm
@@ +23,5 @@
> +}
> +
> +IDServiceStore.prototype._identities = null;
> +
> +IDServiceStore.prototype._loginStates = null;

Put these in the prototype object?

::: toolkit/identity/LogUtils.jsm
@@ +96,5 @@
> +    // Report the error in the browser
> +    Cu.reportError("Identity: " + this._generateLogMessage(prefix));
> +
> +    // And also log to the console, if logging is enabled
> +    this.log(prefix, args);

Presumably this will double-log errors when logging is enabled (via .log's logStringMessage), but harmless.

::: toolkit/identity/RelyingParty.jsm
@@ +169,5 @@
> +    log("_doLogout: rpId:", aRpCaller.id, "origin:", aOptions.origin);
> +
> +    let state = this._store.getLoginState(aOptions.origin) || {};
> +
> +    // XXX add tests for state change

Gasp! No bug number! ;-)

Comment 42

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Justin Dolske [:Dolske] from comment #41)
> Comment on attachment 638100 [details] [diff] [review]
> v.3 modules
> 
> Review of attachment 638100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Responses in comment 36 and comment 37 are fine. Basically just gave this a
> light review pass, focusing on the parts that changed. Only a few minor
> comments.
> 
> I'd call this r+, but I think we're still waiting for an
> IdentityCryptoService.cpp update/reply to my review pass in comment 38?
> 
> So just marking f+, pending resolution of that one last piece. Woot!

w00t indeed!

> ::: toolkit/identity/Identity.jsm
> @@ +28,5 @@
> > +  Logger.log([null].concat(aMessageArgs));
> > +}
> > +function reportError(...aMessageArgs) {
> > +  Logger.reportError([null].concat(aMessageArgs));
> > +}
> 
> These should have s/null/"Identity"/, or else logged messages have no
> identifying prefix to tell what the heck is outputting the string.

It actually always prints "Identity" no matter what, so it will be identifiable; then the [null] or ["IDP"] or ["RP"] or whatever specifies a subcontext.  

That said, the logging got a bit grody-looking.  I've filed follow-up bug 770418 to fix the logging output.  

> I'd also suggest moving the concat bits into Logger, so that these are
> essentially just calling with a prefix...

Yes.  I'm thinking the interface would instead be something like:

let logger = getLogger("IDP")   // No fancy footwork
logger.log( ... )

> Note sure how that works with the fancy new syntax, is it as simple as this?
> 
>   function log(...aMessageArgs) {
>     Logger.log("Identity", aMessageArgs);
>   }

The fancy new syntax throws me a bit, too.  Now that ECMA has these optional argument lists, is there anyway to generate optional args without generating an array?  Like in python, where you can say

someArray = [1, 2, 3]
foo( *someArray )       # equiv to foo(1, 2, 3), not foo([1,2,3])

Reading the ecma docs, it looks like this is only a one-way thing, which makes it hard to compose functions with optional args, unless I'm not finding the syntax I'm looking for...

Anyway, a logger cleanup will hide whatever fanciness there is inside LogUtils alone.

> @@ +89,5 @@
> > +        domain: match[2]
> > +      };
> > +    }
> > +    return null;
> > +  },
> 
> Would be good to add a few lines in a test to ensure that parseEmail()
> accepts/rejects as expected.

Yes!  I had actually already done that:
http://hg.mozilla.org/projects/pine/rev/c271e987098a#l2.12

I used your examples as a basis for the evil tests

> @@ +143,5 @@
> > +      if (!err && assertion) {
> > +        self.RP._doLogin(rp, rpLoginOptions, assertion);
> > +        return;
> > +
> > +      } else {
> 
> else-after-return?

d'oh!  Fixed @4ba5e3

> ::: toolkit/identity/IdentityStore.jsm
> @@ +23,5 @@
> > +}
> > +
> > +IDServiceStore.prototype._identities = null;
> > +
> > +IDServiceStore.prototype._loginStates = null;
> 
> Put these in the prototype object?

Fixed  @22afbc

> ::: toolkit/identity/LogUtils.jsm
> @@ +96,5 @@
> > +    // Report the error in the browser
> > +    Cu.reportError("Identity: " + this._generateLogMessage(prefix));
> > +
> > +    // And also log to the console, if logging is enabled
> > +    this.log(prefix, args);
> 
> Presumably this will double-log errors when logging is enabled (via .log's
> logStringMessage), but harmless.

I don't see the double-logging in xpcshell tests - would that happen through the browser?

> ::: toolkit/identity/RelyingParty.jsm
> @@ +169,5 @@
> > +    log("_doLogout: rpId:", aRpCaller.id, "origin:", aOptions.origin);
> > +
> > +    let state = this._store.getLoginState(aOptions.origin) || {};
> > +
> > +    // XXX add tests for state change
> 
> Gasp! No bug number! ;-)

Aha!  In this case, I had already added tests for state change and just forgot to remove the comment.  So I don't think any follow-up bug is necessary.  The test for state change is here:
http://hg.mozilla.org/projects/pine/rev/948046c081af#l1.86

Thanks, Justin.  Cheers!
j

Comment 43

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jed Parsons from comment #42)
> > Presumably this will double-log errors when logging is enabled (via .log's
> > logStringMessage), but harmless.
> 
> I don't see the double-logging in xpcshell tests - would that happen through
> the browser?

Cu.reportError only appears in the Error Console, it doesn't show up in stdout. logStringMessage also appears in the error console (as a "message" rather than an "error"). Hence the double-logging issue.

Comment 44

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #43)
> (In reply to Jed Parsons from comment #42)
> > > Presumably this will double-log errors when logging is enabled (via .log's
> > > logStringMessage), but harmless.
> > 
> > I don't see the double-logging in xpcshell tests - would that happen through
> > the browser?
> 
> Cu.reportError only appears in the Error Console, it doesn't show up in
> stdout. logStringMessage also appears in the error console (as a "message"
> rather than an "error"). Hence the double-logging issue.

Ah! Sorry, I see now.  

Yes, that's kind of tacky for the logger to do.  I've updated LogUtils so that reportError doesn't call the log function - it does a Cu.reportError() and a dump().  SHA = @0e276f

Comment 45

[Matthew N. [:MattN]]

Attachment: v.4 modules

Comment 46

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

(In reply to Justin Dolske [:Dolske] from comment #38)
> Comments solely on IdentityCryptoService.cpp. Nothing major, just some
> things to consider/look at. Let's call it a "r+ for this file with any
> changes you make".

Thanks Justin!

> Seems nicer as:
> 
>  s = PK11_Foo();
>  mRv = MapSECStatus(s);
> 
> Maybe it's just me.

The advantage is that you don't need |s|. SignRunnable::Run looks ugly because of the reinterpret_casts and const_casts more than because of MapSECStatus. So, I just left it as is.

> Could do_GetService("@mozilla.org/psm;1") to make it a little more clear
> about PSM ownership, as WeaveCrypto does...

Thank you!

> Nuke the privateKey so it doesn't leak? (Not sure how possible it is to
> generate one but not the other...)

Done.

> Why bother copying at all? Just let the KeyPair take ownership.

Done.

> @@ +468,5 @@
> > +  if (!NS_IsMainThread()) {
> > +    nsNSSShutDownPreventionLock locker;
> > +    if (isAlreadyShutDown()) {
> > +      mRv = NS_ERROR_NOT_AVAILABLE;
> > +    } else {
> 
> You could convert these to early returns to eliminate a bunch of nesting...
> I feel less strongly about it in C, though.

those returns would have to be "return NS_DispatchToMainThread(this);" and I think it is clearer to have that call to NS_DispatchToMainThread(this) happen in exactly one place to make this "neat trick" clearer:

> > +    NS_DispatchToMainThread(this);
> > +  } else {
> > +    // Back on Main Thread
> > +    (void) mCallback->GenerateKeyPairFinished(mRv, mKeyPair);
> 
> Oh, so that's kind of a neat trick. You redispatch yourself upon completion
> to mainthread to trigger the JS callback.

> s/&/&&/ (too much code for me today ;-)

I s/&/and/

> Consider breaking out the PK11_SignatureLen call

Done.

> > +        PRUint8 hash[256 / BITS_PER_BYTE]; // big enough for SHA-1 or SHA-256
> 
> This will be incorrect on a GE-600 Multics system. Followup bug?

Can't tell if joking or high. Regardless, I changed it to "PRUint8 hash[32];" because below I end up hard coding "32" anyway, so it is clearer.

> @@ +540,5 @@
> > +        PRUint8 hash[256 / BITS_PER_BYTE]; // big enough for SHA-1 or SHA-256
> > +        SECOidTag hashAlg = mPrivateKey->keyType == dsaKey ? SEC_OID_SHA1
> > +                                                           : SEC_OID_SHA256;
> > +        SECItem hashItem = { siBuffer, hash,
> > +                             hashAlg == SEC_OID_SHA1 ? 20 : 32 };
> 
> I'm vaguely remembering there being a helper to map the hash/key type to a
> size?

Not for PK11_*; only for HASH_*.

>   PromiseFlatCString(mTextToSign).get() ?

Done. (PromiseFlatCString wasn't needed).

Thanks!

Comment 47

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

http://hg.mozilla.org/projects/pine/rev/386b9e0d598f

Comment 48

[Ben Adida [:benadida]]

I think we're good to go to push this, then? MattN, you up for it?

Comment 49

[Matthew N. [:MattN]]

Once dolske gives r+

Comment 50

[Ben Adida [:benadida]]

(In reply to Matthew N. [:MattN] from comment #49)
> Once dolske gives r+

oh I thought he had, pending the changes he asked for, but maybe I misread. dolske, help :)

Comment 51

[Justin Dolske [:Dolske]]

Comment on attachment 638874 [details] [diff] [review]
v.4 modules

r+ with the changes Brian made in http://hg.mozilla.org/projects/pine/rev/386b9e0d598f.

Comment 52

[Justin Dolske [:Dolske]]

Comment on attachment 638101 [details] [diff] [review]
v.3 tests

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

rs=me, didn't go through them in detail. :)

Comment 53

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88070ff09ccd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2699c091b91b

try link [nearly identical push]: https://tbpl.mozilla.org/?tree=Try&rev=3c3b664844bd

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88070ff09ccd
https://hg.mozilla.org/mozilla-central/rev/2699c091b91b

Comment 55

[Jason Smith [:jsmith]]

Is it possible to verify this patch from an end-user perspective? Or is this just internal code changes?

Comment 56

[Matthew N. [:MattN]]

Not until the UI or DOM pieces land.

Comment 57

[:Gavin Sharp [email: [e-mail]]]

I landed a fix for a build failure on clang, and an uninitialized variable use that clang also caught:

https://hg.mozilla.org/mozilla-central/rev/1751d97cc9e4

Comment 62

[Masatoshi Kimura [:emk]]

I'm not sure who is responsible for Identity.jsm (Ben attached WIP, Jed attached v2, and Matthew attached the final patch, but the patch author in the commit is still Jed). So I'm asking all (except Ben whose account is disabled).
Identity.jsm has mozBackgroundRequest after xhr.open(). It caused test failures in bug 1132347. Note that mozBackgroundRequest after xhr.open() has never had effect from the start. Bug 1132347 just made it explicit.
Why did you put it after xhr.open()? Is mozBackgroundRequest needed here?

Comment 63

[Matthew N. [:MattN]]

(In reply to Masatoshi Kimura [:emk] from comment #62)
> Why did you put it after xhr.open()?

It was a mistake.

> Is mozBackgroundRequest needed here?

Yes, we don't want dialogs to popup seemingly out of nowhere.