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

(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.

(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.

(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.

(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.

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)

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.

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.

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

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.

(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!

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