crypto: remove BoringSSL dh-primes addition #57023
Merged
+0
−314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review requested:
|
nodejs-github-bot
added
dependencies
github-actions
bot
removed
the
request-ci
richardlau
approved these changes
Feb 13, 2025
|
Are these now supported in boringssl+fips? Also, just fyi, I'm a few days away from opening a PR to switch deps/ncrypto to the version coming from the nodejs/ncrypto repo, at which time these changes should go there. It would be worth opening a PR there also. |
|
@jasnell as i understand it yes (see linked CL in PR body) - done at nodejs/ncrypto#5 |
joyeecheung
approved these changes
Feb 13, 2025
|
Actually, these are going to need to be kept in ncrypto to support older boringssl + fips configurations where these curves are unspecified. I've got a PR that makes sure they are blocked behind a define flag that will be off in our usage here. Landing this PR is fine since it won't impact node.js at all. |
lpinca
approved these changes
Feb 14, 2025
|
Just to be clear... when we switch over to the repo-version .. these conditionally added primes will still be here, so removing them here is only temporary. There is an additional guard there, however, that ensures they are injected only when specifically needed rather than always when boringssl is used. |
codebytere
force-pushed
the
remove-dh-primes
branch
from
95b3860 to
f1614d4
Compare
codebytere
added
request-ci
github-actions
bot
removed
the
request-ci
codebytere
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/57023 ✔ Done loading data for nodejs/node/pull/57023 ----------------------------------- PR info ------------------------------------ Title crypto: remove BoringSSL dh-primes addition (#57023) Author Shelley Vohr <[email protected]> (@codebytere) Branch codebytere:remove-dh-primes -> nodejs:main Labels dependencies Commits 1 - crypto: remove BoringSSL dh-primes addition Committers 1 - Shelley Vohr <[email protected]> PR-URL: https://github.com/nodejs/node/pull/57023 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/57023 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - crypto: remove BoringSSL dh-primes addition ℹ This PR was created on Thu, 13 Feb 2025 09:35:24 GMT ✔ Approvals: 3 ✔ - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/57023#pullrequestreview-2615045737 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/57023#pullrequestreview-2615449282 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/57023#pullrequestreview-2618471607 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-02-25T11:17:09Z: https://ci.nodejs.org/job/node-test-pull-request/65427/ - Querying data for job/node-test-pull-request/65427/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/13836438978 |
aduh95
added
author ready
github-actions
bot
removed
the
request-ci
|
Landed in b9b4cb7 |
JonasBa
pushed a commit
to JonasBa/node
that referenced
this pull request
Apr 11, 2025
PR-URL: nodejs#57023 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I don't believe these are necessary for current versions of BoringSSL.
The added functions:
BN_get_rfc3526_prime_2048BN_get_rfc3526_prime_3072BN_get_rfc3526_prime_4096BN_get_rfc3526_prime_6144BN_get_rfc3526_prime_8192can be found here and were added in https://boringssl-review.googlesource.com/c/boringssl/+/54305
Electron compiles without this file without issue.