crypto: refactor crypto module #15231

jasnell · 2017-09-06T20:35:46Z

Beginning of a larger bit of work on the crypto API and implementation.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

crypto

Trott · 2017-09-07T07:04:55Z

Should this be labeled in progress?

jasnell · 2017-09-07T07:06:20Z

haven't decided yet lol.... the rest of what I want to do here should likely go into a separate PR. It's rather extensive

jasnell · 2017-09-12T11:01:14Z

ping @nodejs/crypto
ping @nodejs/tsc

mcollina · 2017-09-12T15:00:39Z

@jasnell why is this semver-major?

jasnell · 2017-09-12T16:48:58Z

While moving things around it also updates to use internal/errors

targos · 2017-09-12T19:00:13Z

Is it something that you would like to land before Node 9?

jasnell · 2017-09-13T15:24:21Z

I'd like to yes.

targos · 2017-09-13T19:03:28Z

lib/internal/errors.js

@@ -236,6 +236,7 @@ E('ERR_NO_CRYPTO', 'Node.js is not compiled with OpenSSL crypto support');
 E('ERR_NO_ICU', '%s is not supported on Node.js compiled without ICU');
 E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported');
 E('ERR_OUTOFMEMORY', 'Out of memory');
+E('ERR_OUT_OF_RANGE', (name) => `The "${name}" argument is out of range`);


nit: use printf-like syntax?

targos · 2017-09-13T19:13:56Z

lib/internal/crypto/certificate.js

+
+Certificate.exportChallenge = exportChallenge;
+Certificate.exportPublicKey = exportPublicKey;
+Certificate.verifySpkac = verifySpkac;


We should probably document these and recommend their usage since it's useless to create an instance of the class. That can be done in another PR.

Might as well do it in this PR

targos · 2017-09-13T19:27:59Z

lib/internal/crypto/pbkdf2.js

+
+function _pbkdf2(password, salt, iterations, keylen, digest, callback) {
+
+  if (typeof digest !== 'string')


If I'm reading the C++ code correctly, with the previous undefined check, if a digest that is neither undefined nor a string is passed, a default value would be used. That includes null. Is this something that we want to break?

node/src/node_crypto.cc

Lines 5392 to 5403 in bf1ca8f

if (args[4]->IsString()) {

node::Utf8Value digest_name(env->isolate(), args[4]);

digest = EVP_get_digestbyname(*digest_name);

if (digest == nullptr) {

type_error = "Bad digest name";

goto err;

}

}

if (digest == nullptr) {

digest = EVP_sha1();

}

you're right. this should allow digest to be null so it will default. Will fix.

targos · 2017-09-13T19:36:03Z

lib/internal/errors.js

@@ -110,6 +110,8 @@ E('ERR_CHILD_CLOSED_BEFORE_REPLY', 'Child closed before reply received');
 E('ERR_CONSOLE_WRITABLE_STREAM',
  'Console expects a writable stream instance for %s');
 E('ERR_CPU_USAGE', 'Unable to obtain cpu usage %s');
+E('ERR_CRYPTO_ECDH_INVALID_FORMAT',
+  (format) => `Invalid ECDH format: ${format}`);


nit: use printf-like syntax?

I can, yeah, I tend to prefer this way but it's no biggie for me :-)

I don't mind keeping it this way if that's what you prefer 👍

nah... I'll change it. It's no biggie and I don't feel strongly about it

jasnell · 2017-09-13T21:23:09Z

@targos .. updated!

targos · 2017-09-13T21:30:15Z

Thanks. LGTM.

A CITGM run wouldn't hurt IMO.

jasnell · 2017-09-13T21:39:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10067/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/982/

jasnell · 2017-09-13T23:10:54Z

Not seeing anything of significant concern in CITGM. regular CI is still running.

jasnell · 2017-09-13T23:40:10Z

Btw, all... A big reason I'm going through all this is to work towards support for async crypto API variants. I will likely only pursue it, however, once async iterators have landed. The following is an example of what I'm looking to achieve:

const crypto = require('crypto');

async function hash(alg, data, enc) {
  const hash = crypto.createHash(alg);
  for await (const chunk of data) {
    hash.update(chunk);
  }
  return hash.digest(enc);
}

var n = 0;
async function* dataIterator() {
  while (n++ < 10)
    yield `testing${n}`;
}

const p = hash('sha256', dataIterator(), 'hex');
p.then(console.log).catch(console.error);

The async hash function above returns a Promise that uses an async iterator to feed data into the hash. async variants of hash, hmac, sign, verify, cipher and decipher are all trivially possible with this approach.

This, of course, does not really do much to help with performance on the TLS side, but it does provide a nice Promise-oriented API.

indutny

One question, otherwise LGTM.

indutny · 2017-09-13T23:54:28Z

src/node_crypto.cc

@@ -5568,7 +5568,9 @@ void RandomBytesCheck(RandomBytesRequest* req, Local<Value> (*argv)[2]) {
      memcpy(buf, data, req->size());
      (*argv)[1] = buffer;
    } else {
-      (*argv)[1] = Buffer::New(req->env(), data, size).ToLocalChecked();
+      auto cb = [](char* data, void* hint) {};
+      (*argv)[1] = Buffer::New(req->env(), data, size, cb, nullptr)


Because it segfaults with a double free without it!

so this particular line of code was not being exercised previously because the input was always a Uint8Array. The if check above would always be true. If any of thing is passed, however, the Buffer would segfault when it tried to GC due to a double free.

I will say that I'm not entirely sure this is the right approach to take on this! :-) (Copy might be better but I'm not entirely sure about that either)

(The challenge here, btw, is that we have two TypedArray instances being created that cover the same data space... thinking about it now, copying the data would be best.)

@jasnell What happens if the original Uint8Array is freed before this one?

Yep, that's the exact weirdness I'm looking at. Looking at it again, I've just changed the if block above to s/isUint8Array/isArrayBufferView and everything works as it should :-)

jasnell · 2017-09-14T14:20:05Z

@indutny and @addaleax ... updated!

indutny

LGTM

jasnell · 2017-09-15T19:37:54Z

Thank you @indutny and @targos ... I will land this on Monday if there are no objections!

jasnell · 2017-09-18T14:18:20Z

Preland CI: https://ci.nodejs.org/job/node-test-pull-request/10121/

jasnell · 2017-09-18T14:55:08Z

small linting error fixed. Landing shortly.

* Split single monolithic file into multiple * Make Certificate methods static * Allow randomFill(Sync) to use any ArrayBufferView * Use internal/errors throughout * Improve arg validation in Hash/Hmac * Doc updates

* Split single monolithic file into multiple * Make Certificate methods static * Allow randomFill(Sync) to use any ArrayBufferView * Use internal/errors throughout * Improve arg validation in Hash/Hmac * Doc updates PR-URL: #15231 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

jasnell · 2017-09-18T15:22:10Z

Landed in c75f87c

jasnell · 2017-09-18T15:23:10Z

The "notable change" here is that randomFill and randomFillSync can now fill any TypedArray with random data, it's not just limited to Buffer and Uint8Array any more.

* Split single monolithic file into multiple * Make Certificate methods static * Allow randomFill(Sync) to use any ArrayBufferView * Use internal/errors throughout * Improve arg validation in Hash/Hmac * Doc updates PR-URL: nodejs/node#15231 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Sep 6, 2017

jasnell requested review from indutny and shigeki September 13, 2017 16:40

targos reviewed Sep 13, 2017

View reviewed changes

jasnell force-pushed the crypto-api branch from 8987700 to 5e9f9ef Compare September 13, 2017 21:10

targos approved these changes Sep 13, 2017

View reviewed changes

jasnell requested a review from sam-github September 13, 2017 21:41

indutny suggested changes Sep 13, 2017

View reviewed changes

addaleax mentioned this pull request Sep 14, 2017

crypto: synchronise with doc #14805

Closed

3 tasks

indutny approved these changes Sep 15, 2017

View reviewed changes

jasnell mentioned this pull request Sep 18, 2017

errors crypto : migrate errors to internal/errors.js #15452

Closed

4 tasks

jasnell force-pushed the crypto-api branch from 155dcc3 to 8c76093 Compare September 18, 2017 14:52

crypto: refactor the crypto module

a42fc93

* Split single monolithic file into multiple * Make Certificate methods static * Allow randomFill(Sync) to use any ArrayBufferView * Use internal/errors throughout * Improve arg validation in Hash/Hmac * Doc updates

jasnell force-pushed the crypto-api branch from 8c76093 to a42fc93 Compare September 18, 2017 14:57

jasnell closed this Sep 18, 2017

jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label Sep 18, 2017

bengl mentioned this pull request Sep 28, 2017

crypto: expose ECDH class #8188

Closed

3 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged


		function _pbkdf2(password, salt, iterations, keylen, digest, callback) {

		if (typeof digest !== 'string')

	if (args[4]->IsString()) {
	node::Utf8Value digest_name(env->isolate(), args[4]);
	digest = EVP_get_digestbyname(*digest_name);
	if (digest == nullptr) {
	type_error = "Bad digest name";
	goto err;
	}
	}

	if (digest == nullptr) {
	digest = EVP_sha1();
	}

crypto: refactor crypto module #15231

crypto: refactor crypto module #15231

Conversation

jasnell commented Sep 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott commented Sep 7, 2017

jasnell commented Sep 7, 2017

jasnell commented Sep 12, 2017

mcollina commented Sep 12, 2017

jasnell commented Sep 12, 2017

targos commented Sep 12, 2017

jasnell commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 13, 2017

targos commented Sep 13, 2017

jasnell commented Sep 13, 2017 • edited Loading

jasnell commented Sep 13, 2017

jasnell commented Sep 13, 2017

indutny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Sep 14, 2017

indutny left a comment

Choose a reason for hiding this comment

jasnell commented Sep 15, 2017

jasnell commented Sep 18, 2017

jasnell commented Sep 18, 2017

jasnell commented Sep 18, 2017

jasnell commented Sep 18, 2017

jasnell commented Sep 6, 2017 •

edited

Loading

jasnell commented Sep 13, 2017 •

edited

Loading

jasnell Sep 14, 2017 •

edited

Loading