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

crypto: alias webcrypto.subtle and webcrypto.getRandomValues on crypto #41266

Merged
merged 3 commits into from
Dec 27, 2021

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 21, 2021

The aliases allow code written to assume that crypto.subtle and
crypto.getRandomValues() exist on the crypto global to just work.

Signed-off-by: James M Snell [email protected]

The aliases allow code written to assume that `crypto.subtle` and
`crypto.getRandomValues()` exist on the `crypto` global to just work.

Signed-off-by: James M Snell <[email protected]>
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. webcrypto labels Dec 21, 2021
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 21, 2021
@panva
Copy link
Member

panva commented Dec 21, 2021

code written to assume that [...]

A code like that will likely expect crypto to be on a globalThis, window, global, or similar in the first place. Nevertheless, I don't mind this change if it means some libraries will work.

It's worth pointing out that webcrypto is still experimental and there's no runtime warning about it being so.

@jasnell
Copy link
Member Author

jasnell commented Dec 21, 2021

A code like that will likely expect crypto to be on a globalThis

Yep once this lands, I plan to open a PR to expose crypto on globalThis

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2021
@nodejs-github-bot
Copy link
Collaborator

lib/crypto.js Outdated Show resolved Hide resolved
lib/crypto.js Outdated Show resolved Hide resolved
lib/crypto.js Outdated Show resolved Hide resolved
lib/crypto.js Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
@nodejs-github-bot

This comment has been minimized.

doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/crypto.md Show resolved Hide resolved
@tniessen
Copy link
Member

A code like that will likely expect crypto to be on a globalThis

Yep once this lands, I plan to open a PR to expose crypto on globalThis

What is the plan for doing that? I am unsure if this change makes sense without exposing crypto as a global. Otherwise, users still need to require or import crypto, which means that it remains incompatible with code that targets browsers etc., and not much is gained from these convenience additions.

Would the global crypto be equivalent to require('crypto') or would it be require('crypto').webcrypto? Based on this PR, I'd assume it would be the crypto module, which seems inconsistent with other environments.

@aduh95
Copy link
Contributor

aduh95 commented Dec 22, 2021

Otherwise, users still need to require or import crypto, which means that it remains incompatible with code that targets browsers etc., and not much is gained from these convenience additions.

Not exactly incompatible, thanks to import maps:

<script type="importmap">
  {
      "imports": {
          "node:crypto": "data:text/javascript,export%20default%20globalThis.crypto"
      }
  }
</script>
<script type="module">import crypto from 'node:crypto'; console.log(crypto.subtle);</script>

@tniessen
Copy link
Member

@aduh95 I don't see how that helps. In browsers, there is already a crypto property attached to globalThis, and if users are intentionally using node:crypto (as in your example), then they can already import webcrypto from there and don't need this PR.

@jasnell
Copy link
Member Author

jasnell commented Dec 23, 2021

Would the global crypto be equivalent to require('crypto') or would it be require('crypto').webcrypto? Based on this PR, I'd assume it would be the crypto module, which seems inconsistent with other environments.

It would be require('crypto'), which, yes, is a bit inconsistent with browser environments but ideally in a way that "ain't that bad". Importantly, with the combination of that and this pr, references like crypto.subtle and crypto.randomUUID() and crypto.getRandomValues() should just work even if the rest of the API is not consistent with browsers. It's the closest we can likely get to being consistent.

Co-authored-by: Michaël Zasso <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 23, 2021

Would the global crypto be equivalent to require('crypto') or would it be require('crypto').webcrypto? Based on this PR, I'd assume it would be the crypto module, which seems inconsistent with other environments.

It would be require('crypto'), which, yes, is a bit inconsistent with browser environments but ideally in a way that "ain't that bad". Importantly, with the combination of that and this pr, references like crypto.subtle and crypto.randomUUID() and crypto.getRandomValues() should just work even if the rest of the API is not consistent with browsers. It's the closest we can likely get to being consistent.

why don't we put webcrypto (as crypto) on the global object? that would make more sense I think. that would be also consistent with browsers?

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Dec 23, 2021

@jasnell one thing I'm curious about:

let webcrypto;
function lazyWebCrypto() {
  webcrypto ??= require('internal/crypto/webcrypto');
  return webcrypto;
}

does require not just use the cache as well and return the exact same thing (instance) in this case? meaning, is this not pretty much the same, even from a performance standpoint:

function lazyWebCrypto() {
  return require('internal/crypto/webcrypto');
}

@tniessen
Copy link
Member

It would be require('crypto')

My concern is that

globalThis.crypto === require('crypto')

implies

globalThis.crypto !== require('crypto').webcrypto

unless

require('crypto') === require('crypto').webcrypto

which has implications such as

globalThis.crypto.constructor.name === 'Object'
globalThis.crypto.webcrypto.constructor.name === 'Crypto'

I get that we want to improve compatibility with code that uses web APIs, even if those weren't designed for Node.js, but exposing non-standard features through the standard globalThis.crypto API might add to the confusion and code that exclusively uses globalThis.crypto might work in Node.js, but not in a browser that supports Web Crypto.

Conversely, we could expose only Web Crypto as globalThis.crypto, but that would suggest that we consider Web Crypto to be the better alternative, which I am not sure we should at this point.

@jasnell
Copy link
Member Author

jasnell commented Dec 23, 2021

@dnalborczyk:

why don't we put webcrypto (as crypto) on the global object? that would make more sense I think. that would be also consistent with browsers?

We can't really. That would create an inconsistency with the Node.js REPL where crypto (from require('crypto')) is available globally.

@tniessen:

I get that we want to improve compatibility with code that uses web APIs, even if those weren't designed for Node.js, but exposing non-standard features through the standard globalThis.crypto API might add to the confusion and code that exclusively uses globalThis.crypto might work in Node.js, but not in a browser that supports Web Crypto.

Yes, there's a small risk there but I think it's manageable and I think the benefits here outweigh the risks. You are right, tho, the risks are non-zero.

@jasnell
Copy link
Member Author

jasnell commented Dec 24, 2021

@tniessen ... I just want to clarify, is your concern here blocking or can this PR go ahead and land?

@tniessen
Copy link
Member

I just want to clarify, is your concern here blocking or can this PR go ahead and land?

It's not blocking :) I haven't given this enough thought one way or the other to have an opinion here, I'm just wary of convenience changes with potentially significant API implications.

@aduh95 aduh95 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 27, 2021
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 27, 2021
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41266
✔  Done loading data for nodejs/node/pull/41266
----------------------------------- PR info ------------------------------------
Title      crypto: alias webcrypto.subtle and webcrypto.getRandomValues on crypto (#41266)
Author     James M Snell  (@jasnell)
Branch     jasnell:webcrypto-aliases -> nodejs:master
Labels     crypto, semver-minor, author ready, needs-ci, webcrypto, commit-queue-squash
Commits    3
 - crypto: alias webcrypto.subtle and webcrypto.getRandomValues on crypto
 - [Squash] nits
 - [Squash] nit
Committers 2
 - James M Snell 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/41266
Reviewed-By: Filip Skokan 
Reviewed-By: Michaël Zasso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41266
Reviewed-By: Filip Skokan 
Reviewed-By: Michaël Zasso 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - [Squash] nit
   ℹ  This PR was created on Tue, 21 Dec 2021 18:40:38 GMT
   ✔  Approvals: 2
   ✔  - Filip Skokan (@panva): https://github.com/nodejs/node/pull/41266#pullrequestreview-837777172
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/41266#pullrequestreview-838130176
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2021-12-23T01:16:25Z: https://ci.nodejs.org/job/node-test-pull-request/41621/
- Querying data for job/node-test-pull-request/41621/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1627255427

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 27, 2021
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 27, 2021
@nodejs-github-bot nodejs-github-bot merged commit 353532b into nodejs:master Dec 27, 2021
@nodejs-github-bot
Copy link
Collaborator

Landed in 353532b

targos pushed a commit that referenced this pull request Jan 14, 2022
The aliases allow code written to assume that `crypto.subtle` and
`crypto.getRandomValues()` exist on the `crypto` global to just work.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #41266
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos added a commit that referenced this pull request Jan 16, 2022
Notable changes:

crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) #41266
doc:
  * add Mesteery to collaborators (Mestery) #41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) #41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) #41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) #40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) #41153
stream:
  * add filter method to readable (Benjamin Gruenbaum) #41354
  * add map method to Readable (Benjamin Gruenbaum) #40815

PR-URL: TODO
targos added a commit that referenced this pull request Jan 17, 2022
Notable changes:

crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) #41266
doc:
  * add Mesteery to collaborators (Mestery) #41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) #41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) #41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) #40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) #41153
stream:
  * add filter method to readable (Benjamin Gruenbaum) #41354
  * add map method to Readable (Benjamin Gruenbaum) #40815

PR-URL: #41557
targos added a commit that referenced this pull request Jan 17, 2022
Notable changes:

child_process:
  * (SEMVER-MINOR) add support for URL to `cp.fork` (Antoine du Hamel) #41225
crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) #41266
doc:
  * add Mesteery to collaborators (Mestery) #41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) #41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) #41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) #40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) #41153
stream:
  * (SEMVER-MINOR) add filter method to readable (Benjamin Gruenbaum, Robert Nagy) #41354
  * (SEMVER-MINOR) add isReadable helper (Robert Nagy) #41199
  * (SEMVER-MINOR) add map method to Readable (Benjamin Gruenbaum, Robert Nagy) #40815

PR-URL: #41557
targos added a commit that referenced this pull request Jan 18, 2022
Notable changes:

child_process:
  * (SEMVER-MINOR) add support for URL to `cp.fork` (Antoine du Hamel) #41225
crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) #41266
doc:
  * add Mesteery to collaborators (Mestery) #41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) #41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) #41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) #40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) #41153
stream:
  * (SEMVER-MINOR) add filter method to readable (Benjamin Gruenbaum, Robert Nagy) #41354
  * (SEMVER-MINOR) add isReadable helper (Robert Nagy) #41199
  * (SEMVER-MINOR) add map method to Readable (Benjamin Gruenbaum, Robert Nagy) #40815

PR-URL: #41557
targos added a commit that referenced this pull request Jan 18, 2022
Notable changes:

child_process:
  * (SEMVER-MINOR) add support for URL to `cp.fork` (Antoine du Hamel) #41225
crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) #41266
doc:
  * add Mesteery to collaborators (Mestery) #41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) #41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) #41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) #40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) #41153
stream:
  * (SEMVER-MINOR) add filter method to readable (Benjamin Gruenbaum, Robert Nagy) #41354
  * (SEMVER-MINOR) add isReadable helper (Robert Nagy) #41199
  * (SEMVER-MINOR) add map method to Readable (Benjamin Gruenbaum, Robert Nagy) #40815

PR-URL: #41557
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
Notable changes:

child_process:
  * (SEMVER-MINOR) add support for URL to `cp.fork` (Antoine du Hamel) nodejs#41225
crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) nodejs#41266
doc:
  * add Mesteery to collaborators (Mestery) nodejs#41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) nodejs#41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) nodejs#41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) nodejs#40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) nodejs#41153
stream:
  * (SEMVER-MINOR) add filter method to readable (Benjamin Gruenbaum, Robert Nagy) nodejs#41354
  * (SEMVER-MINOR) add isReadable helper (Robert Nagy) nodejs#41199
  * (SEMVER-MINOR) add map method to Readable (Benjamin Gruenbaum, Robert Nagy) nodejs#40815

PR-URL: nodejs#41557
@aduh95
Copy link
Contributor

aduh95 commented Jan 30, 2022

I don't think the getRandomValues shortcut should land on LTS given #41760. We should probably mark them as experimental given the whole WebCrypto API is itself experimental.
Adding backport-requested label in case you think it's still worth it to have SubtleCrypto shortcut and want to backport that.

Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
The aliases allow code written to assume that `crypto.subtle` and
`crypto.getRandomValues()` exist on the `crypto` global to just work.

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#41266
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
Notable changes:

child_process:
  * (SEMVER-MINOR) add support for URL to `cp.fork` (Antoine du Hamel) nodejs#41225
crypto:
  * (SEMVER-MINOR) alias webcrypto.subtle and webcrypto.getRandomValues on crypto (James M Snell) nodejs#41266
doc:
  * add Mesteery to collaborators (Mestery) nodejs#41543
events:
  * (SEMVER-MINOR) graduate capturerejections to supported (James M Snell) nodejs#41267
  * (SEMVER-MINOR) add EventEmitterAsyncResource to core (James M Snell) nodejs#41246
loader:
  * (SEMVER-MINOR) return package format from defaultResolve if known (Gabriel Bota) nodejs#40980
perf_hooks:
  * (SEMVER-MINOR) multiple fixes for Histogram (James M Snell) nodejs#41153
stream:
  * (SEMVER-MINOR) add filter method to readable (Benjamin Gruenbaum, Robert Nagy) nodejs#41354
  * (SEMVER-MINOR) add isReadable helper (Robert Nagy) nodejs#41199
  * (SEMVER-MINOR) add map method to Readable (Benjamin Gruenbaum, Robert Nagy) nodejs#40815

PR-URL: nodejs#41557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants