-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto.pseudoRandomBytes #72
Comments
I bet this was changed since the commit made to the code. If you check this answer on stackoverflow it describes the situation, although it seems like that the node authors just modified the crypto library and kept the See the following short snippet: var crypto = require('crypto');
crypto.randomBytes(1); // <Buffer 44>
crypto.pseudoRandomBytes(1); // <Buffer 45> See also: |
Sorry for the late, thank you for the explanation. |
pseudoRandomBytes api has been removed from Nodejs since v4.
|
While it seems to have been deprecated, it is still available in Node v16.15.1.
Which is why we never changed it. |
try {
rnd = crypto.randomBytes(howMany);
} catch (e) {
rnd = crypto.pseudoRandomBytes(howMany);
} The code branch is never reachable in Nodejs v8+. |
You are probably right in most of the cases. However, it could happen that I'll need to investigate deeper before we can safely remove this fallback mechanism. |
My encounter of the issue is Nodejs's EMS export. When I use esbuild to bundle my lib, it transpile this lib into esm. Something like this: import {randomBytes, pseudoRandomBytes} from 'crypto'; Then at runtime, Nodejs rejects it because the ESM exports (I guess Nodejs internal) of
|
It is there, but I guess the type definitions are not showing that anymore.
|
It's there for commonjs access, not esm access.
|
FYI, Node.js v12.22.12 also gives the same error. |
I don't really understand your point. Line 15 in 4b51e90
How is your example relevant to |
Sorry, I missed this part. I don't think this is an issue of |
Not esbuild, did you see my example above? The cat and node commands, without esbuild. |
This source code is not from |
Sorry. I misrepresented myself. I am not saying this is Rather just a point for consideration to remove the usage of The other point is Nodejs removed |
https://github.com/nodejs/node/blob/main/lib/crypto.js FYI pseudoRandomBytes is just an alias to randomBytes, it's not the real pseudo anymore. So the try-catch fallback here is actually not doing anything more. |
Thanks for the link. This definitely helps. I'll open a PR as soon as I get some free time. |
randomBytes will still generate an error. So we need a replacement pseudoRandomBytes generator in case that there is not enough entropy in the system and the "true" random bytes generator runs out of data. |
From nodejs doc, crypto.randomUUID does not throw, but it's only available in v15.6.0+. |
reopening since this is definitely an issue and we might run out of entropy, especially during system start. |
@3cp Good point. I also thought about having a UUIDv4 based name generation strategy. However, this will require an additional dependency for node < v15.6.0, as you pointed out. Maybe we will revive the old 'naive' random name generation code that simply used random ints and a lookup table as a fallback. |
I will create a new issue and link to this one in the description. |
Please continue the conversation over at #280 |
Hi, i am a little confused with this line of your code:
rnd = crypto.pseudoRandomBytes(howMany);
but i cannot find this function in node.js document about crypto.
i don't if i am wrong, this is just a kindly remindness
The text was updated successfully, but these errors were encountered: