crypto: add randomFill and randomFillSync

[evanlucas]

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.

[mscdex]

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

[evanlucas]

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

[sam-github]

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.

[addaleax]

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

[addaleax]

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

[evanlucas]

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

[bnoordhuis]

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?

letsupported=true;try{crypto.randomBytes(newBuffer(1));}catch(e){supported=!(einstanceofTypeError);}

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.

[mscdex]

Yes, allowing an offset would be even better.

[trevnorris]

@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.

[evanlucas]

@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?

[jasnell]

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.

[bnoordhuis]

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.

[sam-github]

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

[evanlucas]

@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.

[trevnorris]

@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.

[ronkorving]

@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.

[jasnell]

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

[sam-github]

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) 

[ronkorving]

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

[evanlucas]

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

[gibfahn]

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

[MylesBorins]

should also land with #12541

[gibfahn]

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