node

node copied to clipboard

Reame
Issues

crypto: reject dh,x25519,x448 in {Sign,Verify}Final

Open JLHwung opened this issue 1 year ago • 13 comments

Fixes: https://github.com/nodejs/node/issues/53742

In this PR we handle the return value of EVP_PKEY_{sign,verify}_init, when it returns -2, we throw the ERR_OSSL_EVP_OPERATION_NOT_SUPPORTED_FOR_THIS_KEYTYPE error. 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.

Jul 09 '24 02:07 JLHwung

Review requested:

[ ] @nodejs/crypto

Jul 09 '24 02:07 nodejs-github-bot

@tniessen Thank you for reviewing. PR is updated.

Jul 09 '24 14:07 JLHwung

@tniessen ping. Could you take another look?

Jul 29 '24 14:07 JLHwung

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

Aug 07 '24 15:08 nodejs-github-bot

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

Aug 08 '24 06:08 nodejs-github-bot

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

Aug 26 '24 14:08 nodejs-github-bot

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

Aug 26 '24 14:08 nodejs-github-bot

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

Aug 26 '24 14:08 nodejs-github-bot

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

Aug 26 '24 14:08 nodejs-github-bot

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

Aug 26 '24 14:08 nodejs-github-bot

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

Aug 27 '24 07:08 nodejs-github-bot

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

Aug 27 '24 09:08 nodejs-github-bot

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

Aug 27 '24 17:08 nodejs-github-bot

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

Aug 29 '24 06:08 nodejs-github-bot

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

Aug 29 '24 08:08 nodejs-github-bot

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 <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
--------------------------------------------------------------------------------
   ⚠  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/.ncu

https://github.com/nodejs/node/actions/runs/10613281105

Aug 29 '24 10:08 nodejs-github-bot

@jasnell @tniessen can you re-review the latest state and https://github.com/nodejs/node/labels/commit-queue afterwards?

Aug 29 '24 13:08 panva

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 <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
--------------------------------------------------------------------------------
   ℹ  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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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 <[email protected]>
 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)
Executing: git node land --amend --yes
⚠  Found Fixes: https://github.com/nodejs/node/issues/53742, skipping..
--------------------------------- New Message ----------------------------------
crypto: reject dh,x25519,x448 in {Sign,Verify}Final
Fixes: https://github.com/nodejs/node/issues/53742
PR-URL: https://github.com/nodejs/node/pull/53774
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
[detached HEAD a344e8ff9e] crypto: reject dh,x25519,x448 in {Sign,Verify}Final
Author: Huáng Jùnliàng <[email protected]>
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
Rebasing (3/12)
Rebasing (4/12)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
format cpp
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
[detached HEAD 366e36419c] format cpp
Author: Huáng Jùnliàng <[email protected]>
Date: Tue Jul 9 09:43:28 2024 -0400
1 file changed, 11 insertions(+), 10 deletions(-)
Rebasing (5/12)
Rebasing (6/12)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
generate fixture dh keys from openssl
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
[detached HEAD f34822ce9a] generate fixture dh keys from openssl
Author: Huáng Jùnliàng <[email protected]>
Date: Tue Jul 9 09:56:12 2024 -0400
3 files changed, 29 insertions(+), 16 deletions(-)
Rebasing (7/12)
Rebasing (8/12)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
add test comment
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
[detached HEAD d898a87301] add test comment
Author: Huáng Jùnliàng <[email protected]>
Date: Tue Jul 9 10:14:37 2024 -0400
1 file changed, 2 insertions(+)
Rebasing (9/12)
Rebasing (10/12)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix linter-js error
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
[detached HEAD 981b557a9c] fix linter-js error
Author: Huáng Jùnliàng <[email protected]>
Date: Wed Jul 10 16:05:03 2024 -0400
1 file changed, 1 insertion(+), 1 deletion(-)
Rebasing (11/12)
Rebasing (12/12)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Update test-crypto-sign-verify.js
Co-authored-by: Tobias Nießen <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/53774
Fixes: https://github.com/nodejs/node/issues/53742
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
[detached HEAD 64f5998fc5] Update test-crypto-sign-verify.js
Author: Huáng Jùnliàng <[email protected]>
Date: Wed Aug 7 10:46:10 2024 -0400
1 file changed, 3 insertions(+), 15 deletions(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10745754619

Sep 06 '24 21:09 nodejs-github-bot

Landed in 18101d83a158b877ac765936aba973c664130ea2

Sep 06 '24 22:09 nodejs-github-bot

Labels

crypto

c++

needs-ci

Owner

Other Repo Issues