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.

Sign up for GitHub

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

Jump to bottom

crypto: allow monkey patching of pseudoRandomBytes #24108

Closed
wants to merge 2 commits into from
Closed

crypto: allow monkey patching of pseudoRandomBytes #24108

wants to merge 2 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Nov 5, 2018

Make pseudoRandomBytes and it's aliases prng and rng
configurable to allow monkey patching.

Background: Some modules still use these deprecated APIs (see e.g.
#23013) and therefore these APIs are only pending deprecated. As APM
vendor we can't decide which modules customers so we need a way to
monitor also the aliases similar as the correct API. Current way to
do this is monkey patching.

Maybe we could even consider to set writeable: true.

If there are good arguments to keep it as it is I'm also fine with
closing this PR.

Refs: #22519
Refs: #23017

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 nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Nov 5, 2018
@refack
Copy link
Contributor

refack commented Nov 5, 2018

/CC @nodejs/crypto

@refack
Copy link
Contributor

refack commented Nov 5, 2018 •
edited
Loading

Hello @Flarna and thank you for the contribution.
The change SGTM, but I don't feel I'm qualified to approve.
What I would suggest (iff the mood will be to approve this) is adding a test that asserts that these APIs are indeed configurable. Maybe even document that.

sam-github
sam-github approved these changes Nov 5, 2018
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The monkey patchability of the API surface isn't usually tested or documented, so while I'm usually a big fan of both, I don't personally think its necessary in this case. I can't think of any other APIs where we document the reassignability of the function. Monkey-patchability isn't something we formally commit to, but in the interests of supporting APM vendors, if the maintenance cost is low I don't think we should get in its way, either. This change looks to me to have very low maintenance cost.

@Flarna
Copy link
Member Author

Flarna commented Nov 5, 2018

Sure, I can add tests.
Main point is to first get on an agreement if this is ok or not and if we should set also writeable: true - like for "normal" exports.

@refack
Copy link
Contributor

refack commented Nov 5, 2018

The monkey patchability of the API surface isn't usually tested or documented, so while I'm usually a big fan of both, I don't personally think its necessary in this case. I can't think of any other APIs where we document the reassignability of the function.

Af I see it most of our current monkey patchability was just emergent. This is a bit different since we are enabling this explicitly and so IMHO we should commit to.

@Flarna
Copy link
Member Author

Flarna commented Nov 5, 2018

I think the main target of the initial change was just to have the APIs not enumerable.
Having them non writeable/configurable was maybe just a side effect of the defaults which are inverted if a property descriptor is used compared to just setting a named property.

I think it would be good to define a guideline for deprecations like this to get a consistent API.

@Trott
Copy link
Member

Trott commented Nov 6, 2018

@nodejs/security-wg

@Flarna
Copy link
Member Author

Flarna commented Nov 7, 2018

Updated by setting also writeable: true and added a test.
I have seen that writeable: true is also used in other places (e.g. queueMicrotask) and it avoids to play around with descriptors during patching.

@Flarna
crypto: allow monkey patching of pseudoRandomBytes
8e1be37 
Make `pseudoRandomBytes` and it's aliases `prng` and `rng`
configurable to allow monkey patching.
@Flarna
set additionally writeable: true
fa4a695 
add test
@Flarna
Copy link
Member Author

Flarna commented Nov 21, 2018

Is there anything left I have to do with this PR?

@tniessen
Copy link
Member

@Flarna No, sorry for the delay. CI: https://ci.nodejs.org/job/node-test-pull-request/18857/

@Trott
Copy link
Member

Trott commented Nov 25, 2018 •
edited
Loading

Landed in f85d636

@Trott Trott closed this Nov 25, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 25, 2018
@Flarna @Trott
crypto: allow monkey patching of pseudoRandomBytes
f85d636 
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 <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Flarna Flarna deleted the crypto_patchable branch November 25, 2018 15:36
targos pushed a commit that referenced this pull request Nov 25, 2018
@Flarna @targos
crypto: allow monkey patching of pseudoRandomBytes
310b1ec 
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 <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@Flarna @rvagg
crypto: allow monkey patching of pseudoRandomBytes
16d7060 
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 <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
This was referenced Dec 8, 2018
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@Flarna @refack
crypto: allow monkey patching of pseudoRandomBytes
6ad6588 
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 <vieuxtech@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs
Copy link
Member

This change does not land cleanly on v10.x-staging. I've added the backport-requested-v10.x label, but feel free to swap to dont-land-on- if this change shouldn't land on v10.x.

@Flarna
Copy link
Member Author

Flarna commented Feb 12, 2019

@BethGriggs This change is not applicable to v10.x but I don't think I have rights to change labels here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@tniessen tniessen tniessen approved these changes

@vdeturckheim vdeturckheim vdeturckheim approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@watson watson Awaiting requested review from watson

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.