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.

Sign up for GitHub

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

Jump to bottom

crypto: remove BoringSSL dh-primes addition #57023

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

crypto: remove BoringSSL dh-primes addition #57023

wants to merge 1 commit into from
+0 −314

Conversation

codebytere
Copy link
Member

I don't believe these are necessary for current versions of BoringSSL.

The added functions:

  • BN_get_rfc3526_prime_2048
  • BN_get_rfc3526_prime_3072
  • BN_get_rfc3526_prime_4096
  • BN_get_rfc3526_prime_6144
  • BN_get_rfc3526_prime_8192

can be found here and were added in https://boringssl-review.googlesource.com/c/boringssl/+/54305

Electron compiles without this file without issue.

@codebytere codebytere requested a review from jasnell February 13, 2025 09:35
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Feb 13, 2025
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65173/

@jasnell
Copy link
Member

jasnell commented Feb 13, 2025 •
edited
Loading

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.

@codebytere
Copy link
Member Author

@jasnell as i understand it yes (see linked CL in PR body) - done at nodejs/ncrypto#5

@jasnell
Copy link
Member

jasnell commented 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.

@jasnell
Copy link
Member

jasnell commented Feb 14, 2025 •
edited
Loading

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 codebytere added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Feb 25, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/65427/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@codebytere @nodejs-github-bot @jasnell @lpinca @joyeecheung @richardlau