upgraded to new jwcrypto API, including backend, frontend, and tests. #1533

Merged
merged 4 commits into from May 4, 2012

Projects

None yet

4 participants

@benadida
Contributor
benadida commented May 2, 2012

plus conformance tests.

@lloyd lloyd commented on the diff May 3, 2012
bin/verifier
@@ -88,7 +88,7 @@ app.post('/verify', function(req, resp, next) {
} catch (e) {
return resp.json({ status: "failure", reason: "Content-Type expected to be one of: " + want_ct.join(", ") }, 415);
}
- return resp.json({ status: "failure", reason: "need assertion and audience" }, 400);
+ return resp.json({ status: "failure", reason: "need assertion and audience"}, 400);
@lloyd
lloyd May 3, 2012 Contributor

awww. I liked that space. (NOTE: this is funny)

@lloyd lloyd commented on the diff May 3, 2012
lib/keysigner/ca.js
@@ -4,15 +4,18 @@
// certificate authority
-var jwcert = require('jwcrypto/jwcert'),
- jwk = require('jwcrypto/jwk'),
- jws = require('jwcrypto/jws'),
+var jwcrypto = require('jwcrypto'),
+ cert = jwcrypto.cert,
@lloyd
lloyd May 3, 2012 Contributor

+1 on a single entry point

@lloyd lloyd commented on the diff May 3, 2012
lib/keysigner/ca.js
path = require("path"),
fs = require("fs"),
secrets = require('../secrets.js'),
logger = require('../logging.js').logger,
urlparse = require('urlparse');
+// load up the right algorithms
+require("jwcrypto/lib/algs/rs");
+require("jwcrypto/lib/algs/ds");
@lloyd
lloyd May 3, 2012 Contributor

this seems confusing and magical to me. Why do you need to explicitly require algorithms to load them? Could this possibly happen on demand?

@benadida
benadida May 3, 2012 Contributor

I agree that this should be fixed, I don't have an immediately simple way to do this that doesn't cause browserify to bloat the bid-bundle, and the goal was to keep that minimal. Are you okay with my delaying a fix for this until the next rev?

@lloyd
lloyd May 4, 2012 Contributor

Yeah, this needent block merge

@lloyd lloyd and 1 other commented on an outdated diff May 3, 2012
lib/keysigner/ca.js
if (expiration == null)
- throw "expiration cannot be null";
- return new jwcert.JWCert(hostname, expiration, new Date(), publicKey, {email: email}).sign(secret_key);
+ return cb("expiration cannot be null");
+
+ cert.sign(publicKey, {email: email},
+ {iss: hostname, iat: new Date(), exp: expiration},
@lloyd
lloyd May 3, 2012 Contributor

(nitpick warning) unfortunate that iss, iat, which are arguably illegible protocol details must be known by the user of the api.

@benadida
benadida May 3, 2012 Contributor

I tried initially to make those not map to the spec, but it turned out to be harder to code against, in my opinion. If you feel very strongly, I can try again.

@lloyd lloyd commented on the diff May 3, 2012
lib/keysigner/ca.js
exports.parsePublicKey = parsePublicKey;
-exports.parseCert = parseCert;
exports.PUBLIC_KEY = public_key;
@lloyd
lloyd May 3, 2012 Contributor

not all of this code exported is run in production. there are some comments in the code that are disconcerting when you don't know that (it took me a while to figure out that verifyChain is only used by our own unit tests. The comment that concerned me was for now we only do our own browserid.org keys.

not a regression, shouldn't block the party.

@benadida
benadida May 3, 2012 Contributor

as it turns out, verifyChain was already there and not used, I noticed the same thing. I chose not to take it out yet. Agree that cleaning up dead code is something we should do. Do we have an easy way to detect dead code?

@lloyd
lloyd May 4, 2012 Contributor

I know of no easy/automated way.

@lloyd lloyd and 1 other commented on an outdated diff May 3, 2012
lib/static_resources.js
@@ -19,7 +19,8 @@ var common_js = [
'/lib/jquery-1.7.1.min.js',
'/lib/winchan.js',
'/lib/underscore-min.js',
- '/lib/vepbundle.js',
+// '/lib/vepbundle.js',
@lloyd
lloyd May 3, 2012 Contributor

why comment? delete!

@benadida
benadida May 3, 2012 Contributor

agreed, I'll fix this.

@lloyd lloyd commented on an outdated diff May 3, 2012
lib/static_resources.js
@@ -118,7 +119,8 @@ exports.resources = {
'/lib/jschannel.js',
'/lib/winchan.js',
'/lib/underscore-min.js',
- '/lib/vepbundle.js',
+// '/lib/vepbundle.js',
+ '/lib/bidbundle-min.js',
@lloyd
lloyd May 3, 2012 Contributor

ditto

@lloyd
Contributor
lloyd commented May 3, 2012

tests fail. both locally for me and travis agrees: http://travis-ci.org/#!/mozilla/browserid/builds/1231601

@lloyd lloyd commented on an outdated diff May 3, 2012
lib/verifier/certassertion.js
// audience must match!
- var err = compareAudiences(tok.audience, audience)
+ var err = compareAudiences(payload.aud, audience)
@lloyd
lloyd May 3, 2012 Contributor

.aud is a bit of a readability loss by clients of the lib.

@lloyd lloyd commented on an outdated diff May 3, 2012
package.json
"mustache": "0.3.1-dev",
+ "jwcrypto": "https://github.com/mozilla/jwcrypto/tarball/978614f01c17739b507",
@lloyd
lloyd May 3, 2012 Contributor

jwcrypto 0.2.0 is published, we can update to that.

@lloyd lloyd commented on the diff May 3, 2012
resources/static/shared/network.js
@@ -47,7 +47,8 @@ BrowserID.Network = (function() {
// seed the PRNG
// FIXME: properly abstract this out, probably by exposing a jwcrypto
// interface for randomness
- require("./libs/all").sjcl.random.addEntropy(result.random_seed);
+ // require("./libs/all").sjcl.random.addEntropy(result.random_seed);
+ // FIXME: this wasn't doing anything for real, so commenting this out for now
@lloyd
lloyd May 3, 2012 Contributor

if we don't want to block the merge on this (technically not a regression), let's open an issue to track?

@lloyd lloyd commented on the diff May 3, 2012
resources/static/shared/provisioning.js
@@ -69,8 +69,10 @@ BrowserID.Provisioning = (function() {
});
chan.bind('genKeyPair', function(trans, s) {
- keypair = jwk.KeyPair.generate("DS", BrowserID.KEY_LENGTH);
- return keypair.publicKey.toSimpleObject();
+ trans.delayReturn(true);
@lloyd
lloyd May 3, 2012 Contributor

author of jschannel is a doofus, he shoulda taken a page from vows, perhaps.

@lloyd lloyd commented on an outdated diff May 3, 2012
resources/static/shared/storage.js
@@ -111,9 +111,12 @@ BrowserID.Storage = (function() {
storage.tempKeypair = null;
if (raw_kp) {
prepareDeps();
- var kp = new jwk.KeyPair();
- kp.publicKey = jwk.PublicKey.fromSimpleObject(raw_kp.publicKey);
- kp.secretKey = jwk.SecretKey.fromSimpleObject(raw_kp.secretKey);
+
+ // WORK HERE
@lloyd
lloyd May 3, 2012 Contributor

let's improve this comment. it's ambiguous and could be a merge blocker.

@lloyd lloyd commented on the diff May 3, 2012
resources/static/shared/user.js
@@ -36,14 +33,14 @@ BrowserID.User = (function() {
// if the certificate expires in less that 5 minutes from now.
function isExpired(cert) {
// if it expires in less than 2 minutes, it's too old to use.
- var diff = cert.expires.valueOf() - serverTime.valueOf();
+ var diff = cert.payload.exp.valueOf() - serverTime.valueOf();
@lloyd
lloyd May 3, 2012 Contributor

ok, third times the charm, then I shut up. Being a crypto idiot, this coule be the exponent of the payload, it could be an expiration date. Now I'm done harping on this.

@callahad
callahad May 3, 2012 Member

+1 to @lloyd's comment. .expires or .expiry would be more immediately comprehensible. Though I missed the fact that the abbreviation maps to the spec at https://developers.google.com/payment-express/api-reference, which is a mitigating factor...

@lloyd lloyd commented on an outdated diff May 3, 2012
resources/static/shared/user.js
@@ -929,10 +925,12 @@ BrowserID.User = (function() {
*/
syncEmailKeypair: function(email, onComplete, onFailure) {
prepareDeps();
- var keypair = jwk.KeyPair.generate("DS", bid.KEY_LENGTH);
- setTimeout(function() {
- certifyEmailKeypair(email, keypair, onComplete, onFailure);
- }, 0);
+ jwcrypto.generateKeypair({algorithm: "DS", keysize: bid.KEY_LENGTH}, function(err, keypair) {
+ // FIXME: eventually we can get rid of setTimeout since it's already a callback
@lloyd
lloyd May 3, 2012 Contributor

can we get rid of it now, or is does generateKeypair invoke callback before function completion?

@lloyd lloyd and 1 other commented on an outdated diff May 3, 2012
resources/static/shared/user.js
// yield!
setTimeout(function() {
// assertions are valid for 2 minutes
var expirationMS = serverTime.getTime() + (2 * 60 * 1000);
var expirationDate = new Date(expirationMS);
- var tok = new jwt.JWT(null, expirationDate, audience);
-
- // yield!
- setTimeout(function() {
- assertion = vep.bundleCertsAndAssertion([idInfo.cert], tok.sign(sk), true);
- storage.site.set(audience, "email", email);
- complete(assertion);
- }, 0);
+
+ jwcrypto.assertion.sign(
@lloyd
lloyd May 3, 2012 Contributor

followon from previous comment. is assertion.sign() guaranteed to invoke its callback argument after it returns? if not, then this is a regression that could cause the "javascript is taking too long" errors we've seen on slow browsers (read: IE8). Given our reduction of key length, this might no longer be an issue, worth the comment tho.

my preference is to have all apis in jwcrypto guarantee that callbacks are not invoked before the function that they're passed to returns.

@benadida
benadida May 3, 2012 Contributor

that's not currently the case. Sounds like you consider this a blocker. Will work on this.

@lloyd lloyd and 1 other commented on an outdated diff May 3, 2012
resources/static/shared/user.js
// yield!
setTimeout(function() {
// assertions are valid for 2 minutes
var expirationMS = serverTime.getTime() + (2 * 60 * 1000);
var expirationDate = new Date(expirationMS);
- var tok = new jwt.JWT(null, expirationDate, audience);
-
- // yield!
- setTimeout(function() {
- assertion = vep.bundleCertsAndAssertion([idInfo.cert], tok.sign(sk), true);
- storage.site.set(audience, "email", email);
- complete(assertion);
- }, 0);
+
+ jwcrypto.assertion.sign(
+ {aud: audience}, {exp: expirationDate},
+ sk,
+ function(err, signedAssertion) {
+ setTimeout(function() {
@lloyd
lloyd May 3, 2012 Contributor

followup to my followup, what's the utility of setTimeout here?

@benadida
benadida May 3, 2012 Contributor

just leftover from when calls were synchronous.

@lloyd lloyd commented on the diff May 3, 2012
tests/verifier-test.js
wsapi.post('/verify', {
audience: TEST_ORIGIN,
assertion: assertion
}).call(this);
},
- "to return a clear error message": function (err, r) {
+ "to succeed": function (err, r) {
var resp = JSON.parse(r.body);
assert.strictEqual(resp.status, 'okay');
assert.strictEqual(resp.issuer, "example.domain");
@lloyd
lloyd May 3, 2012 Contributor

awesome updates to test coverage. I did a cursory sampling, but love how many new areas we're testing.

@lloyd
Contributor
lloyd commented May 3, 2012

tl;dr; r-

Stuff that I think should block the merge:

  1. tests must pass
  2. all asynchronous functions exposed by jwcrypto should guarantee not to invoke callbacks before they return - and we should remove setTimeouts in higher level code.
  3. comment which says "WORK HERE" should be updated by @benadida - it's either a critical oversight or a future improvement, i can't tell.
  4. we should update to jwcrypto 0.2.0 in package.json - npm does horribly with URLs and 0.2.0 is published
  5. future improvements that are called out in comments should probably be turned into issues at the time we merged so we can triage and they don't get lost.

Non-critical stuff that lloyd cares about but shouldn't block the merge:

  1. ideally jwcrypto would abstract us from three letter abbreviations
  2. remove comments that pertain to a previous version of the code
  3. for keysigner, because the code is so high value, I'd like to see us only have code that runs in production under lib/keysigner and move everything used in testing out to the unit tests. makes security review easier and more focused.
  4. require("jwcrypto/lib/algs/rs"); to add supported algorithms is not intuitive to this reader, and occurs in our code in at least three places. what happens if I forget to do that?

hope this is useful review! holler where I can help implement to get this merged.

@benadida benadida was assigned May 3, 2012
@benadida
Contributor
benadida commented May 4, 2012

comments 1, 2, 3, and 5 should be addressed. 4 I will address once jwcrypto PR is merged.
Also addressed non-critical issue 2.

Now working on non-critical issues 1 and 3. I don't think 4 will be addressed in this PR, but I'll try.

@benadida
Contributor
benadida commented May 4, 2012

ok, choosing to NOT address non-critical issue 3. because that issue was not introduced by this PR. We should fix it, so here's an issue to track it:

#1541

but no need to bundle it with this PR.

@benadida benadida upgraded to new jwcrypto API, including backend, frontend, and tests.
added conformance tests.

tests should now pass
updated to jwcrypto that has proper callback delay guarantees and removed unnecessary setTimeouts

Updated all calls to jwcrypto to use the more intuitive API. Fixed a front-end test that was failing due to true asynchronicity of jwcrypto.
ac121c8
@benadida
Contributor
benadida commented May 4, 2012

non-critical issue 1 is now taken care of. I believe this is now ready for review. @lloyd have at it!

@lloyd
Contributor
lloyd commented on ac121c8 May 4, 2012

r+ upon reading. performing some manual local testing, pending that, merging.

@lloyd
Contributor
lloyd commented May 4, 2012

two remaining problems:

  1. pull request won't merge cleanly, we need to update it.
  2. tests still don't pass, neither locally nor on travis: http://travis-ci.org/#!/mozilla/browserid/jobs/1246959

If you haven't set up mysql locally, you wouldn't have seen this.

@lloyd
Contributor
lloyd commented May 4, 2012

ok, if these tests pass: http://travis-ci.org/#!/mozilla/browserid/builds/1248922

and if @benadida reviews f45689f and approves, then I say r+, let's merge.

@ozten
Member
ozten commented May 4, 2012

Thanks for merging, delegation stuff looks like a good merge.

@benadida
Contributor
benadida commented May 4, 2012

ok, merging!

@benadida benadida merged commit dd26f97 into dev May 4, 2012
@benadida benadida removed their assignment Jan 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment