deps: start working on ncrypto dep - #53803
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
Is this specifically for workerd or have other runtimes expressed interest in this?
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? |
Sorry, something went wrong.
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.
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.
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. |
Sorry, something went wrong.
anonrig
left a comment
There was a problem hiding this comment.
Can you add version and add it to node metadata just like nbytes?
Sorry, something went wrong.
Already done https://github.com/nodejs/node/pull/53803/files#diff-906d4d2a785066ab68f8cbadfdf32b34991648ec259d97b61be70b0ed502bf3c |
Sorry, something went wrong.
This comment was marked as outdated.
We (Bun) are interested! |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
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 <yagiz@nizipli.com>
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 <yagiz@nizipli.com>
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 <yagiz@nizipli.com>
The ncrypto move introduced regressions in cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it. PR-URL: #56554 Fixes: #56375 Refs: #53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The ncrypto move introduced regressions in cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it. PR-URL: #56554 Fixes: #56375 Refs: #53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The ncrypto move introduced regressions in cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it. PR-URL: #56554 Fixes: #56375 Refs: #53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The ncrypto move introduced regressions in cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it. PR-URL: #56554 Fixes: #56375 Refs: #53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The ncrypto move introduced regressions in cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it. PR-URL: #56554 Fixes: #56375 Refs: #53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
The ncrypto move introduced regressions in cryptoErrorListToException() by passing in the size of the vector unnecessarily into the vector constructor and then use push_back() (which would result in a crash on dereferencing empty handles during later iteration) and having incorrect logic for checking the presence of an exception. This patch fixes it. PR-URL: nodejs#56554 Fixes: nodejs#56375 Refs: nodejs#53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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/node#53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
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/node#53803 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
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.