tools: add ncrypto updater script

[aduh95]

With nodejs/ncrypto#17 in a release, I think there's no reason to keep maintaining a separate copy of ncrypto in this repo.

[nodejs-github-bot]

Review requested:

• @nodejs/actions
• @nodejs/security-wg
• @nodejs/tsc

[panva]

I think there's no reason to keep maintaining a separate copy of ncrypto in this repo.

I think this misses the point, ncrypto is the separate copy of node's crypto internals. Not the other way around. Not yet.

The entire reason for ncrypto's existance was to make node's internals available to other runtimes. Setting up a process where nodejs/node latest releases get pulled to and tagged in nodejs/ncrypto seems more appropriate to me.

ncrypto is a tool for non-node runtimes, not for node.

ncrypto changes are not tested with the same CI rigour, with the same openssl matrix, are not reviewed by @nodejs/crypto and there's no guarantee that changes landing in ncrypto are compatible with node. Waiting for ncrypto releases to make dependant changes in node is not a process that would welcome contributions. I for one cannot imagine having to do all of the post-quantum work last year crippled by ncrypto having to have code merged and released first.

Switching ncrypto to be a full blown dependency is still a long way off (if ever). I raised this in nodejs/ncrypto#7 and that still stands.

As is I am not convinced the nodejs project is better off relying on ncrypto releases. If anything, things will live in src/crypto again.

[anonrig]

ncrypto is a tool for non-node runtimes, not for node.

Yes I definitely agree but we don't run certain things on deps folder, such as linting testing etc. In order to even run individual tests, either we append our test runner here with unrelated tests, such as Bazel etc, or we just behave like it's a dependency just like another.

I think it's less work to act like it's Ada/v8 etc.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.86%. Comparing base (f6464c5) to head (ae2825f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61613      +/-   ##
==========================================
- Coverage   89.76%   88.86%   -0.90%     
==========================================
  Files         673      673              
  Lines      203944   203944              
  Branches    39191    39109      -82     
==========================================
- Hits       183080   181245    -1835     
- Misses      13194    14970    +1776     
- Partials     7670     7729      +59     

see 108 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aduh95]

Adding the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label as both approaches sound reasonable, and maybe we would benefit from having a quick vote.

[joyeecheung]

IIUC, the intention of this PR goes beyond a simple tool update - it will make https://github.com/nodejs/ncrypto the source of truth from now on, and every update must be there first before it can be rolled into this repo?

[joyeecheung]

I see there's a thumbs up - I think in that case, apart from the issues raised by @panva I also spotted some issues:

1. ncrypto does not provide gyp configurations at the moment. That means update can break when being rolled in here due to lack of support in the build configuration. While this happens to other dependencies too, those are usually not in the Node.js organization. This one specifically is part of Node.js, so not requiring a working gyp configuration there and allowing contributors to land changes there that can't be built with GYP, only to deal the breakage during the update here feels like shooting ourselves in the foot.
2. How would the security release be handled for ncrypto? At the moment all the security triage and fixes are done in Node.js HackerOne and nodejs-private. That means we'll still need to maintain floated patches?

I think the biggest issue, as also raised by @panva, is how would dynamically linked OpenSSL builds be tested there? And all the versions still supported by Node.js (including OpenSSL 1.1.1?). That's fairly important for Linux distributions. And we need to make sure that it works in the final Node.js builds too - otherwise we might again need to deal with breakages during ncrypto updates.

I am not against the idea per-se, but at the moment it seems these questions are largely left unanswered and not enough work has been done to make them go away?

[joyeecheung]

such as linting testing etc. In order to even run individual tests, either we append our test runner here with unrelated tests, such as Bazel etc, or we just behave like it's a dependency just like another.

I think it would be possible to just add additional make targets to run linting and testing for deps/ncrypto - we do have for example make test-v8 that runs V8 tests, and the corresponding CI jobs for our fork of V8. We just don't run them normally unless the PR touches deps/v8.