Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new data formats - jwcrypto 0.5.x #78

Merged
merged 1 commit into from Jul 28, 2014
Merged

new data formats - jwcrypto 0.5.x #78

merged 1 commit into from Jul 28, 2014

Conversation

@lloyd
Copy link
Contributor

@lloyd lloyd commented Dec 20, 2013

This implements almost the entirety of new data formats support describe in https://github.com/djc/id-specs/blob/prod/browserid/json-formats.md.

One tweak left is the changed property for pubkey. currently this work is published on npm under the dev tag.

Strategy here is this version of jwcrypto parses old or new formats, and always generates new formats. It should be integrated into:

  1. verifier
  2. all environments that generate browserid artifacts.

It must be done in this order.

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Dec 20, 2013

@fmarier you want to put a critical eye to this work? feel free to throw away the under thought, and merge with your in progress implementation as needed. love to review after that.


function extractAlg(obj) {
var origAlg = obj.algorithm || obj.kty;
var alg = ALIASES[origAlg] || origAlg;

This comment has been minimized.

@fmarier

fmarier Jan 14, 2014
Contributor

This is a nit, but if we're dealing with a new format object (i.e. it has kty) then we don't need to look for aliases.

This comment has been minimized.

@lloyd

lloyd Jan 15, 2014
Author Contributor

love it.

@@ -93,14 +95,14 @@ var CERTS = [
"email": "user@exampleidp.com",
"certifierPublicKey": {"algorithm":"RS","n":"14992830413702950214310095212044491259620359262383741324696388958190897089691526259734048412721912364240221301689826865084526414386073707804839978986676709963946069225361038897793675105866773424177081334731736862288361853661697790251045350661007112837048725805572051406892322870828536513516637815563734426985085169776230353505099335068959396036415837272499551706990150656379682592552383284722119011793645821942132094135988926383389368449569136547237729302181561293580509148227224997417221099523657138090327493805636962561720869470208329064396135474786068129369609669835010448697628273776942729022409017231229381035477","e":"65537"},
"containedPublicKey": {"algorithm":"DS","y":"be83f1cf4dc2569af822b6b5c541792e321d9c6194ea567142e9869e495bb25fbf49693c6f577efe162093ef27c7c550ec3d810bf005e523d3ee6c77346e05fdcd9adf584670b368005b1ed2e26adc0eef9b2a871d7d51e5601349822710f3028a58b5af45f72b8366f530936a98ca7eafc7ba31d6e13636365e113a0a5b626c","p":"ff600483db6abfc5b45eab78594b3533d550d9f1bf2a992a7a8daa6dc34f8045ad4e6e0c429d334eeeaaefd7e23d4810be00e4cc1492cba325ba81ff2d5a5b305a8d17eb3bf4a06a349d392e00d329744a5179380344e82a18c47933438f891e22aeef812d69c8f75e326cb70ea000c3f776dfdbd604638c2ef717fc26d02e17","q":"e21e04f911d1ed7991008ecaab3bf775984309c3","g":"c52a4a0ff3b7e61fdf1867ce84138369a6154f4afa92966e3c827e25cfa6cf508b90e5de419e1337e07a2e9e2a3cd5dea704d175f8ebf6af397d69e110b96afb17c7a03259329e4829b0d03bbc7896b15b4ade53e130858cc34d96269aa89041f409136c7242a38895c9d5bccad4f389af1d7a4bd1398bd072dffa896233397a"}
},
} /* ,

This comment has been minimized.

@fmarier

fmarier Jan 14, 2014
Contributor

Does this test need to be ported or removed?

delete params.publicKey;
certParams.principal = params.principal;
delete params.principal;
[ 'principal', 'sub' ].forEach(function(k) {

This comment has been minimized.

@fmarier

fmarier Jan 14, 2014
Contributor

Should we reject any certs that have both sub and principal?

This comment has been minimized.

@lloyd

lloyd Jan 15, 2014
Author Contributor

yes.

@fmarier
Copy link
Contributor

@fmarier fmarier commented Jan 14, 2014

There's something else about the transition to the new format that we have to think about: IdPs.

One way to phase it in would be:

  1. upgrade verifier to accept both formats
  2. upgrade IdPs to accept old-style and new-style certs but sign them using the new sig format
  3. upgrade the shim to generate new-style certs

Or we bundle 2 and 3 together and we have a flag day, but that will screw our existing IdPs.

Note: my patches don't necessarily take into account the above timeline, I thought of that after I was done writing these.

@fmarier
Copy link
Contributor

@fmarier fmarier commented Jan 14, 2014

Finally (and I should have started with this!), this pull request looks really good!

I like the way you detect the appropriate data format version. It's much better than my ill-fated attempt at preserving the existing versioning code in jwcrypto.

@seanmonstar seanmonstar merged commit 592cb54 into master Jul 28, 2014
1 check failed
1 check failed
@jaredhirsch
continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details
@seanmonstar seanmonstar deleted the 0.5.x branch Jul 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants