crypto: refactor crypto module by jasnell · Pull Request #15231 · nodejs/node · GitHub

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

Should this be labeled in progress?

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

ping @nodejs/crypto
ping @nodejs/tsc

@jasnell why is this semver-major?

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

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

I'd like to yes.

nit: use printf-like syntax?

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

Done!

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.

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

@targos .. updated!

Thanks. LGTM.

A CITGM run wouldn't hurt IMO.

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

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

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.

One question, otherwise LGTM.

Huh? Why?

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 :-)

@indutny and @addaleax ... updated!

LGTM

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

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

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>

Landed in c75f87c

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

panva mentioned this pull request Apr 20, 2026

Runtime-deprecate calling digest() on HMAC more than once #62838

Closed

	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 by jasnell · Pull Request #15231 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: refactor crypto module - #15231

crypto: refactor crypto module#15231jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:crypto-apijasnell/node:crypto-apiCopy head branch name to clipboard

Conversation

jasnell commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Sep 7, 2017

Uh oh!

jasnell commented Sep 7, 2017

Uh oh!

jasnell commented Sep 12, 2017

Uh oh!

mcollina commented Sep 12, 2017

Uh oh!

jasnell commented Sep 12, 2017

Uh oh!

targos commented Sep 12, 2017

Uh oh!

jasnell commented Sep 13, 2017

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!

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!

jasnell commented Sep 13, 2017

Uh oh!

targos commented Sep 13, 2017

Uh oh!

jasnell commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Sep 13, 2017

Uh oh!

jasnell commented Sep 13, 2017

Uh oh!

indutny left a comment

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!

jasnell Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jasnell commented Sep 14, 2017

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

crypto: refactor crypto module#15231
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:crypto-api

jasnell commented Sep 6, 2017 •
edited
Loading

jasnell commented Sep 13, 2017 •
edited
Loading

jasnell Sep 14, 2017 •
edited
Loading