crypto: deprecate aliases for randomBytes #22519

tniessen · 2018-08-25T10:28:36Z

There are three aliases for crypto.randomBytes now, all of them are undocumented. I think we should strive to have as little undocumented API surface as possible, so the next logical step would be to either document or deprecate these aliases.

crypto.pseudoRandomBytes used to be a different function but became an alias for crypto.randomBytes in #557 (three years ago). It has been undocumented ever since and is rarely used within the ecosystem.

The other two aliases, crypto.rng and crypto.prng have never been documented as far as I can tell and their names are unintuitive for people who are not familiar with crypto and the concept of CSRNGs / CSPRNGs.

Checklist

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

nodejs-github-bot · 2018-08-25T10:28:37Z

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/669/pipeline

tniessen · 2018-08-25T10:58:48Z

cc @nodejs/crypto @nodejs/security-wg

Deprecation was suggested back in #557 by the way.

tniessen · 2018-08-25T10:59:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16750/
Resumed in: https://ci.nodejs.org/job/node-test-pull-request/16751/
Resumed in: https://ci.nodejs.org/job/node-test-pull-request/16758/

ryzokuken

LGTM.

Trott · 2018-08-25T13:56:28Z

doc/api/deprecations.md

+Type: Runtime
+
+In recent versions of Node.js, there is no difference between
+[`crypto.randomBytes()`][] and `crypto.pseudoRandomBytes()`. The latter has been


Nit: has been -> is for consistency with the rest of the deprecation messages (with one exception that should also be changed but not in this PR).

mcollina

LGTM

mcollina · 2018-08-25T14:56:08Z

@dougwilson which module is that? why is it doing that check?

mcollina · 2018-08-25T18:03:34Z

@dougwilson I would release a new major version of that module, and drop support for Node < 4 (even Node < 6). However, you could still detect this without version checking via crypto.propertyIsEnumerable('pseudoRandomBytes'). It will tell you if the property is there and it has not been deprecated. Let me know if you would like to get a PR with this fix.

Note that when this land any user of random-bytes would see the warning.

mcollina · 2018-08-25T18:10:35Z

As for the warning, wouldn't the warning only occur on calling the method? The module doesn't call the method, just checks equality.

My bad.

mcollina · 2018-08-25T18:11:15Z

As for that check, I can do that. I read through the docs on deprecate and it didn't say that was a method to detect, so if that's the official way to detect if something is deprecated, I can make a PR to add it to the docs and make a test to add to the Node.js test suite

I would say go for it.

mcollina · 2018-08-25T18:12:39Z

lib/crypto.js

@@ -234,5 +231,20 @@ Object.defineProperties(exports, {
    configurable: false,
    enumerable: true,
    value: constants
+  },
+
+  // Aliases for randomBytes are deprecated.


Can you please add a comment:

// The ecosystem needs those to exists for backwards compatibility with // ancient Node.js runtimes (0.10, 0.12).

Added it. IMO this kind of makes it sound as if we couldn't remove these functions in the future though.

Exactly, this is the sound of #22519 (comment) comment.

tniessen · 2018-08-26T10:31:42Z

that point of the module is to provide that back-compat, so releasing a new major version without it is kind of pointless -- modules can just use Node.js API directly if they didn't need that. SO making that change doesn't make any sense.

@dougwilson Thanks for weighing in! Are you sure that people are still using that package for backcompatibility? The only Node.js releases which require this kind of check are from 2015 or even older and have long been EOL. Maybe people just want to use promises?

tniessen · 2018-08-26T15:51:28Z

Are you sure that people are still using that package for backcompatibility? The only Node.js releases which require this kind of check are from 2015 or even older and have long been EOL. Maybe people just want to use promises?

How can I make this determination?

@dougwilson I don't know. Your previous comment suggested that backcompatibility was the primary intention of people using the package, but I'd be surprised if that was true. There are barely any packages depending on random-bytes, so maybe the majority of downloads is caused by people using uid-safe?

It is cool that packages such as yours try to support "ancient" versions as @mcollina called them, but that shouldn't stand in the way of improving Node.js.

tniessen · 2018-08-26T16:11:02Z

@dougwilson I am sorry for the misunderstanding, this was in no way directed at you! This was entirely based on #22519 (comment), which suggests that we cannot remove these functions.

thefourtheye · 2018-08-27T06:43:24Z

lib/crypto.js

+  },
+
+  // Aliases for randomBytes are deprecated.
+  // The ecosystem needs those to exists for backwards compatibility with


Nit: exists -> exist?

Refs: nodejs#22501 Refs: nodejs#22519

tniessen · 2018-08-27T13:07:23Z

Rebased, old HEAD was 8a304fe59a87cf8be7e0a804c860d017b91375ab.

tniessen · 2018-08-30T15:22:42Z

Landed in 221df22.

PR-URL: #22519 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]>

targos · 2018-09-01T14:51:14Z

This affects a module used by npm: https://github.com/npm/unique-slug
I have Node canary on my system and get a warning when I install git dependencies.

tniessen · 2018-09-01T17:30:30Z

@targos I opened npm/unique-slug#6.

Make `pseudoRandomBytes` and it's aliases `prng` and `rng` configurable to allow monkey patching. PR-URL: nodejs#24108 Refs: nodejs#22519 Refs: nodejs#23017 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: James M Snell <[email protected]>

Make `pseudoRandomBytes` and it's aliases `prng` and `rng` configurable to allow monkey patching. PR-URL: #24108 Refs: #22519 Refs: #23017 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: James M Snell <[email protected]>

Trott · 2018-12-29T15:41:19Z

The deprecation warning causes an npm test to fail, at least when run via our make test-npm task.

    # Subtest: tarball paths should update port if updating protocol
        not ok 1 - no error output
          ---
          found: >
            (node:51947) [DEP0115] DeprecationWarning: crypto.pseudoRandomBytes 
is
            deprecated.
          wanted: ''
          compare: ===
          at:
            line: 75
            column: 9
            file: test/tap/add-named-update-protocol-port.js
            type: global

Hopefully npm/unique-slug#6 can land soon, but if not, I wonder if we should document pseudoRandomBytes() as an alias for randomBytes() and move it back to doc-only deprecation.

We could also consider the Buffer constructor route and only produce a runtime warning on use outside of node_modules.

Updating the npm test somehow may also be an option, I suppose, although it's not obvious to me how in this case, as the whole point of this test is probably to check that there isn't stderr output on success.

tniessen · 2018-12-29T16:16:45Z

@Trott I think npm/unique-slug#6 should land even if we un-deprecate pseudoRandomBytes. It has been open for four months now...

I am +0.5 on the deprecation itself. I suggested documentation as an alternative, but I still prefer the deprecation.

addaleax · 2018-12-30T16:43:33Z

Aliases are very cheap, and if this adds a deprecation warning to npm output in some cases, I’d prefer to undo the deprecation (or make it a --pending-deprecation-only thing)

tniessen · 2018-12-30T16:47:02Z

I’d prefer to undo the deprecation (or make it a --pending-deprecation-only thing)

@addaleax I think you already did that in #23017 :)

addaleax · 2018-12-30T17:00:34Z

@tniessen oh, ha 😄 Is there actually anything to do for us then? Does the npm test still fail without --pending-deprecation?

Trott · 2018-12-30T17:35:02Z

Does the npm test still fail without --pending-deprecation?

Oh, right, I have --pending-deprecation on by default. Let's try make test-npm without it...

Heh, the test still fails on master but not because of this. It's instead because it gets this warning:

npm WARN npm npm does not support Node.js v12.0.0-pre

The test in question is making sure that there are no warnings emitted. So as-is, I suppose it will always fail on master. Wonder if we should make it an expected failure in our wrapper or if the test itself should be updated to be tolerant about -pre releases. Or maybe there's a way to invoke npm such that it suppresses only that warning?]

EDIT: Will be solved by updating core's npm from 6.5.0-next to 6.5.0. So: There's no problem.

Make `pseudoRandomBytes` and it's aliases `prng` and `rng` configurable to allow monkey patching. PR-URL: nodejs#24108 Refs: nodejs#22519 Refs: nodejs#23017 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: James M Snell <[email protected]>

tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 25, 2018

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 25, 2018

targos approved these changes Aug 25, 2018

View reviewed changes

tniessen added the deprecations Issues and PRs related to deprecations. label Aug 25, 2018

ryzokuken approved these changes Aug 25, 2018

View reviewed changes

lpinca approved these changes Aug 25, 2018

View reviewed changes

Trott reviewed Aug 25, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

mcollina approved these changes Aug 25, 2018

View reviewed changes

targos requested a review from a team August 25, 2018 15:01

This comment has been minimized.

Sign in to view

mcollina reviewed Aug 25, 2018

View reviewed changes

dougwilson mentioned this pull request Aug 25, 2018

Determine if a method is deprecated in code #22524

Closed

trivikr approved these changes Aug 26, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

thefourtheye approved these changes Aug 27, 2018

View reviewed changes

tniessen added a commit to tniessen/node that referenced this pull request Aug 27, 2018

crypto: make _toBuf non-enumerable

8ba1d71

Refs: nodejs#22501 Refs: nodejs#22519

tniessen mentioned this pull request Aug 27, 2018

crypto: make _toBuf non-enumerable #22551

Closed

2 tasks

crypto: deprecate aliases for randomBytes

86b7efd

tniessen force-pushed the crypto-deprecate-random-alias branch from 8a304fe to 86b7efd Compare August 27, 2018 13:06

Address Trott's comment

6282e34

tniessen mentioned this pull request Aug 27, 2018

doc: unify deprecation wording #22555

Closed

2 tasks

jasnell approved these changes Aug 27, 2018

View reviewed changes

tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 28, 2018

tniessen closed this Aug 30, 2018

tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018

Flarna mentioned this pull request Nov 5, 2018

crypto: allow monkey patching of pseudoRandomBytes #24108

Closed

4 tasks

Trott mentioned this pull request Dec 29, 2018

deps: upgrade npm to v6.5.0 #25234

Closed

3 tasks

littledivy mentioned this pull request Dec 19, 2023

node:crypto.pseudoRandomBytes function is missing denoland/deno#21632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: deprecate aliases for randomBytes #22519

crypto: deprecate aliases for randomBytes #22519

tniessen commented Aug 25, 2018 •

edited

Loading

nodejs-github-bot commented Aug 25, 2018

tniessen commented Aug 25, 2018

tniessen commented Aug 25, 2018 •

edited

Loading

ryzokuken left a comment

Trott Aug 25, 2018

This comment has been minimized.

mcollina left a comment

mcollina commented Aug 25, 2018

This comment has been minimized.

mcollina commented Aug 25, 2018

This comment has been minimized.

mcollina commented Aug 25, 2018

mcollina commented Aug 25, 2018

mcollina Aug 25, 2018

tniessen Aug 26, 2018

mcollina Aug 26, 2018

tniessen commented Aug 26, 2018

This comment has been minimized.

tniessen commented Aug 26, 2018

This comment has been minimized.

This comment has been minimized.

tniessen commented Aug 26, 2018

thefourtheye Aug 27, 2018

tniessen commented Aug 27, 2018

tniessen commented Aug 30, 2018

targos commented Sep 1, 2018

tniessen commented Sep 1, 2018

Trott commented Dec 29, 2018

tniessen commented Dec 29, 2018

addaleax commented Dec 30, 2018

tniessen commented Dec 30, 2018

addaleax commented Dec 30, 2018

Trott commented Dec 30, 2018 •

edited

Loading

crypto: deprecate aliases for randomBytes #22519

crypto: deprecate aliases for randomBytes #22519

Conversation

tniessen commented Aug 25, 2018 • edited Loading

Checklist

nodejs-github-bot commented Aug 25, 2018

tniessen commented Aug 25, 2018

tniessen commented Aug 25, 2018 • edited Loading

ryzokuken left a comment

Choose a reason for hiding this comment

Trott Aug 25, 2018

Choose a reason for hiding this comment

This comment has been minimized.

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Aug 25, 2018

This comment has been minimized.

mcollina commented Aug 25, 2018

This comment has been minimized.

mcollina commented Aug 25, 2018

mcollina commented Aug 25, 2018

mcollina Aug 25, 2018

Choose a reason for hiding this comment

tniessen Aug 26, 2018

Choose a reason for hiding this comment

mcollina Aug 26, 2018

Choose a reason for hiding this comment

tniessen commented Aug 26, 2018

This comment has been minimized.

tniessen commented Aug 26, 2018

This comment has been minimized.

This comment has been minimized.

tniessen commented Aug 26, 2018

thefourtheye Aug 27, 2018

Choose a reason for hiding this comment

tniessen commented Aug 27, 2018

tniessen commented Aug 30, 2018

targos commented Sep 1, 2018

tniessen commented Sep 1, 2018

Trott commented Dec 29, 2018

tniessen commented Dec 29, 2018

addaleax commented Dec 30, 2018

tniessen commented Dec 30, 2018

addaleax commented Dec 30, 2018

Trott commented Dec 30, 2018 • edited Loading

tniessen commented Aug 25, 2018 •

edited

Loading

tniessen commented Aug 25, 2018 •

edited

Loading

Trott commented Dec 30, 2018 •

edited

Loading