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

deps: start working on ncrypto dep #53803

Closed
wants to merge 1 commit into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 10, 2024

The idea here is to start moving the bulk of the implementation of src/crypto/*.h and src/crypto.c++ into a separate dependency that can be used by other runtimes that are seeking to emulate Node.js' behaviors.

Implementing this will be incremental since there is a lot of code to migrate. Accordingly, while the idea is to eventually move ncrypto into its own nodejs/ncrypto repo, it will remain only in deps until most of the functionality that will be moved to it is moved. Doing so will make it much easier to review individual changes that move function out to the dep.

@jasnell jasnell requested review from anonrig, tniessen and mhdawson July 10, 2024 18:48
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/gyp
  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 10, 2024
@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. dependencies Pull requests that update a dependency file. labels Jul 10, 2024
@tniessen
Copy link
Member

a separate dependency that can be used by other runtimes that are seeking to emulate Node.js' behaviors.

Is this specifically for workerd or have other runtimes expressed interest in this?

while the idea is to eventually move ncrypto into its own nodejs/ncrypto repo

What about versioning of such a dependency? If multiple other runtimes depend on this library, wouldn't they expect API/ABI stability that we currently do not provide for such internal bindings?

Regarding contributions, would future pull requests then target nodejs/ncrypto, and would this repository then have its own set of test suites?

@jasnell
Copy link
Member Author

jasnell commented Jul 10, 2024

Is this specially for workerd...

That's the immediate target for me, yes, but I wouldn't say it is only for that.

FWIW, I'm also hoping that by separating this out we can more easily deal with various openssl updates, differences with boringssl in environments that use that, and provide some isolate to work on performance and functionality improvements.

What about versioning of such a dependency? If multiple other runtimes depend on this library, wouldn't they expect API/ABI stability that we currently do not provide for such internal bindings?

I think we address that problem if/when it becomes a problem. For workerd we really wouldn't need to expect strict API/ABI stability as we would be able to track any updates accordingly.

Regarding contributions, would future pull requests then target nodejs/ncrypto, and would this repository then have its own set of test suites?

Yes. Future PRs that specifically touch the underlying c++ code would happen there. The nodejs/ncrypto repo would have its own set of tests that would need to be created as part of this effort since we currently do not have any c++ tests for the low level crypto stuff in the runtime.

src/crypto/crypto_util.cc Show resolved Hide resolved
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
src/crypto/crypto_util.cc Show resolved Hide resolved
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Can you add version and add it to node metadata just like nbytes?

@jasnell
Copy link
Member Author

jasnell commented Jul 10, 2024

@nodejs-github-bot

This comment was marked as outdated.

src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
src/crypto/crypto_util.cc Outdated Show resolved Hide resolved
@Jarred-Sumner
Copy link

Is this specifically for workerd or have other runtimes expressed interest in this?

We (Bun) are interested!

@jasnell
Copy link
Member Author

jasnell commented Jul 10, 2024

We (Bun) are interested!

Awesome. Yeah, I figured y'all might be. Re-implementing this stuff so that it works consistently with node.js and tracks changes is a ton of work. Separating out the key bits that do not directly depend on v8 will make life a whole lot easier and will make it easier to be actually compatible with node.js

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@panva
Copy link
Member

panva commented Jul 14, 2024

the idea is to eventually move ncrypto into its own nodejs/ncrypto repo

I think it's worth pointing out that this might reduce visibility in this subsystem's development/maintenance. Example being nodejs/undici contributions and releases that occasionaly include breaking behaviours in a core API.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell
Copy link
Member Author

jasnell commented Jul 15, 2024

...Example being nodejs/undici contributions and releases that occasionaly include breaking behaviours in a core API.

Noted and agreed that is a risk. I think the benefits for cross-runtime compatibility and preventing divergence across implementations are benefits that outweigh the risk.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 15, 2024

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Jul 15, 2024
Start moving src/crypto functionality out to a separate dep that
can be shared with other projects that need to emulate Node.js
crypto behavior.
jasnell added a commit that referenced this pull request Jul 18, 2024
Start moving src/crypto functionality out to a separate dep that
can be shared with other projects that need to emulate Node.js
crypto behavior.

PR-URL: #53803
Reviewed-By: Yagiz Nizipli <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Jul 18, 2024

Landed in efe5b81

@jasnell jasnell closed this Jul 18, 2024
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
Start moving src/crypto functionality out to a separate dep that
can be shared with other projects that need to emulate Node.js
crypto behavior.

PR-URL: nodejs#53803
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request Jul 28, 2024
Start moving src/crypto functionality out to a separate dep that
can be shared with other projects that need to emulate Node.js
crypto behavior.

PR-URL: #53803
Reviewed-By: Yagiz Nizipli <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 30, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
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. crypto Issues and PRs related to the crypto subsystem. dependencies Pull requests that update a dependency file. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants