crypto: reject dh,x25519,x448 in {Sign,Verify}Final #53774
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
There was a problem hiding this comment.
Thank you for the contribution, well-done spotting the bug @JLHwung!
I'll review the actual implementation later. For now, could you please add the OpenSSL commands for generating the files in test/fixtures/keys to the Makefile in that directory, and then replace the new files by deleting them locally and running make to re-generate them?
|
@tniessen Thank you for reviewing. PR is updated. |
|
@tniessen ping. Could you take another look? |
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
github-actions
bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
panva
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/53774
✔ Done loading data for nodejs/node/pull/53774
----------------------------------- PR info ------------------------------------
Title crypto: reject dh,x25519,x448 in {Sign,Verify}Final (#53774)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch JLHwung:fix-node-signfinal-evp-pkey-usage -> nodejs:main
Labels crypto, c++, needs-ci
Commits 6
- crypto: reject dh,x25519,x448 in {Sign,Verify}Final
- format cpp
- generate fixture dh keys from openssl
- add test comment
- fix linter-js error
- Update test-crypto-sign-verify.js
Committers 2
- Huáng Jùnliàng <jlhwung@gmail.com>
- GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
--------------------------------------------------------------------------------
⚠ Commits were pushed since the last approving review:
⚠ - Update test-crypto-sign-verify.js
ℹ This PR was created on Tue, 09 Jul 2024 02:14:17 GMT
✔ Approvals: 2
✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53774#pullrequestreview-2170042008
✔ - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/53774#pullrequestreview-2225381615
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-08-29T08:17:53Z: https://ci.nodejs.org/job/node-test-pull-request/61644/
- Querying data for job/node-test-pull-request/61644/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/10613281105 |
|
@jasnell @tniessen can you re-review the latest state and
commit-queue
|
panva
removed
the
commit-queue-failed
panva
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
Commit Queue failed- Loading data for nodejs/node/pull/53774
✔ Done loading data for nodejs/node/pull/53774
----------------------------------- PR info ------------------------------------
Title crypto: reject dh,x25519,x448 in {Sign,Verify}Final (#53774)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch JLHwung:fix-node-signfinal-evp-pkey-usage -> nodejs:main
Labels crypto, c++, needs-ci
Commits 6
- crypto: reject dh,x25519,x448 in {Sign,Verify}Final
- format cpp
- generate fixture dh keys from openssl
- add test comment
- fix linter-js error
- Update test-crypto-sign-verify.js
Committers 2
- Huáng Jùnliàng <jlhwung@gmail.com>
- GitHub <noreply@github.com>
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
--------------------------------------------------------------------------------
ℹ This PR was created on Tue, 09 Jul 2024 02:14:17 GMT
✔ Approvals: 2
✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53774#pullrequestreview-2283542175
✔ - Tobias Nießen (@tniessen) (TSC): https://github.com/nodejs/node/pull/53774#pullrequestreview-2225381615
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2024-08-29T10:28:10Z: https://ci.nodejs.org/job/node-test-pull-request/61644/
- Querying data for job/node-test-pull-request/61644/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ No git cherry-pick in progress
✔ No git am in progress
✔ No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
* branch main -> FETCH_HEAD
✔ origin/main is now up-to-date
- Downloading patch for 53774
From https://github.com/nodejs/node
* branch refs/pull/53774/merge -> FETCH_HEAD
✔ Fetched commits as c046c9b3d8bc..3f90853e83e3
--------------------------------------------------------------------------------
Auto-merging src/crypto/crypto_sig.cc
[main 1c15b23ddb] crypto: reject dh,x25519,x448 in {Sign,Verify}Final
Author: Huáng Jùnliàng <jlhwung@gmail.com>
Date: Mon Jul 8 17:59:52 2024 -0400
4 files changed, 63 insertions(+), 11 deletions(-)
create mode 100644 test/fixtures/keys/dh_private.pem
create mode 100644 test/fixtures/keys/dh_public.pem
Auto-merging src/crypto/crypto_sig.cc
[main ee73052bfa] format cpp
Author: Huáng Jùnliàng <jlhwung@gmail.com>
Date: Tue Jul 9 09:43:28 2024 -0400
1 file changed, 11 insertions(+), 10 deletions(-)
[main b6a633e621] generate fixture dh keys from openssl
Author: Huáng Jùnliàng <jlhwung@gmail.com>
Date: Tue Jul 9 09:56:12 2024 -0400
3 files changed, 29 insertions(+), 16 deletions(-)
[main 27abfd6280] add test comment
Author: Huáng Jùnliàng <jlhwung@gmail.com>
Date: Tue Jul 9 10:14:37 2024 -0400
1 file changed, 2 insertions(+)
[main 8722e00458] fix linter-js error
Author: Huáng Jùnliàng <jlhwung@gmail.com>
Date: Wed Jul 10 16:05:03 2024 -0400
1 file changed, 1 insertion(+), 1 deletion(-)
[main 39dea715b8] Update test-crypto-sign-verify.js
Author: Huáng Jùnliàng <jlhwung@gmail.com>
Date: Wed Aug 7 10:46:10 2024 -0400
1 file changed, 3 insertions(+), 15 deletions(-)
✔ Patches applied
There are 6 commits in the PR. Attempting autorebase.
Rebasing (2/12)
https://github.com/nodejs/node/actions/runs/10745754619 |
nodejs-github-bot
added
the
commit-queue-failed
panva
added
commit-queue-squash
nodejs-github-bot
removed
the
commit-queue
|
Landed in 18101d8 |
Fixes: #53742
In this PR we handle the return value of
EVP_PKEY_{sign,verify}_init, when it returns-2, we throw theERR_OSSL_EVP_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPEerror. This approach is future proof as we don't have to maintain a list of key types that can not be used with signing / verifying.