Skip to content
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

docs: more explicit crypto.pseudoRandomBytes information #545

Closed
wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Contributor

the docs as it is make it sound like pseudoRandomBytes is tapping a different entropy source or possibly might be a /dev/urandom to crypto.pseudoRandomBytes's /dev/random.

Is there a reason pseudoRandomBytes is in the api in the first place?

@bnoordhuis
Copy link
Member

Is there a reason pseudoRandomBytes is in the api in the first place?

Yes, but the reason may be invalid. When I added them, I reasoned that people may want to plug in a custom RAND engine but, to this day, I've never heard of anyone actually doing that. I speculate that people with hardware RNG devices feed that entropy into the system entropy pool, not directly into openssl.

The PR LGTM but can you update the commit log? The first line should be <= 50 characters and there should be one or two lines describing the what and why of the change. Thanks!

@bnoordhuis
Copy link
Member

On the subject of crypto.pseudoRandomBytes(), your PR made me realize that there is probably no downside to making it an alias of crypto.randomBytes(). In the past, there was a material difference between them, but as of f68a116, that's no longer the case.

@calvinmetcalf
Copy link
Contributor Author

Ok can amend the commit.

Would it make sense to think about depreciating it?

On Wed, Jan 21, 2015, 5:59 PM Ben Noordhuis [email protected]
wrote:

Is there a reason pseudoRandomBytes is in the api in the first place?

Yes, but the reason may be invalid. When I added them, I reasoned that
people may want to plug in a custom RAND engine but, to this day, I've
never heard of anyone actually doing that. I speculate that people with
hardware RNG devices feed that entropy into the system entropy pool, not
directly into openssl.

The PR LGTM but can you update the commit log? The first line should be <=
50 characters and there should be one or two lines describing the what and
why of the change. Thanks!


Reply to this email directly or view it on GitHub
#545 (comment).

@calvinmetcalf
Copy link
Contributor Author

Ok i can update the pull to do that as well

On Wed, Jan 21, 2015, 6:03 PM Ben Noordhuis [email protected]
wrote:

On the subject of crypto.pseudoRandomBytes(), your PR made me realize that
there is probably no downside to making it an alias of
crypto.randomBytes(). In the past, there was a material difference between
them, but as of f68a116
f68a116,
that's no longer the case.


Reply to this email directly or view it on GitHub
#545 (comment).

@bnoordhuis
Copy link
Member

@calvinmetcalf Let's start with the documentation update. Changing crypto.pseudoRandomBytes() would also require ripping out some code from src/node_crypto.cc.

@calvinmetcalf
Copy link
Contributor Author

Haha ok won't get ahead of myself

On Wed, Jan 21, 2015, 6:07 PM Ben Noordhuis [email protected]
wrote:

@calvinmetcalf https://github.com/calvinmetcalf Let's start with the
documentation update. Changing crypto.pseudoRandomBytes() would also
require ripping out some code from src/node_crypto.cc.


Reply to this email directly or view it on GitHub
#545 (comment).

Updates the docs for the crypto.pseudoRandomBytes function
to more explicitly detail how it's the same as crypto.randomBytes
just without a safety net (e.g. it doesn't throw an error when
there is low entropy).
@calvinmetcalf
Copy link
Contributor Author

updated

bnoordhuis pushed a commit that referenced this pull request Jan 22, 2015
Updates the docs for the crypto.pseudoRandomBytes function
to more explicitly detail how it's the same as crypto.randomBytes
just without a safety net (e.g. it doesn't throw an error when
there is low entropy).

PR-URL: #545
Reviewed-By: Ben Noordhuis <[email protected]>
@bnoordhuis
Copy link
Member

Thanks Calvin, landed in d481bb6 with minor touch-ups for line length.

This was referenced Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants