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

crypto: don't expose OpenSSL internals #29325

Closed

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 26, 2019

#28335 extended RSA-OAEP support with oaepHash, which works perfectly fine for OpenSSL but unfortunately is a bespoke internal for BoringSSL, and so fails to compile in Electron as BoringSSL does not expose EVP_PKEY_OP_TYPE_CRYPT or EVP_PKEY_CTRL_RSA_OAEP_MD.

OpenSSL has a macro EVP_PKEY_CTX_set_rsa_oaep_md that accomplished the selfsame purpose as documented, and which this PR switches to in order to allow BoringSSL compilation.

Passing a bad or null digest to this new macro returns an error like ERR_OSSL_BAD_DECODE and as such the error i've added to node_errors allows for a thrown error that retains the previous context.

cc @tniessen @ryzokuken

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 26, 2019
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to land after @tniessen or @sam-github approve. Thanks for this!

src/node_crypto.cc Outdated Show resolved Hide resolved
@codebytere codebytere changed the title crypto: don't expose openssl internals crypto: don't expose OpenSSL internals Aug 27, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Looks good!

@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 29, 2019
@ryzokuken
Copy link
Contributor

CI passed too! Landing this.

ryzokuken pushed a commit that referenced this pull request Aug 29, 2019
PR-URL: #29325
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@ryzokuken
Copy link
Contributor

Landed in 17a697c 🎉

@ryzokuken ryzokuken closed this Aug 29, 2019
@codebytere codebytere deleted the crypto-oaepHash-boringssl branch August 29, 2019 05:33
BridgeAR pushed a commit that referenced this pull request Sep 3, 2019
PR-URL: #29325
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR pushed a commit that referenced this pull request Sep 4, 2019
PR-URL: #29325
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 5, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 6, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 19, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 23, 2019
codebytere added a commit to electron/electron that referenced this pull request Sep 27, 2019
codebytere added a commit to electron/electron that referenced this pull request Oct 14, 2019
codebytere added a commit to electron/electron that referenced this pull request Oct 14, 2019
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. c++ Issues and PRs that require attention from people who are familiar with C++. 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.

8 participants