crypto: add randomFill and randomFillSync by evanlucas · Pull Request #10209 · nodejs/node · GitHub

Checklist

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

Affected core subsystem(s)

crypto

Description of change

crypto.randomFill and crypto.randomFillSync are similar to
crypto.randomBytes, but allow passing in a buffer as the first
argument. This allows us to reuse buffers to prevent having to
create a new one on every call.

Shouldn't the int64_t in these related Random* functions be size_t instead?

hm, it was changed from a uint32_t in 944f680. @trevnorris?

No strong opinion, but what about Buffer.randomFill() and Buffer.randomFillSync()? Or crypto.randomFill(buffer) (with a Sync version).

Also, while I appreciate you not wanting to commit too much work to something before you know it is getting a warm receiption, its hard to get meaningful comment on an API addition if you don't provide API documentation in the PR, you can only get feedback from those willing to wade through the C++.

And I'm generally +1 on the idea, seems reasonable.

Yeah, I agree, it would probably be easier to have this as its own API, too.

Would there be any reason not to fill the passed buffer directly instead of copying?

Without doing a copy like this, I couldn't get it to work for some reason. I'll dig more into it though.

I think I would prefer to have a separate API for something like this because this prevents feature detection in a way

This would work, wouldn't it?

let supported = true;
try {
  crypto.randomBytes(new Buffer(1));
} catch (e) {
  supported = !(e instanceof TypeError);
}

I have a mild preference for extending the existing API.

That said, the API should ideally also take an offset and a length so you can read into a buffer at a specific location. That might get awkward to retrofit onto the existing API.

On the other hand: if the first argument is a buffer, it's unambiguous what version of the API you are trying to invoke.

Yes, allowing an offset would be even better.

@evanlucas @mscdex I was doing some forward proofing, though I seemed to have left in the IsUint32() check above it. That should have been removed and the >= 0 check placed after retrieving size. Since the maximum size of the Buffer depends on the architecture, and the fact that ArrayBuffer has a much higher maximum size, I was hoping that the maximum length of a Uint8Array would increase in the future.

@bnoordhuis are you suggesting something like this?:

crypto.randomBytes(buf[, offset[, size]][, callback])

That seems like the following arguments would need to be possible:

crypto.randomBytes(size)
crypto.randomBytes(size, callback)
crypto.randomBytes(buf)
crypto.randomBytes(buf, callback)
crypto.randomBytes(buf, offset)
crypto.randomBytes(buf, offset, callback)
crypto.randomBytes(buf, offset, size)
crypto.randomBytes(buf, offset, size, callback)

Is there any way I could sway you on introducing a new api for this?

I'm definitely +1 on this but agree that a new API would be better. We consistently get feedback that adding new methods is preferable to extending an existing one.

Yes, but we also consistently get feedback that our APIs sprawl and aren't very cohesive. :-)

What about crypto.randomBytesBuffer()? That's close enough to the original that it should be easy to memorize.

No one likes my Buffer.randomFill() idea? I'll pitch it a bit more: By moving it to a different module we associate it more closely with Buffer and make it more discoverable, and avoid sprawl within the crypto API.

It does have the downside of coupling the Buffer API to an underlying OpenSSL/crypto facility, though, so maybe that downside overweighs the up.

@evanlucas in your examples of the syntaxes that would be reasonably expected to work, will the callback-less one still be async, or will it become magically sync? For a new API, I'd lean towards the more node-typical API pattern of a function with Sync in the name explicitly for sync (not the uv-derived "don't pass a callback and it becomes sync" behaviour).

@sam-github I would prefer to explicitly have to specify ...Sync() in the method name. I actually like randomFill() but not sure about it being on Buffer. I think it makes more sense to stick it on crypto.

@sam-github Though we need to consider that there are already APIs in core that write into a Buffer. e.g. fs.read(). i.e. keeping it in the module where the buffer is being used makes more sense (also b/c there are other places i'd like to be able to do this, and we can't throw them all on Buffer).

+1 for @bnoordhuis proposal.

@sam-github I prefer this staying in crypto, rather than being a Buffer method, because at least it indicates that OpenSSL is the source of the randomness (rather than Math.random()). Regardless of that being documented, that's something you instinctively know when using the API.

+1 to it staying in crypto. fwiw, I'd really like to see this land :-)

OK, consensus seems to be keep it in crypto and to use Sync for sync APIs.

I think randomBytesBuffer() as a name can be improved by describing the action rather than the data types it operates on:

crypto.randomFill(size, callback)
crypto.randomFill(buf, callback)
crypto.randomFill(buf, offset, callback)
crypto.randomFill(buf, offset, size, callback)

crypto.randomFillSync(size)
crypto.randomFillSync(buf)
crypto.randomFillSync(buf, offset)
crypto.randomFillSync(buf, offset, size)

@sam-github Is there a point in supporting randomFill without supplying a buffer? I would just drop those in favor of the randomBytes we have today (unless I'm not getting what that's trying to do...). Other than that, thumbs up from me :)

I've updated this to be crypto.randomFill() and crypto.randomFillSync(). PTAL. Thanks!

Notable changes: * **crypto**: - add randomFill and randomFillSync (Evan Lucas) #10209 * **meta**: Added new collaborators - add lucamaraschi to collaborators (Luca Maraschi) #12538 - add DavidCai1993 to collaborators (David Cai) #12435 - add jkrems to collaborators (Jan Krems) #12427 - add AnnaMag to collaborators (AnnaMag) #12414 * **process**: - fix crash when Promise rejection is a Symbol (Cameron Little) #11640 * **url**: - make WHATWG URL more spec compliant (Timothy Gu) #12507 * **v8**: - fix stack overflow in recursive method (Ben Noordhuis) #12460 - fix build errors with g++ 7 (Ben Noordhuis) #12392 PR-URL: #12775

Notable changes: * **crypto**: - add randomFill and randomFillSync (Evan Lucas) nodejs/node#10209 * **meta**: Added new collaborators - add lucamaraschi to collaborators (Luca Maraschi) nodejs/node#12538 - add DavidCai1993 to collaborators (David Cai) nodejs/node#12435 - add jkrems to collaborators (Jan Krems) nodejs/node#12427 - add AnnaMag to collaborators (AnnaMag) nodejs/node#12414 * **process**: - fix crash when Promise rejection is a Symbol (Cameron Little) nodejs/node#11640 * **url**: - make WHATWG URL more spec compliant (Timothy Gu) nodejs/node#12507 * **v8**: - fix stack overflow in recursive method (Ben Noordhuis) nodejs/node#12460 - fix build errors with g++ 7 (Ben Noordhuis) nodejs/node#12392 PR-URL: nodejs/node#12775 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable changes: * **crypto**: - add randomFill and randomFillSync (Evan Lucas) nodejs#10209 * **meta**: Added new collaborators - add lucamaraschi to collaborators (Luca Maraschi) nodejs#12538 - add DavidCai1993 to collaborators (David Cai) nodejs#12435 - add jkrems to collaborators (Jan Krems) nodejs#12427 - add AnnaMag to collaborators (AnnaMag) nodejs#12414 * **process**: - fix crash when Promise rejection is a Symbol (Cameron Little) nodejs#11640 * **url**: - make WHATWG URL more spec compliant (Timothy Gu) nodejs#12507 * **v8**: - fix stack overflow in recursive method (Ben Noordhuis) nodejs#12460 - fix build errors with g++ 7 (Ben Noordhuis) nodejs#12392 PR-URL: nodejs#12775

LTS opted to punt on this for this release, and consider it before the next semver-minor v6.x release.

Should probably land with #10209

should also land with #12541

Release team were +1 on backporting to v6.x and v8.x.

crypto.randomFill and crypto.randomFillSync are similar to crypto.randomBytes, but allow passing in a buffer as the first argument. This allows us to reuse buffers to prevent having to create a new one on every call. PR-URL: #10209 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

crypto.randomFill and crypto.randomFillSync are similar to crypto.randomBytes, but allow passing in a buffer as the first argument. This allows us to reuse buffers to prevent having to create a new one on every call. PR-URL: #10209 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

crypto.randomFill and crypto.randomFillSync are similar to crypto.randomBytes, but allow passing in a buffer as the first argument. This allows us to reuse buffers to prevent having to create a new one on every call. PR-URL: #10209 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This LTS release comes with 109 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 29 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) #12678 * crypto: - expose ECDH class (Bryan English) #8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) #10209 - warn on invalid authentication tag length (Tobias Nießen) #17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) #16835 * dgram: - added socket.setMulticastInterface() (Will Young) #7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) #13005 * lib: - return this from net.Socket.end() (Sam Roberts) #13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) #16386 * net: - return this from getConnections() (Sam Roberts) #13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) #13784 * repl: - improve require() autocompletion (Alexey Orlenko) #14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) #16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) #12087 - add process.ppid (cjihrig) #16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) #12839 * tools, build: - a new macOS installer! (JP Wesselink) #15179 * url: - WHATWG URL api support (James M Snell) #7448 * util: - add %i and %f formatting specifiers (Roman Reiss) #10308 PR-URL: #18342

This LTS release comes with 112 commits, 17 of which are considered Semver-Minor. This includes 32 which are doc related, 30 which are test related, 8 which are build / tool related and 1 commit which updates a dependency. Notable Changes: * console: - added console.count() and console.clear() (James M Snell) nodejs#12678 * crypto: - expose ECDH class (Bryan English) nodejs#8188 - added cypto.randomFill() and crypto.randomFillSync() (Evan Lucas) nodejs#10209 - warn on invalid authentication tag length (Tobias Nießen) nodejs#17566 * deps: - upgrade libuv to 1.16.1 (cjihrig) nodejs#16835 * dgram: - added socket.setMulticastInterface() (Will Young) nodejs#7855 * http: - add agent.keepSocketAlive and agent.reuseSocket as to allow overridable keep-alive behavior of `Agent` (Fedor Indutny) nodejs#13005 * lib: - return this from net.Socket.end() (Sam Roberts) nodejs#13481 * module: - add builtinModules api that provides list of all builtin modules in Node (Jon Moss) nodejs#16386 * net: - return this from getConnections() (Sam Roberts) nodejs#13553 * promises: - more robust stringification for unhandled rejections (Timothy Gu) nodejs#13784 * repl: - improve require() autocompletion (Alexey Orlenko) nodejs#14409 * src: - add openssl-system-ca-path configure option (Daniel Bevenius) nodejs#16790 - add --use-bundled-ca --use-openssl-ca check (Daniel Bevenius) nodejs#12087 - add process.ppid (cjihrig) nodejs#16839 * tls: - accept `lookup` option for `tls.connect()` (Fedor Indutny) nodejs#12839 * tools, build: - a new macOS installer! (JP Wesselink) nodejs#15179 * url: - WHATWG URL api support (James M Snell) nodejs#7448 * util: - add %i and %f formatting specifiers (Roman Reiss) nodejs#10308 PR-URL: nodejs#18342

nodejs-github-bot added dont-land-on-v7.x c++ Issues and PRs that require attention from people who are familiar with C++. labels Dec 9, 2016

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Dec 9, 2016

addaleax removed the dont-land-on-v7.x label Dec 11, 2016

addaleax reviewed Dec 11, 2016

View reviewed changes

evanlucas added the wip Issues and PRs that are still a work in progress. label Dec 13, 2016

evanlucas force-pushed the cryptobuf branch 2 times, most recently from 34cf94e to 5e3c4d1 Compare January 13, 2017 11:25

evanlucas removed the wip Issues and PRs that are still a work in progress. label Jan 13, 2017

evanlucas changed the title src: allow passing a buffer to RandomBytes crypto: add randomFill and randomFillSync Jan 13, 2017

evanlucas force-pushed the cryptobuf branch from 5e3c4d1 to 42daca5 Compare January 13, 2017 11:30

evanlucas added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 13, 2017

gibfahn mentioned this pull request May 16, 2017

doc: improve randomfill and fix broken link #12541

Closed

2 tasks

This was referenced May 16, 2017

Potential Semver Minor Backports nodejs/Release#188

Closed

Review semver-minor backports nodejs/Release#228

Closed

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

crypto: add randomFill and randomFillSync by evanlucas · Pull Request #10209 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: add randomFill and randomFillSync - #10209

crypto: add randomFill and randomFillSync#10209evanlucas wants to merge 1 commit intonodejs:masternodejs/node:masterfrom evanlucas:cryptobufevanlucas/node:cryptobufCopy head branch name to clipboard

Conversation

evanlucas commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Dec 9, 2016

Uh oh!

evanlucas commented Dec 9, 2016

Uh oh!

sam-github commented Dec 10, 2016

Uh oh!

addaleax commented Dec 11, 2016

Uh oh!

addaleax Dec 11, 2016

Choose a reason for hiding this comment

Uh oh!

evanlucas Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 12, 2016

Uh oh!

mscdex commented Dec 12, 2016

Uh oh!

trevnorris commented Dec 13, 2016

Uh oh!

evanlucas commented Dec 14, 2016

Uh oh!

jasnell commented Dec 23, 2016

Uh oh!

bnoordhuis commented Dec 23, 2016

Uh oh!

sam-github commented Dec 29, 2016

Uh oh!

evanlucas commented Dec 29, 2016

Uh oh!

trevnorris commented Jan 11, 2017

Uh oh!

ronkorving commented Jan 12, 2017

Uh oh!

jasnell commented Jan 12, 2017

Uh oh!

sam-github commented Jan 12, 2017

Uh oh!

ronkorving commented Jan 13, 2017

Uh oh!

evanlucas commented Jan 13, 2017

Uh oh!

gibfahn commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Jan 11, 2018

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

crypto: add randomFill and randomFillSync#10209
evanlucas wants to merge 1 commit into
nodejs:masterfrom
evanlucas:cryptobuf

evanlucas commented Dec 9, 2016 •
edited
Loading

gibfahn commented May 16, 2017 •
edited
Loading