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

doc: deprecate crypto.getRandomValues alias #41779

Closed
wants to merge 3 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 30, 2022

Refs: #41760

@aduh95 aduh95 added crypto Issues and PRs related to the crypto subsystem. notable-change PRs with changes that should be highlighted in changelogs. deprecations Issues and PRs related to deprecations. webcrypto labels Jan 30, 2022
@aduh95 aduh95 requested a review from jasnell January 30, 2022 23:35
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 30, 2022
@jasnell
Copy link
Member

jasnell commented Jan 30, 2022

What's the reason for deprecating this? It just says "non-standard behaviors"... Not clear what those are. Reading through the other issue, I'd argue that the illegal this error is not really sufficient justification.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 30, 2022

This alias is basically useless if we enforce this value, the only way it can be used would be

const { getRandomValues, webcrypto } = require('node:crypto')

getRandomValues.call(webcrypto, new Uint8Array)

What value is there in keeping an alias like that?

@mscdex
Copy link
Contributor

mscdex commented Jan 31, 2022

I agree, the doc deprecate description could probably use more detail so end users can have a better idea if additional changes are needed in their code to make things "standard."

@jasnell
Copy link
Member

jasnell commented Jan 31, 2022

I'd say that it's just a bug if it can't be called directly without passing in the Crypto object as this. Let's fix that bug and leave this alone otherwise. I see no reason to deprecate

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

I'd say that it's just a bug if it can't be called directly without passing in the Crypto object as this. Let's fix that bug and leave this alone otherwise. I see no reason to deprecate

Web browsers and Deno enforce that this must be an instance of Crypto. What do you mean “let’s fix that bug”? Do you mean you think Node.js should not align with the rest of the ecosystem? Or do you mean we should try to change the spec so this alias can work?

@targos
Copy link
Member

targos commented Jan 31, 2022

The alias doesn't have to have the same limitations as the spec-compliant one

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

I discover this limitation because I worked on a integrating on the web a Node.js library that was using WebCrypto, and got really confused by the Illegal invocation my Chrome was throwing at the code that was working without complaints in Node.js. I don't think we are making our users a good deed to our users by having a more relax implementation.
I think the correct way forward is to expose the WebCrypto as globalThis.crypto, and it will break the REPL but that's a better ecosystem for most users I would think. This alias in particular doesn't sound right to me.

@targos
Copy link
Member

targos commented Jan 31, 2022

I don't think we are making our users a good deed to our users by having a more relax implementation.

I don't think we should have a more relax implementation. I think we can make require('crypto').webcrypto.getRandomValues strict while keeping require('crypto').getRandomValues as it is.

@targos
Copy link
Member

targos commented Jan 31, 2022

By the way, there is the same issue with require('crypto').randomUUID and I am using it as import { randomUUID } from 'crypto' in several projects already. I don't want that to become deprecated.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

Maybe I'm confused on the use case here. My assumption is folks who are using getRandomValues use it because it lets them write web-compatible code. I don't see the point of using require('crypto').getRandomValues when require('crypto').randomBytes is available – unless we can make it web compatible (but how? By making require('crypto') instanceof Crypto true somehow?).

randomUUID is not advertised as an alias, and hasn't a Node.js-specific equivalent, so maybe it's a different story? Anyway, let's focus on getRandomValues, randomUUID is another discussion.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

Also, require('crypto').getRandomValues has landed in only one release (Node.js v17.4.0 two weeks ago), so we can expect that virtually no one has started using it yet.

doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
@targos
Copy link
Member

targos commented Jan 31, 2022

Maybe I'm confused on the use case here. My assumption is folks who are using getRandomValues use it because it lets them write web-compatible code. I don't see the point of using require('crypto').getRandomValues when require('crypto').randomBytes is available

It's closer to require('crypto').randomFillSync, but I get your point.
I did'n realize that the alias was only added in v17.4.0. In that case, I think all of this requires some discussion/collaboration with interested parties. The fact that you would like to immediately deprecate an API added in the previous release shows that there are different goals here.

From my PoV, exposing require('crypto').getRandomValues doesn't help to write web-compatible code, so I see the method more as something that can be used by people who know the webcrypto one and want to be able to import it easily in Node.js code.

– unless we can make it web compatible (but how? By making require('crypto') instanceof Crypto true somehow?).

This wouldn't help people doing import { getRandomValues } from 'crypto'; I don't see any issue with this use case and the API is simpler and less "scary" than randomFillSync.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

If instead, we add a semver-minor change to randomBytes to accept an ArrayBufferView as parameter, and internally calls webcrypto.getRandomValues, would that work for you?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

This wouldn't help people doing import { getRandomValues } from 'crypto'; I don't see any issue with this use case and the API is simpler and less "scary" than randomFillSync.

OK, but let's be clear in the docs this is NOT an alias to the webcrypto one then.

@targos
Copy link
Member

targos commented Jan 31, 2022

OK, but let's be clear in the docs this is NOT an alias to the webcrypto one then.

I have nothing against specifying in the docs that contrary to webcrypto.getRandomValues, this "alias" can be called without an enforced this, but honestly I think almost nobody cares or risks being affected by it.

It's also worth asking in the spec repo why this is enforced on these methods.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

honestly I think almost nobody cares or risks being affected by it.

I don't know what to tell you, I've been affected by it (and therefore I care), and I don't think I'm the only one trying to use WebCrypto to write web-compatible code. Or if you mean the alias specifically, well it's blocking the other PR so...

It's also worth asking in the spec repo why this is enforced on these methods.

Definitely, I also think this condition is silly. I won't do it, but if someone feels like they can to open an issue to see if the rest of the ecosystem can aligns with the more relaxed behavior, that'd be nice.

@targos
Copy link
Member

targos commented Jan 31, 2022

Or if you mean the alias specifically, well it's blocking the other PR so...

I do mean the alias specifically.

@tniessen
Copy link
Member

This seems like a larger problem to me. I didn't approve the aliases in the first place because of somewhat related concerns, but we have a serious problem if we start landing and releasing APIs just to deprecate them in the very next release.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 31, 2022

This seems like a larger problem to me. I didn't approve the aliases in the first place because of somewhat related concerns, but we have a serious problem if we start landing and releasing APIs just to deprecate them in the very next release.

I don't see the problem as long as we follow semver rules, any API is considered stable until it is not. Although maybe this shows that we should have a policy that any new API should land as experimental first so we don't have to go through a whole deprecation cycle if problems arise when it's released.

@targos
Copy link
Member

targos commented Jan 31, 2022

In this case, we could argue that being an alias of an experimental method crypto.getRandomValues is also experimental.

jasnell
jasnell previously requested changes Feb 5, 2022
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I am still -1 on deprecating this. The reasons for wanting to deprecate this are not at all clear.

@aduh95
Copy link
Contributor Author

aduh95 commented Feb 14, 2022

@jasnell should this land now that #41938 have landed? Or do you think it's still useful to keep the alias even if it's already available through globalThis.crypto?

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 16, 2022
@tniessen
Copy link
Member

Is #41938 going to be unflagged in a major release? It breaks the REPL. If it isn't, and crypto will never be an unflagged global, then the aliases might have some justification.

@targos
Copy link
Member

targos commented Feb 16, 2022

It breaks the REPL

Does it break it in the sense that the REPL doesn't work or that it's a breaking (semver-major) change?

@tniessen
Copy link
Member

Does it break it in the sense that the REPL doesn't work or that it's a breaking (semver-major) change?

I'd say it's a semver-major-MAJOR-major change. It breaks every script that uses crypto through node -p etc.

@targos
Copy link
Member

targos commented Feb 16, 2022

Does it break it in the sense that the REPL doesn't work or that it's a breaking (semver-major) change?

I'd say it's a semver-major-MAJOR-major change. It breaks every script that uses crypto through node -p etc.

I didn't think about node -p. You think these kinds of scripts are common?

@tniessen
Copy link
Member

You think these kinds of scripts are common?

I don't think we have any data on that. It wouldn't surprise me to see something like this:

# Figure out what OpenSSL version the installed node version is using.
node_ossl=$(node -p 'process.versions.openssl')
# Figure out what ciphers the installed node version supports.
node_ciphers=$(node -p 'crypto.getCiphers().join("\n")')

@Mesteery
Copy link
Member

Mesteery commented Feb 16, 2022

Examples:

It breaks every script that uses crypto through node -p etc.

It also breaks scripts that uses crypto through eval, but I think it is much less frequent.

@jasnell
Copy link
Member

jasnell commented Feb 16, 2022

@aduh95:

Or do you think it's still useful to keep the alias even if it's already available through globalThis.crypto?

No, really the key point was to align somewhat with the global web crypto so now that we're moving forward with making that global, I can drop my objection here

aduh95 added a commit that referenced this pull request Feb 22, 2022
Refs: #41779
Refs: #41760
PR-URL: #41782
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 22, 2022

I've opened #42083 to discuss make Web Crypto available on the global scope by default, except for --eval and --print to keep supporting the use case reported above. If the other PR is well received, I wouldn't want to remove this alias anymore, nor deprecate it (although we might still want to call that use case (and also this alias) legacy? I don't know, maybe it's not worth it.)

@aduh95 aduh95 removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 23, 2022
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 24, 2022

I'm no longer convinced a deprecation is necessary at this time, this can be reopened at a later time if necessary.

@aduh95 aduh95 closed this Feb 24, 2022
@aduh95 aduh95 deleted the deprecate-getRandomValues-alias branch February 24, 2022 10:39
sxa pushed a commit to sxa/node that referenced this pull request Mar 7, 2022
Refs: nodejs#41779
Refs: nodejs#41760
PR-URL: nodejs#41782
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs. webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants