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: fix setEngine() when OPENSSL_NO_ENGINE set #47977

Merged
merged 1 commit into from
May 15, 2023
Merged

crypto: fix setEngine() when OPENSSL_NO_ENGINE set #47977

merged 1 commit into from
May 15, 2023

Conversation

tniessen
Copy link
Member

When OpenSSL is configured with OPENSSL_NO_ENGINE, setEngine() currently throws an internal error because the C++ binding does not export the relevant function, which causes _setEngine() to be undefined within JS.

Instead, match the behavior of tls/secure-context.js and throw the existing error code ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED when OpenSSL has been configured with OPENSSL_NO_ENGINE.

For reference:

node/lib/internal/tls/secure-context.js

Lines 202 to 205 in 7984af6

if (context.setEngineKey)
context.setEngineKey(privateKeyIdentifier, privateKeyEngine);
else
throw new ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED();

node/lib/internal/tls/secure-context.js

Lines 285 to 288 in 7984af6

if (typeof context.setClientCertEngine !== 'function')
throw new ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED();
else
context.setClientCertEngine(clientCertEngine);

@tniessen
crypto: fix setEngine() when OPENSSL_NO_ENGINE set
742bd83 
When OpenSSL is configured with OPENSSL_NO_ENGINE, setEngine() currently
throws an internal error because the C++ binding does not export the
relevant function, which causes _setEngine() to be undefined within JS.

Instead, match the behavior of tls/secure-context.js and throw the
existing error code ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED when OpenSSL
has been configured with OPENSSL_NO_ENGINE.
@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. openssl Issues and PRs related to the OpenSSL dependency. labels May 12, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 12, 2023
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request May 14, 2023
Open
14 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request May 15, 2023
Open
17 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label May 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 15, 2023
@nodejs-github-bot nodejs-github-bot merged commit 5b4c7bd into nodejs:main May 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5b4c7bd

This was referenced May 16, 2023
Open
Open
Open
Open
targos pushed a commit that referenced this pull request May 30, 2023
@tniessen @targos
crypto: fix setEngine() when OPENSSL_NO_ENGINE set
15c2de4 
When OpenSSL is configured with OPENSSL_NO_ENGINE, setEngine() currently
throws an internal error because the C++ binding does not export the
relevant function, which causes _setEngine() to be undefined within JS.

Instead, match the behavior of tls/secure-context.js and throw the
existing error code ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED when OpenSSL
has been configured with OPENSSL_NO_ENGINE.

PR-URL: #47977
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@targos targos mentioned this pull request Jun 4, 2023
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@tniessen @danielleadams
crypto: fix setEngine() when OPENSSL_NO_ENGINE set
9d69843 
When OpenSSL is configured with OPENSSL_NO_ENGINE, setEngine() currently
throws an internal error because the C++ binding does not export the
relevant function, which causes _setEngine() to be undefined within JS.

Instead, match the behavior of tls/secure-context.js and throw the
existing error code ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED when OpenSSL
has been configured with OPENSSL_NO_ENGINE.

PR-URL: #47977
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@tniessen @MoLow
crypto: fix setEngine() when OPENSSL_NO_ENGINE set
8cabfe7 
When OpenSSL is configured with OPENSSL_NO_ENGINE, setEngine() currently
throws an internal error because the C++ binding does not export the
relevant function, which causes _setEngine() to be undefined within JS.

Instead, match the behavior of tls/secure-context.js and throw the
existing error code ERR_CRYPTO_CUSTOM_ENGINE_NOT_SUPPORTED when OpenSSL
has been configured with OPENSSL_NO_ENGINE.

PR-URL: nodejs#47977
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@lpinca lpinca lpinca approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@panva panva panva approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tniessen @nodejs-github-bot @panva @bnoordhuis @lpinca @richardlau