Native SPKAC support #5604

jas- · 2013-05-30T15:14:24Z

crypto: Implements new class 'Certificate' within crypto object
with (as of this commit) three new functions for working
with SPKAC's. Issue #5546 as discussed with @bnoordhuis

bool Certificate::verifySpkac(buffer)
buffer Certificate::exportPublicKey(buffer)
buffer Certificate::exportChallenge(buffer)

Also includes tests

Nodejs-Jenkins · 2013-05-30T15:14:29Z

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit jas-/node@2f80449b8e390e6045237f6e90c430818be18e91 has the following error(s):

Commit message must indicate the subsystem this commit changes

The following commiters were not found in the CLA:

jas-

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

isaacs · 2013-06-01T05:46:33Z

test/simple/test-crypto.js

 assert.deepEqual(dh1.getPrime(), dh3.getPrime());
 assert.deepEqual(dh1.getGenerator(), dh3.getGenerator());
 assert.deepEqual(dh1.getPublicKey(), dh3.getPublicKey());
 assert.deepEqual(dh1.getPrivateKey(), dh3.getPrivateKey());
-
+console.log(dh1.getPrivateKey());
+console.log(dh3.getPrivateKey());


Extraneous?

Apologies, I will remove this (I was looking into the assert.deepEqual() function regarding buffer comparisions). That should not have been in the pr

isaacs · 2013-06-01T05:50:54Z

There's some style issues, and the binding should accept strings with an encoding, as well as buffers, and then pull the data out in C++. (Really, all of crypto should use the StringBytes api, but some of it is old and grimy.) It's late here, and I haven't reviewed it very carefully, so there may be other issues.

As for the functionality itself, it seems useful enough to me, and doing this in userland is a huge PITA, so it kind of makes sense. @bnoordhuis, @indutny: what do you think?

isaacs · 2013-06-01T05:52:34Z

@jas- Also, please go to http://nodejs.org/cla.html and click the buttons to appease the legal gods. Thanks :)

trevnorris · 2013-06-01T07:53:41Z

@jas- also you'll need to redo all your commit messages (remove the time stamp and prepend crypto:). just because so many contributors seem to have the problem of closing the current request and opening a new one, I'd like to mention that all commit messages can be changed by using git rebase -i master (with the branch checked out).

trevnorris · 2013-06-01T07:54:54Z

lib/crypto.js

@@ -577,7 +577,26 @@ function pbkdf2(password, salt, iterations, keylen, callback) {
  }
 }

+exports.Certificate = exports.Certificate = Certificate;


am I missing why the double assignation?

Mistake, thanks

jas- · 2013-06-03T18:57:11Z

@isaacs I filed out the required docs as asked.

@trevnorris I did this over the weekend git rebase -i master to :greplace 's/\w+\-/crypto\:/g' but it did not seem to modify the previous commit messages.

trevnorris · 2013-06-03T20:36:42Z

@jas- no other commands should be run with git rebase -i master. Make sure you have the topic branch checked out and run it. Here's an example output of a branch I'm working on:

pick 005130c smalloc: initial implementation
pick a693dee smalloc: add api to manually dispose Persistent
pick 5ec9a29 buffer: use smalloc as backing data store
pick cf80cf5 buffer: reimplement Buffer pools
pick fe0857b buffer: expose class methods alloc and dispose
pick 30651ed buffer: remove c-style casts
pick 3e86f56 buffer: deprecate legacy code
pick 124916b buffer: implement new fill behavior
pick 01b12a3 src: add node_isolate's from stable merge
pick 9e78622 smalloc: implement new v8 api

# Rebase 2900f07..9e78622 onto 2900f07
#
# Commands:
#  p, pick = use commit
#  r, reword = use commit, but edit the commit message
#  e, edit = use commit, but stop for amending
#  s, squash = use commit, but meld into previous commit
#  f, fixup = like "squash", but discard this commit's log message
#  x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out

From here switch all pick to reword, or r. Then it'll bring up all your commit messages and allow you to rewrite them.

trevnorris · 2013-06-03T20:37:59Z

Also, you'll need to ditch the Thu May 30 15:53:01 MDT 2013 in your commit headers. That'll put it over 40 chars.

jas- · 2013-06-04T03:21:28Z

Well git log shows the changes but github does not seem to reflect them. Thanks for the rebase tutorial!

trevnorris · 2013-06-04T16:26:59Z

@jas- ok. then you'll need to force push by git push -f origin openssl-spkac. that will override the remote changes w/ your local changes.

jas- · 2013-06-05T10:56:50Z

That did it. Thanks. I squashed all previous commits for brevity.

trevnorris · 2013-06-10T16:36:21Z

@isaacs commit format lgtm, but want sign off on the crypto aspect.

/cc @indutny

bnoordhuis · 2013-06-10T21:43:56Z

src/node_crypto.cc

+  HandleScope scope(node_isolate);
+
+  EVP_PKEY *pkey = NULL;
+  NETSCAPE_SPKI *spki = NULL;


Style: type*, not type *

trevnorris · 2013-06-19T19:33:29Z

src/node_crypto.cc

+  Certificate* certificate = ObjectWrap::Unwrap<Certificate>(args.This());
+
+  ASSERT_IS_BUFFER(args[0]);
+  ssize_t length = Buffer::Length(args[0]);


returns size_t.

trevnorris · 2013-06-19T19:49:03Z

@jas- left a few comments. post again when they're cleaned up (github doesn't alert on force-push).

jas- · 2013-06-20T10:59:04Z

@trevnorris Modifications complete.

trevnorris · 2013-06-20T18:34:13Z

@jas- github is being really strange. just to double check, (assuming you have openssl-spkac checked out) did you git push -f origin openssl-spkac?

trevnorris · 2013-06-20T18:35:42Z

wait. seems you did. really strange. the file changes you pushed aren't showing up on the files tab, but the new commit shows up on the commit tab.

jas- · 2013-06-20T18:41:20Z

@trevnorris It is the timestamp from the development server that affected that

trevnorris · 2013-06-20T18:57:54Z

ah, there we go.

@jas- ok. just a few more. (just trying to foresee anything @bnoordhuis might bring up. :)

bnoordhuis · 2013-06-20T20:44:09Z

src/node_crypto.cc

+
+  pkey = X509_PUBKEY_get(spki->spkac->pubkey);
+  if (pkey == NULL)
+    return 0;


Call NETSCAPE_SPKI_free() before returning.

I am embarrassed that I forgot this

Implements new class 'Certificate' within crypto object for working with SPKAC's (signed public key & challenge) bool Certificate::verifySpkac(buffer) buffer Certificate::exportPublicKey(buffer) buffer Certificate::exportChallenge(buffer)

trevnorris · 2013-07-15T17:50:37Z

The cc api has changed a bit since the v8 3.20 upgrade. Doesn't take much to correct, if you know the new API. Went ahead and did that for you. Also fixed a couple trailing white space, long line issues. Just run the following:

curl 'https://gist.github.com/trevnorris/6001911/raw/dc829768d2bfb56be82f7f7d3d5be29e0053f030/pr5604.diff' | git am

Then go ahead and --amend the commit and force push. Ping after you've done that.

Also for future reference, there's no need to place a HandleScope in a function that isn't working with any Locals. Reason for the HandleScope is to prevent GC from cleaning up anything you're actively working on.

bmatusiak · 2013-09-30T07:39:52Z

+1

bnoordhuis · 2013-10-08T14:30:35Z

@jas- You still want to go through with this PR? It probably needs a little rebasing on top of Trevor's patch but IIRC it was close to finished last time I looked at it.

jas- · 2013-10-08T14:59:29Z

@bnoordhuis Hello, yes I do. I will rebase it on top of @trevnorris's patch as suggested. Sorry about the delay.

jas- · 2013-10-14T21:21:39Z

I am closing this request. Any future references should refer to pull request #6330

isaacs reviewed Jun 1, 2013
View reviewed changes

trevnorris reviewed Jun 1, 2013
View reviewed changes

bnoordhuis reviewed Jun 10, 2013
View reviewed changes

trevnorris reviewed Jun 19, 2013
View reviewed changes

bnoordhuis reviewed Jun 20, 2013
View reviewed changes

crypto: Native SPKAC support

61168e6

Implements new class 'Certificate' within crypto object for working with SPKAC's (signed public key & challenge) bool Certificate::verifySpkac(buffer) buffer Certificate::exportPublicKey(buffer) buffer Certificate::exportChallenge(buffer)

jas- mentioned this pull request Oct 11, 2013

Native SPKAC support #6330

Closed

jas- closed this Oct 14, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native SPKAC support #5604

Native SPKAC support #5604

jas- commented May 30, 2013

Nodejs-Jenkins commented May 30, 2013

isaacs Jun 1, 2013

jas- Jun 2, 2013

isaacs commented Jun 1, 2013

isaacs commented Jun 1, 2013

trevnorris commented Jun 1, 2013

trevnorris Jun 1, 2013

jas- Jun 2, 2013

jas- commented Jun 3, 2013

trevnorris commented Jun 3, 2013

trevnorris commented Jun 3, 2013

jas- commented Jun 4, 2013

trevnorris commented Jun 4, 2013

jas- commented Jun 5, 2013

trevnorris commented Jun 10, 2013

bnoordhuis Jun 10, 2013

trevnorris Jun 19, 2013

trevnorris commented Jun 19, 2013

jas- commented Jun 20, 2013

trevnorris commented Jun 20, 2013

trevnorris commented Jun 20, 2013

jas- commented Jun 20, 2013

trevnorris commented Jun 20, 2013

bnoordhuis Jun 20, 2013

jas- Jun 22, 2013

trevnorris commented Jul 15, 2013

bmatusiak commented Sep 30, 2013

bnoordhuis commented Oct 8, 2013

jas- commented Oct 8, 2013

jas- commented Oct 14, 2013

Native SPKAC support #5604

Native SPKAC support #5604

Conversation

jas- commented May 30, 2013

Nodejs-Jenkins commented May 30, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacs commented Jun 1, 2013

isaacs commented Jun 1, 2013

trevnorris commented Jun 1, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jas- commented Jun 3, 2013

trevnorris commented Jun 3, 2013

trevnorris commented Jun 3, 2013

jas- commented Jun 4, 2013

trevnorris commented Jun 4, 2013

jas- commented Jun 5, 2013

trevnorris commented Jun 10, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jun 19, 2013

jas- commented Jun 20, 2013

trevnorris commented Jun 20, 2013

trevnorris commented Jun 20, 2013

jas- commented Jun 20, 2013

trevnorris commented Jun 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevnorris commented Jul 15, 2013

bmatusiak commented Sep 30, 2013

bnoordhuis commented Oct 8, 2013

jas- commented Oct 8, 2013

jas- commented Oct 14, 2013