Native SPKAC support #6330

jas- · 2013-10-10T19:32:09Z

crypto: Native SPKAC support

Implements new class 'Certificate' within crypto object
for working with SPKAC's (signed public key & challenge)
natively.

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

Nodejs-Jenkins · 2013-10-10T19:32:13Z

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

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

trevnorris · 2013-10-10T20:05:38Z

@jas- ObjectWrap has been removed from core usage on master. I'll write another diff for you to apply and fix this issue.

trevnorris · 2013-10-10T20:31:13Z

@jas- Assuming your remote's name to joyent/node is upstream, do the following steps:

$ git checkout openssl-spkac
$ git fetch upstream
$ git reset --hard upstream/master
$ curl 'https://gist.github.com/trevnorris/6925027/raw/a6ba4075340a469cca0a1c993c8009991afde1d3/pr6330.patch' | git am
$ git push -f origin openssl-spkac

I've edited your patch to work on latest master. Once this is done, ping me and it should be ready for merge. Also, if you haven't, please sign the CLA as noted above.

jas- · 2013-10-10T20:54:05Z

@trevnorris how can I pay better attention to core API changes such as this in the future? I used the docs and current usage when creating a new branch based on master while tracking v.0.11.7-release and still missed that change. Thanks for all of your tips and assistance!

trevnorris · 2013-10-10T21:27:17Z

@jas- master is usually moving quickly, and most of it is internal. The best thing I've found is to do a git log -- <file> on the file I'll be making changes to. That'll let me know what's been going on most recently. Also, helping the community keep up is one reason we're here. :)

jas- · 2013-10-11T00:36:22Z

@trevnorris That is good information. Again I appreciate your help. Should I close the other open issue for this PR?

My old dev env got wiped out so I rebuilt it from scratch. Although I used the same branch name after forking node core it didn't update the existing PR. There is still quite a bit I need to learn about git.

bnoordhuis · 2013-10-11T07:03:11Z

src/node_crypto.cc

s/String::New/FIXED_ONE_BYTE_STRING/

Should I use the node_isolate arg? (ie FIXED_ONE_BYTE_STRING(node_isolate, "Certificate"))

Fixed. Thanks.

bnoordhuis · 2013-10-15T07:48:48Z

src/node_crypto.cc

i = NETSCAPE_SPKI_verify(spki, pkey) > 0;? Right now, VerifySpkac() returns true when NETSCAPE_SPKI_verify() returns -1.

Doh! Fixed, thanks.

bnoordhuis · 2013-10-15T07:57:05Z

Last round of comments. If you fix these up, I'm pretty sure we're there. :-)

bnoordhuis · 2013-10-15T10:32:18Z

src/node_crypto.h

Sorry, maybe I didn't make myself clear: the thing is that the len parameter is never < 0, right? So in that case it could (and arguably should) be an unsigned int. The bool return value was perfectly okay, I had no beef with that.

Whoops, fixed. Thanks.

bnoordhuis · 2013-10-15T14:07:42Z

Thanks Jason, landed in 7f66e44. Feel like writing documentation for it?

jas- · 2013-10-15T14:09:44Z

@bnoordhuis Sure, example app too?

isaacs · 2013-10-15T23:00:17Z

This fails its test on OS X and SmartOS. Reverted on d9b4cc3 and a555992.

That being said, I would love to see this functionality in Node, and have no objection to it. Reopening this pull req so we can investigate.

isaacs · 2013-10-15T23:04:52Z

I'm very rarely getting garbage characters after the result. Added some logging in the test:

console.error('%j', stripLineEndings(certificate.exportPublicKey(spkacValid).toString('utf8')));
console.error('%j', stripLineEndings(spkacPem.toString('utf8')));

and I see this:

"-----BEGIN PUBLIC KEY-----MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDcvQh9SKOPv4DwI8LwSaFx02h7l9QCiDs6sF2GfsSTEUG61SnjQ/v4uJiLKBgbVOagj9rkSCwtTez23ATPeGaBj2Zgipv+tv5IXyqUP8ropXJQ5ELtaXPUN/gvw7cO5EbPHr/7eMhbpw8Gl+AfWxW5hLW8MGw/+AwwjHBOwong/QIDAQAB-----END PUBLIC KEY-----�\u0002\u000e\u0010"
"-----BEGIN PUBLIC KEY-----MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDcvQh9SKOPv4DwI8LwSaFx02h7l9QCiDs6sF2GfsSTEUG61SnjQ/v4uJiLKBgbVOagj9rkSCwtTez23ATPeGaBj2Zgipv+tv5IXyqUP8ropXJQ5ELtaXPUN/gvw7cO5EbPHr/7eMhbpw8Gl+AfWxW5hLW8MGw/+AwwjHBOwong/QIDAQAB-----END PUBLIC KEY-----"

jas- · 2013-10-16T11:04:22Z

@isaacs I have come across this in more extensive testing as well. @bnoordhuis I alleviated this problem using the buf[ptr->length - 1] = '\0'; when exporting the public key.

I am trying to track this down in the OpenSSL libs, I believe it to be stemming from the BIO_get_mem_ptr() call possibly leaking extraneous memory. I suppose I could look into possibly removing this call and using the lower level BIO_ctrl().

isaacs · 2013-10-16T15:13:57Z

@jas- Awesome, thanks for continuing to plug away on it.

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

jas- · 2013-10-16T15:28:33Z

@isaacs I think I may be it resolved, can you test? I am not seeing it now.

Architecture: Linux node.dev 2.6.32-358.18.1.el6.x86_64 #1 SMP Wed Aug 28 17:19:38 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

Node: v0.11.8-pre

It seems that using BIO_write(bio, "\0", 1) I can set the pointer referenced within the PEM_write_bio_PUBKEY() return struct vs. attempting to overwrite the last bit using the buf[ptr->length - 1] = '\0'; as to not introduce an underflow problem.

Ah OpenSSL how I love digging through your BIO_ functions.

trevnorris · 2013-10-16T16:30:43Z

All tests are passing. I'm going to re-merge this and the docs fix. Nice job @jas-.

jas- · 2013-10-16T16:45:56Z

@trevnorris No problem, hope it solves it. Perhaps I should put together a more robust test case?

trevnorris · 2013-10-16T16:55:22Z

Was merged again in 7bf46ba

isaacs · 2013-10-16T17:18:53Z

Thanks!

jas- · 2013-10-16T19:29:08Z

@isaacs, @bnoordhuis & @trevnorris I realize you have already merged this but as it is my first submission and I already feel like I made a few rookie mistakes and wanted a more robust test case I whipped this up based on SPKAC's generated from multiple private keys of various size as well as various hashing algorithms.

Also I have been lurking on IRC as I wanted to ask if it would be a beneficial feature to generate new SPKAC (useful for OAuth server -> server auth cases) as well as certificate generation (CSR's come to mind).

In any event, thanks for all of the pointers too.

bnoordhuis · 2013-10-16T20:23:37Z

@jas- If you have a better test case, well, you know, we take patches. ;-)

jas- · 2013-10-16T20:32:14Z

@bnoordhuis Fair enough.

bnoordhuis reviewed Oct 11, 2013
View reviewed changes

This was referenced Oct 14, 2013

Native SPKAC support #5604

Closed

SPKAC support JoshKaufman/ursa#25

Closed

bnoordhuis reviewed Oct 15, 2013
View reviewed changes

bnoordhuis closed this Oct 15, 2013

isaacs reopened this Oct 15, 2013

crypto: Native SPKAC support

9f06c07

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

trevnorris closed this Oct 16, 2013

Uh oh!

Native SPKAC support #6330

Native SPKAC support #6330

Uh oh!

Conversation

jas- commented Oct 10, 2013

Uh oh!

Nodejs-Jenkins commented Oct 10, 2013

Uh oh!

trevnorris commented Oct 10, 2013

Uh oh!

trevnorris commented Oct 10, 2013

Uh oh!

jas- commented Oct 10, 2013

Uh oh!

trevnorris commented Oct 10, 2013

Uh oh!

jas- commented Oct 11, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 15, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Oct 15, 2013

Uh oh!

jas- commented Oct 15, 2013

Uh oh!

isaacs commented Oct 15, 2013

Uh oh!

isaacs commented Oct 15, 2013

Uh oh!

jas- commented Oct 16, 2013

Uh oh!

isaacs commented Oct 16, 2013

Uh oh!

jas- commented Oct 16, 2013

Uh oh!

trevnorris commented Oct 16, 2013

Uh oh!

jas- commented Oct 16, 2013

Uh oh!

trevnorris commented Oct 16, 2013

Uh oh!

isaacs commented Oct 16, 2013

Uh oh!

jas- commented Oct 16, 2013

Uh oh!

bnoordhuis commented Oct 16, 2013

Uh oh!

jas- commented Oct 16, 2013

Uh oh!

Uh oh!