936146 - Conform native jwcrypto API to the spec

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Identity, defect)

Description

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

The API for toolkit/identity/jwcrypto.js does not conform to the spec [1].

Now that we need native jwcrypto in FirefoxOS for Firefox Accounts, we should fix this.

I believe altering the api in the native jwcrypto module is of low risk.  It has no callers in Firefox, and I doubt (though I will have to check) that there are add-ons using it.

[1] https://github.com/mozilla/jwcrypto#basic-api

Comment 1

[Matthew N. [:MattN]]

(In reply to Jed Parsons (use request feedback, please) [:jedp, :jparsons] from comment #0)
> It has no callers in Firefox

It does have callers for native persona in desktop Firefox and is being tested. Of course those can be fixed.

Comment 2

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

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #1)
> (In reply to Jed Parsons (use request feedback, please) [:jedp, :jparsons]
> from comment #0)
> > It has no callers in Firefox
> 
> It does have callers for native persona in desktop Firefox and is being
> tested. Of course those can be fixed.

Aha - elm and pine branches?  I'll check them out.  Thanks.

Comment 3

[Matthew N. [:MattN]]

(In reply to Jed Parsons (use request feedback, please) [:jedp, :jparsons] from comment #2)
> Aha - elm and pine branches?  I'll check them out.  Thanks.

It's in mozilla-central so it should be in all forks of it.

Comment 4

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

(In reply to Matthew N. [:MattN] (catching up on reviews) from comment #3)
> It's in mozilla-central so it should be in all forks of it.

Ahhh ... Sorry, I'm with you now.  Sleep-deprived.  :)  

Yes, absolutely, while I'm in toolkit/identity, I'll make all those updates.  I should have said in the Description that it has no *other* callers in Firefox.  I.e., we don't need to coordinate with other projects.

Comment 5

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

Attachment: Conform jwcrypto api to published spec - wip

wip - now provides these calls (almost) in accordance with the spec:

jwcrypto.generateKeypair
jwcrypto.assertion.sign
jwcrypto.cert.bundle

next: make keypair api as consistent as possible with the spec (starting with the return value of generateKeypair)

Comment 6

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

Attachment: Conform jwcrypto api to published spec - wip

jwcrypto.generateKeypair() now behaves and is named according to the spec.  It returns a keypair, not a wrapper object.

I've added two utility functions that are not in the spec, to keep life uncomplicated and pleasant for callers (RP, IdP modules).  They are:

  jwcrypto.serializePublicKey(kp) -> string
  jwcrypto.generateBackedAssertion(cert, kp, aud) -> cert~as.ser.tion

MattN, I've updated all the toolkit/identity code, and tests are all passing.  Do you have any concerns?

Warner, this isn't a complete implementation of all the jwcrypto functions, but I think it's all we need for Firefox Accounts at the moment.

Comment 7

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

Attachment: Conform jwcrypto api to published spec - wip

Comment 8

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

Attachment: Conform jwcrypto api to published spec - wip

Reverted name of generateBackedAssertion to generateAssertion.

I don't like the generateAssertion name, but I also don't want to mess with the original api function names.

Comment 9

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

Attachment: Conform jwcrypto api to published spec - wip

Sorry - uploaded the wrong patch last time - didn't have the change back to generateAssertion.  Think I got it this time :)

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 831900 [details] [diff] [review]
Conform jwcrypto api to published spec - wip

JWCrypto.serializePublicKey(), in the "default" clause, uses an undefined
"aCallback()" leftover from the async version. That should probably be
changed to throw an exception. Since the tests pass, we also need a test to
exercise this.

It looks like jwcrypto.sign() takes "aKeypair" (the result of
jwcrypto.generateKeypair), whereas in the node.js version it takes
"secretKey" (which is the .publicKey property of the result of
generateKeypair). I'm ok with divergence if you are. If you want to fix it,
you'll probably need to stick the .keyType on both the secret and public
objects.

JWCrypto.generateKeypair() takes the same arguments as the node.js version,
but to avoid confusion when comparing them, it might be good to name its
first argument "opts" instead of "algorithm" (since "algorithm" is also a
property name that's expected to be inside that argument).

In test_bundle(), it'd be nice to check that the second component is equal to
the signedAssertion string.

r=warner with those changes, specifically the "aCallback" leftover issue.

Comment 11

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

(In reply to Brian Warner [:warner :bwarner] from comment #10)
> Comment on attachment 831900 [details] [diff] [review]
> Conform jwcrypto api to published spec - wip
> 
> JWCrypto.serializePublicKey(), in the "default" clause, uses an undefined
> "aCallback()" leftover from the async version. That should probably be
> changed to throw an exception. Since the tests pass, we also need a test to
> exercise this.

Blech!  Thanks for catching that.

> It looks like jwcrypto.sign() takes "aKeypair" (the result of
> jwcrypto.generateKeypair), whereas in the node.js version it takes
> "secretKey" (which is the .publicKey property of the result of
> generateKeypair). I'm ok with divergence if you are. If you want to fix it,
> you'll probably need to stick the .keyType on both the secret and public
> objects.

Yes, I was frowning at that, too.  I'd like to fix it.

> JWCrypto.generateKeypair() takes the same arguments as the node.js version,
> but to avoid confusion when comparing them, it might be good to name its
> first argument "opts" instead of "algorithm" (since "algorithm" is also a
> property name that's expected to be inside that argument).

Good point.  Yes.

> In test_bundle(), it'd be nice to check that the second component is equal to
> the signedAssertion string.

Yes.

> r=warner with those changes, specifically the "aCallback" leftover issue.

Thanks for your review, Brian!

Comment 12

[Firefox Bug Husbandry Bot]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1497358; closing remaining open bugs in this component.