664614 - Browser level API for firefox Identity Client

Status: VERIFIED INVALID

(Core Graveyard :: Identity, defect)

Description

[JR Conlin [:jrconlin,:jconlin]]

Create the browser API for the identity client

Comment 2

[David Dahl :ddahl]

Attachment: v 1 Patch - needs feedback

I am sure many things are wrong with this patch, if you want to build it use the patch in bug 665057

Tests can be run like so:

cd {OBJ_DIR}; TEST_PATH=services/identity/tests/ make -C . mochitest-plain

The platform IDL referenced in IDService.jsm is here:
https://bugzilla.mozilla.org/attachment.cgi?id=600465&action=diff#a/security/manager/ssl/public/nsIIdentityServiceKeyPair.idl_sec1

Comment 3

[David Dahl :ddahl]

Additional notes:

This code in this patch can be incorporated into an extension or the browser itself. It is raw and still requires the platform patch in bug 665057. This is a great bug for you to get to know Firefox front end coding methods, style and process;) (I will also continue to work on it after I go back and refactor the crypto and XPCOM bits.)

I have roughed out a navigator property that does some things that are mentioned in https://wiki.mozilla.org/Identity/BrowserID - however, some things are not completely clear so I no doubt am missing some things. I have so far only attempted to implement id.genKeyPair and id.get. I have stubbed out the rest of the nav.id API method names.

Don't hesitate to ping me on questions.

Comment 4

[Gregory Szorc [:gps]]

MPL 2.0 should be used.

Comment 5

[David Dahl :ddahl]

(In reply to Gregory Szorc [:gps] from comment #4)
> MPL 2.0 should be used.

Yes. i will fix that in the next set of updates.

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 600467 [details] [diff] [review]
v 1 Patch - needs feedback

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

Mostly nits on my feedback.

My biggest question is how this will work for clients that are not web sites (like Sync). These could be running in chrome-privileged code as well. Not sure if that plays nice with the sandboxing, etc.

::: services/identity/IdentityAPI.js
@@ +5,5 @@
> +
> +let Cu = Components.utils;
> +let Ci = Components.interfaces;
> +let Cc = Components.classes;
> +

const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

is the convention typically used.

@@ +9,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "IDService", function (){

nit space between () and {

@@ +15,5 @@
> +  Cu.import("resource:///modules/IdentityService.jsm", scope);
> +  return scope.IdentityService;
> +});
> +
> +const CONFIG_PROPERTIES = { allowPersistent: "allowPersistent",

nit no space between container brace and key

@@ +42,5 @@
> +
> +  window: null,
> +
> +  get host() {
> +    return Services.io.newURI(this.window.location, null, null).prePath;

I think this means you are approaching things from the perspective that a web page (window) will be the client. What about invisible clients, like Sync? Is there a more appropriate API for them?

@@ +51,5 @@
> +  assertions: null, // TODO: cache assertion(s) for this host
> +
> +  sandbox: null,
> +
> +  init: function IA_init(aWindow)

nit reconsider prefixing function names. This is an area of contention. The stack traces give you the file and line number. Unless you have similarly named function in the same file, I feel the prefixing just makes things uglier.

@@ +52,5 @@
> +
> +  sandbox: null,
> +
> +  init: function IA_init(aWindow)
> +  {

nit cuddle braces. Comment applies throughout.

@@ +233,5 @@
> +  {
> +    // TODO: make this configurable for RSA or DSA
> +
> +    let jwk = {
> +      "public-key": {

Don't need to quote key names in JS.

@@ +298,5 @@
> +  {
> +    let header = {
> +      "alg": "DS160",
> +      "exp": Date.now() + MIL_SEC_PER_DAY,
> +      "aud": this.host,

Don't quote keys.

::: services/identity/IdentityService.jsm
@@ +64,5 @@
> +{
> +  dump("IDService: " + aMsg + "\n");
> +}
> +
> +const ALGORITHMS = { RS256: 1, DS160: 2, };

Nit no space in this file too.

@@ +88,5 @@
> +  },
> +
> +  registry: null,
> +
> +  generateUUID: function IS_generateUUID()

What's the relationship between this in uuid()?

@@ +103,5 @@
> +
> +    let alg = ALGORITHMS[aAlgorithm];
> +    let url = Services.io.newURI(aOrigin, null, null).prePath;
> +    let keypair = DOMCrypto.generateIdentityServiceKeyPair(alg);
> +    let id = uuid();

I'm not sure how the UUID is used. But, some UUIDs (like type 1, which contains the MAC address) may leak data you don't want leaked. Consider asking for an explicit UUID type. If type 1 is acceptable, document assumptions about how it is used so it may not be inadvertently leaked in the future.

@@ +162,5 @@
> +    return this.registry.get(aKey);
> +  }
> +};
> +
> +var IdentityService = new IDService();

let

::: services/identity/Makefile.in
@@ +52,5 @@
> +  $(NULL)
> +
> +EXTRA_JS_MODULES = \
> +	IdentityService.jsm \
> +	$(NULL)

only use tabs in Makefiles rules (targets), not simple assignment

@@ +56,5 @@
> +	$(NULL)
> +
> +ifdef ENABLE_TESTS
> +DIRS += tests
> +endif

TEST_DIRS += tests (see bug 702388)

::: services/identity/tests/Makefile.in
@@ +45,5 @@
> +
> +MODULE		= test_services_identity
> +
> +DIRS		+= mochitest \
> +		$(NULL)

Spaces please

::: services/identity/tests/mochitest/Makefile.in
@@ +35,5 @@
> +#
> +# ***** END LICENSE BLOCK *****
> +
> +DEPTH	= ../../../..
> +topsrcdir	= @top_srcdir@

space not tab

@@ +45,5 @@
> +include $(topsrcdir)/config/rules.mk
> +
> +_TEST_FILES = \
> +		test_identity_navigator_id.html \
> +		$(NULL)

spaces not tabs

Comment 7

[David Dahl :ddahl]

(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 600467 [details] [diff] [review]
> v 1 Patch - needs feedback
> 
> Review of attachment 600467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly nits on my feedback.
> 
> My biggest question is how this will work for clients that are not web sites
> (like Sync). These could be running in chrome-privileged code as well. Not
> sure if that plays nice with the sandboxing, etc.

I don't think that is a requirement for this bug. If it is, someone should begin thinking about an additional, alternate API or additional methods for internal clients. This is only for browsers. 

> 
> ::: services/identity/IdentityAPI.js
> @@ +5,5 @@
> > +
> > +let Cu = Components.utils;
> > +let Ci = Components.interfaces;
> > +let Cc = Components.classes;
> > +
> 
> const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> 
> is the convention typically used.
> 
Incorrect. That is not ever used in Firefox front-end code. Jetpack, perhaps.

> @@ +15,5 @@
> > +  Cu.import("resource:///modules/IdentityService.jsm", scope);
> > +  return scope.IdentityService;
> > +});
> > +
> > +const CONFIG_PROPERTIES = { allowPersistent: "allowPersistent",
> 
> nit no space between container brace and key
> 
Actually, the space is encourage: https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#JavaScript_Practices

> @@ +42,5 @@
> > +
> > +  window: null,
> > +
> > +  get host() {
> > +    return Services.io.newURI(this.window.location, null, null).prePath;
> 
> I think this means you are approaching things from the perspective that a
> web page (window) will be the client. What about invisible clients, like
> Sync? Is there a more appropriate API for them?

Have not thought about this at all. Sounds like a bug to file.

> 
> @@ +51,5 @@
> > +  assertions: null, // TODO: cache assertion(s) for this host
> > +
> > +  sandbox: null,
> > +
> > +  init: function IA_init(aWindow)
> 
> nit reconsider prefixing function names. This is an area of contention. The
> stack traces give you the file and line number. Unless you have similarly
> named function in the same file, I feel the prefixing just makes things
> uglier.

Prefixing is a standard Firefox front end practice. 

> 
> @@ +52,5 @@
> > +
> > +  sandbox: null,
> > +
> > +  init: function IA_init(aWindow)
> > +  {
> 
> nit cuddle braces. Comment applies throughout.
cuddle?

not sure what that means. A The proper style is newline then brace for functions on a prototype. 


> 
> @@ +298,5 @@
> > +  {
> > +    let header = {
> > +      "alg": "DS160",
> > +      "exp": Date.now() + MIL_SEC_PER_DAY,
> > +      "aud": this.host,
> 
> Don't quote keys.

right, this was a copy-paste thing


> @@ +88,5 @@
> > +  },
> > +
> > +  registry: null,
> > +
> > +  generateUUID: function IS_generateUUID()
> 
> What's the relationship between this in uuid()?
I need to remove that function. I am not sure yet if the uuid is needed. This patch is just to get someone started with frontend code that uses the platform functionality in bug 665057  

> ::: services/identity/Makefile.in
> @@ +52,5 @@
> > +  $(NULL)
> > +
> > +EXTRA_JS_MODULES = \
> > +	IdentityService.jsm \
> > +	$(NULL)
> 
> only use tabs in Makefiles rules (targets), not simple assignment
> 

I blame emacs.

Thank you for the feedback, however, the feedback I am requesting is not actual and perceived JS nits - it is the functionality of the JWK and identity assertion creation code.

1. to make sure the platform code is doing what we need it to and 

2. to give some guidance on using the navigator property component with the new nsIIdentityServiceAPI service to whoever will be implementing the next iteration of the Identity DOM API, which, btw, may end up being written in C++ anyway. I think the jury is out on that because of the move to a fully event-driven API and just how well we can implement nsIEventTarget objects, etc in JS. Maybe khuey can add his 2c?

Comment 8

[David Dahl :ddahl]

> (In reply to Gregory Szorc [:gps] from comment #6)

> > My biggest question is how this will work for clients that are not web sites
> > (like Sync). These could be running in chrome-privileged code as well. Not
> > sure if that plays nice with the sandboxing, etc.

Now I am questioning what this bug is for. Is it a sync bug after all? the title is not very descriptive. If this is a sync bug, I will file a new one and be on my way.

Comment 9

[Richard Newman [:rnewman]]

(In reply to David Dahl :ddahl from comment #7)

Small note:

> > ::: services/identity/IdentityAPI.js
...
> Incorrect. That is not ever used in Firefox front-end code. Jetpack, perhaps.
...
> Actually, the space is encourage:
> https://developer.mozilla.org/En/
> Mozilla_Coding_Style_Guide#JavaScript_Practices

Greg's review here is correct, given that this code is in the services tree, which has its own established coding conventions that are a little more modern.

> not sure what that means. A The proper style is newline then brace for
> functions on a prototype. 

Again, not in our coding style.

Comment 10

[Richard Newman [:rnewman]]

(In reply to David Dahl :ddahl from comment #8)

> Now I am questioning what this bug is for. Is it a sync bug after all? the
> title is not very descriptive. If this is a sync bug, I will file a new one
> and be on my way.

Not Sync; Services.

"Identity", as mentioned in Comment 0, is/was/will be a Services project (on which I very briefly worked!). It turned into what we now know as BrowserID.

Whether the DOMish bits are considered to be part of Services or part of some other component… dunno. Probably Services is a good place for them, because services are best built and maintained holistically.

Comment 11

[David Dahl :ddahl]

(In reply to Richard Newman [:rnewman] from comment #10)

> Whether the DOMish bits are considered to be part of Services or part of
> some other component… dunno. Probably Services is a good place for them,
> because services are best built and maintained holistically.

The more I think about this code (which provides a DOM API) might better be located in dom/base - but I am probably not the person to make that decision.

Comment 12

[Gregory Szorc [:gps]]

(In reply to David Dahl :ddahl from comment #11)
> (In reply to Richard Newman [:rnewman] from comment #10)
> 
> > Whether the DOMish bits are considered to be part of Services or part of
> > some other component… dunno. Probably Services is a good place for them,
> > because services are best built and maintained holistically.
> 
> The more I think about this code (which provides a DOM API) might better be
> located in dom/base - but I am probably not the person to make that decision.

Yes, but there are also components applicable to *any* BID client. May I suggest that you design the BID client bits as a generic library that anybody can use. You can put those in services/identity, toolkit/mozapps/shared, or wherever. Then, write some middleware to connect the DOM to them. Everybody wins.

Comment 13

[David Dahl :ddahl]

(In reply to Gregory Szorc [:gps] from comment #12)
> Yes, but there are also components applicable to *any* BID client. May I
> suggest that you design the BID client bits as a generic library that
> anybody can use. You can put those in services/identity,
> toolkit/mozapps/shared, or wherever. Then, write some middleware to connect
> the DOM to them. Everybody wins.

Indeed, the dom bits will mainly be used for handling events, so the shared part can be a key generation get/create routine and a JWK/JWT formatter. The way that sync uses BID will be completely unique. Are there any design docs on how Sync will consume BID on a wiki somewhere?

Comment 14

[Gregory Szorc [:gps]]

(In reply to David Dahl :ddahl from comment #13)
> Are there any design docs on how Sync will consume BID on a wiki somewhere?

I wish. The current story is Sync gets a BID assertion "somehow" and we run with that. Then there are parts about how to derive a Sync/encryption key in a BID world without J-PAKE. It is a lovely mess. So, the short answer is "no [there is nothing definitive]."

Comment 15

[David Dahl :ddahl]

(In reply to Gregory Szorc [:gps] from comment #14)
> I wish. The current story is Sync gets a BID assertion "somehow" and we run
> with that. Then there are parts about how to derive a Sync/encryption key in
> a BID world without J-PAKE. It is a lovely mess. So, the short answer is "no
> [there is nothing definitive]."

You may want to follow bug 649154 (DOMCrypt) where I am building an 'internal' scriptable crypto API that is more or less general purpose PK crypto, symmetric (DHKE) crypto, signing, hashing, hmac, etc. See: https://wiki.mozilla.org/Privacy/Features/DOMCryptAPISpec/Latest (This draft spec does not reflect the latest round of discussions and the public DOM-exposed API is the strawman for a W3C standard, see: http://www.w3.org/2011/11/webcryptography-charter.html )

Comment 16

[Gregory Szorc [:gps]]

(In reply to David Dahl :ddahl from comment #15)
> You may want to follow bug 649154 (DOMCrypt) where I am building an
> 'internal' scriptable crypto API that is more or less general purpose PK
> crypto, symmetric (DHKE) crypto, signing, hashing, hmac, etc. See:
> https://wiki.mozilla.org/Privacy/Features/DOMCryptAPISpec/Latest (This draft
> spec does not reflect the latest round of discussions and the public
> DOM-exposed API is the strawman for a W3C standard, see:
> http://www.w3.org/2011/11/webcryptography-charter.html )

Oh yay! Hopefully that will cover some of the crypto we have in services/crypto and services/sync/modules/util.js!

Comment 17

[David Dahl :ddahl]

(In reply to Gregory Szorc [:gps] from comment #16)
> Oh yay! Hopefully that will cover some of the crypto we have in
> services/crypto and services/sync/modules/util.js!

Is there a bug for what you need in the future for Sync?

Comment 18

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #16)

> Oh yay! Hopefully that will cover some of the crypto we have in
> services/crypto and services/sync/modules/util.js!

IIRC DOMCrypt started its POC life as a wrapper around WeaveCrypto (The Most Hideous Code In The Universe®, some of which is My Fault®), so I have high hopes that chunks of DOMCrypt will be reusable for Sync.

I've been watching that bug for aaaaages :D

My only concern (which I believe I've mentioned before) is around some of the key memoization stuff that Sync does to try to be fast and allocate fewer objects, but I'm sure ddahl has addressed those along the way.

Comment 19

[Gregory Szorc [:gps]]

For BID, no (mostly because we don't know what all we'll need yet). And, we obviously have everything we need in services/ already :)

Comment 20

[Richard Newman [:rnewman]]

(In reply to David Dahl :ddahl from comment #17)
> (In reply to Gregory Szorc [:gps] from comment #16)
> > Oh yay! Hopefully that will cover some of the crypto we have in
> > services/crypto and services/sync/modules/util.js!
> 
> Is there a bug for what you need in the future for Sync?

Nope; we haven't really touched WeaveCrypto for about a year now, and have no concrete plans to do so.

If we had to phrase things as a bug title, it'd be "Alternative to WeaveCrypto with equivalent or better speed and safety"; that entails string <=> key, AES encryption and decryption, SHA* HMAC, HKDF, PBKDF2, and of course crossing from NSS-land into JS (for which we currently use JS-ctypes).

I'm sure none of this is news to you :D

Philipp and I discussed several times having a purpose-built Sync crypto component, into which we could shove Sync payloads and have the whole base64, JSON, HMAC, and enc/dec take place without crossing too many boundaries. (We make a lot of strings and arrays…).

We have a very different performance profile than most JS or crypto code, in that we can process (JSON, base64, HMAC, decrypt, JSON) tens of thousands of items in a single sequence, so the fewer allocations and function calls we have, the better.

That might still be on the cards, but probably not until Q4.

Comment 21

[David Dahl :ddahl]

(In reply to Richard Newman [:rnewman] from comment #18)
> My only concern (which I believe I've mentioned before) is around some of
> the key memoization stuff that Sync does to try to be fast and allocate
> fewer objects, but I'm sure ddahl has addressed those along the way.

For one thing, we have settled on using typed arrays to write encrypted or decrypted data into. Any kind of string conversion is up to the consumer. I'm not sure if that helps your use case or not. I know WeaveCrypto was welded to slow/memory intense string conversion.

Comment 22

[David Dahl :ddahl]

(In reply to Richard Newman [:rnewman] from comment #20)

> If we had to phrase things as a bug title, it'd be "Alternative to
> WeaveCrypto with equivalent or better speed and safety"; that entails string
> <=> key, AES encryption and decryption, SHA* HMAC, HKDF, PBKDF2, and of
> course crossing from NSS-land into JS (for which we currently use JS-ctypes).
> 
> I'm sure none of this is news to you :D
> 
I imagine you will have to ditch js-ctypes for a scriptable native code interface

> ... (We make a lot of strings and arrays…).
> 
> We have a very different performance profile than most JS or crypto code, in
> that we can process (JSON, base64, HMAC, decrypt, JSON) tens of thousands of
> items in a single sequence, so the fewer allocations and function calls we
> have, the better.

If at some point you spec something out, please cc me.

Comment 23

[Anant Narayanan [:anant]]

After a bit of discussion today, we are splitting this bug into two parts, (the wip patch current does both):

- Implementing the DOM navigator.id interface itself (Bug 753239).
- Implementing a shared JS module "Identity.jsm" that will provide a set of common identity related functions that can be used by other Gecko code (Bug 753238).

Comment 24

[Ben Adida [:benadida]]

ok, marking this as invalid since this has been overtaken by other bugs.

Comment 25

[James Bonacci [:jbonacci]]

Got it. Marking as Verified/Invalid.

Comment 26

[Ben Adida [:benadida]]

Comment on attachment 600467 [details] [diff] [review]
v 1 Patch - needs feedback

older code, dropping this.