crypto: clarify require("crypto").getRandomValues is Node.js specific

[aduh95]

Refs: #41779
Refs: #41760

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[aduh95]

@jasnell could you help us here? It seems there is a confusion on what is the plan for exposing a crypto object on the global scope. Is the plan:

• Exposing require('crypto') as crypto everywhere?
• Exposing require('crypto').webcrypto as crypto everywhere, breaking change for the REPL?
• Exposing require('crypto').webcrypto as crypto on files, keep exposing require('crypto') as crypto on the REPL?

If it's the latter, then I guess we should land this PR, as well as #41760, but this would need your input to move forward.

[jasnell]

My preference is exposing require('crypto') as the global.

[aduh95]

My preference is exposing require('crypto') as the global.

This is going to create all sort of compatibility problems (I've been myself affected by #41760), I'm -1 on this idea. Adding it to the TSC agenda so we can discuss this further.

[tniessen]

Exposing require('crypto').webcrypto as crypto on files, keep exposing require('crypto') as crypto on the REPL?

I suggested this when talking to @jasnell earlier, except that I'd maybe add the aliases to crypto only in the REPL. (Not sure if it's a good idea after all.)

[jasnell]

One possible approach we can take here is to modify Node.js' crypto to extend from Web Crypto crypto. Specifically, something like:

class NodeCrypto extends Crypto {
// ... add the Node.js stuff...
};
module.exports = new NodeCrypto();

[aduh95]

One possible approach we can take here is to modify Node.js' crypto to extend from Web Crypto crypto. Specifically, something like:

class NodeCrypto extends Crypto {
// ... add the Node.js stuff...
};
module.exports = new NodeCrypto();

It wouldn't help folks doing import { getRandomValues } from 'node:crypto', right? In that case, that'd be fine with me, as long as we explicitly say that it's not a use-case we want to support.

Could you maybe explain why we wouldn't expose require('crypto').webcrypto as global.crypto? I'm uncomfortable at the idea of introducing another Node.js specific global, especially if it's competing with one already in browsers and Deno. Is it because REPL (and node --eval) already expose a global-like named crypto and you are concerned it would be too breaking to change that? Sorry if you already discussed that somewhere else, but I feel like I'm missing some context here.

[tniessen]

Is it because REPL (and node --eval) already expose a global-like named crypto and you are concerned it would be too breaking to change that?

Yes, I think that was the motivation behind #41266 (see #41266 (comment)).

[aduh95]

FYI, I've opened #41938 to suggest exposing the Web Crypto on the global scope instead of the Node.js one.

[tniessen]

FYI, I've opened #41938 to suggest exposing the Web Crypto on the global scope instead of the Node.js one.

@aduh95 Now that #41938 has landed, should this be closed or would you still like to remove require('crypto').getRandomValues?

[aduh95]

would you still like to remove require('crypto').getRandomValues?

I don't think we get much value from having require('crypto').getRandomValues if it's already available (in a web-compatible way) through globalThis.crypto.getRandomValues. I still think we should remove it, or make it web compatible.

should this be closed

Whatever we decide to do we the alias, I think we should land this ASAP: all this PR is doing is documenting that the shortcut is not exactly the same as the web version, and it unblocks #41760.

Currently our docs for Web crypto are "broken", for example here we tell the user to use object destructuring to get a reference to getRandomValues:

node/doc/api/webcrypto.md

Lines 128 to 159 in b8de7aa

	### Encryption and decryption
	```js
	const { subtle, getRandomValues } =require('crypto').webcrypto;
	asyncfunctionaesEncrypt(plaintext) {
	constec=newTextEncoder();
	constkey=awaitgenerateAesKey();
	constiv=getRandomValues(newUint8Array(16));
	constciphertext=awaitsubtle.encrypt({
	name:'AES-CBC',
	iv,
	}, key, ec.encode(plaintext));
	return {
	key,
	iv,
	ciphertext
	};
	}
	asyncfunctionaesDecrypt(ciphertext, key, iv) {
	constdec=newTextDecoder();
	constplaintext=awaitsubtle.decrypt({
	name:'AES-CBC',
	iv,
	}, key, ciphertext);
	returndec.decode(plaintext);
	}
	```

If the user tries to use this code in a browser or Deno, it will throw an arguably quite surprising error (e.g. Chromium throws TypeError: Illegal invocation).

[mcollina]

lgtm

[mcollina]

@jasnell have you got any concerns in landing this as-is?

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[aduh95]

Landed in 470c284

[danielleadams]

@aduh95 this doesn't land cleanly on v16.x-staging. Can you make a backport for this? Thank you