crypto: fix `generateKeyPair` with encoding 'jwk' by himself65 · Pull Request #39319 · nodejs/node · GitHub

@nodejs/crypto

The PR's on a good track, but lacks some of the details that are present in KeyObject.prototype.export

more concretely

I think there are a few ways of moving forward

either moving these asserts to C and have export just call the appropriate C method
having generate piggyback on a keyobject and then returning the results of calling export on those
a combination of those, e.g. leave the passphrase part in JS but move the rest to C

kAsymmetricKeyJWKProperties is not a static method, so we have to call it after initialized.'

I'd like to move these to the C part. because the second way must have performance regression. And the third way looks like a too complex struct.

Fixes: nodejs#39205

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

Co-authored-by: Filip Skokan <panva.ip@gmail.com>

I've moved kAsymmetricKeyJWKProperties to the c++ part. is it correct?

node/src/crypto/crypto_ec.cc

Lines 790 to 812 in 95bd164

    
           const char* curve = nullptr; 
        
           switch (EVP_PKEY_id(pkey.get())) { 
        
             case EVP_PKEY_ED25519: 
        
               curve = "Ed25519"; 
        
               break; 
        
             case EVP_PKEY_ED448: 
        
               curve = "Ed448"; 
        
               break; 
        
             case EVP_PKEY_X25519: 
        
               curve = "X25519"; 
        
               break; 
        
             case EVP_PKEY_X448: 
        
               curve = "X448"; 
        
               break; 
        
             default: 
        
               UNREACHABLE(); 
        
           } 
        
           if (target->Set( 
        
                   env->context(), 
        
                   env->jwk_crv_string(), 
        
                   OneByteString(env->isolate(), curve)).IsNothing()) { 
        
             return Nothing<bool>(); 
        
           }

addressed

/cc @nodejs/crypto

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

Does anyone review this pr? I think this is a nice fix 🤔

cc @nodejs/crypto

Landed in 257312a

Fixes: #39205 PR-URL: #39319 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs/node#39319

* chore: bump node in DEPS to v16.6.0 * chore: bump node in DEPS to v16.6.1 * crypto: fix generateKeyPair with encoding 'jwk' nodejs/node#39319 * build: add library_files to gyp variables nodejs/node#39293 * debugger: rename internal module nodejs/node#39378 * chore: fixup patch indices * deps: extract gtest source files to deps/googletest nodejs/node#39386 * crypto: fix generateKeyPair with encoding 'jwk' nodejs/node#39319 * deps: bump HdrHistogram_C to 0.11.2 nodejs/node#39462 * fixup! deps: extract gtest source files to deps/googletest * chore: bump node in DEPS to v16.6.2 * chore: update patches * deps: reflect c-ares source tree nodejs/node#39653 * deps: update c-ares to 1.17.2 nodejs/node#39724 * fix: _ReadBarrier undefined symbol error on WOA arm64 * chore: update patches * chore: bump node in DEPS to v16.7.0 * deps: upgrade to libuv 1.42.0 nodejs/node#39525 * chore: update filenames * src: remove extra semicolons outside fns * chore: fixup patch filenames * chore: sort and alphabetize disabled tests Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>

himself65 added the crypto Issues and PRs related to the crypto subsystem. label Jul 9, 2021

himself65 requested a review from jasnell July 9, 2021 04:09

github-actions 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. needs-ci PRs that need a full CI run. labels Jul 9, 2021

himself65 changed the title crypto: fix enerateKeyPair with encoding 'jwk' crypto: fix generateKeyPair with encoding 'jwk' Jul 9, 2021

himself65 force-pushed the 20210709-crypto-fix branch 2 times, most recently from 8050957 to 68df3f7 Compare July 9, 2021 04:52

VoltrexKeyva reviewed Jul 9, 2021

View reviewed changes

Comment thread lib/internal/crypto/keygen.js Outdated

VoltrexKeyva reviewed Jul 9, 2021

View reviewed changes

Comment thread lib/internal/crypto/keygen.js Outdated

panva self-requested a review July 10, 2021 21:03

panva requested changes Jul 11, 2021

View reviewed changes

Comment thread test/parallel/test-crypto-keygen.js Outdated

panva previously requested changes Jul 11, 2021

View reviewed changes

Comment thread test/parallel/test-crypto-keygen.js Outdated

himself65 force-pushed the 20210709-crypto-fix branch 2 times, most recently from da40de3 to 790e4ff Compare July 14, 2021 02:51

himself65 and others added 5 commits July 19, 2021 11:00

crypto: fix generateKeyPair with encoding 'jwk'

032bb15

Fixes: nodejs#39205

fixup!: Update lib/internal/crypto/keygen.js

edcc4c1

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

fixup!: Update lib/internal/crypto/keygen.js

5a20497

Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>

fixup!: Update test/parallel/test-crypto-keygen.js

f81c859

Co-authored-by: Filip Skokan <panva.ip@gmail.com>

fixup!: Update test/parallel/test-crypto-keygen.js

5d0b081

Co-authored-by: Filip Skokan <panva.ip@gmail.com>

himself65 force-pushed the 20210709-crypto-fix branch from 790e4ff to 5d0b081 Compare July 19, 2021 03:01

himself65 added 2 commits July 19, 2021 14:05

fixup!: passing format 'jwk'

a5672f1

fixup!: move crv_name check to c++ part

95bd164

himself65 added 3 commits July 19, 2021 15:40

fixup!: lint fixes

353a397

fixup!: remove unused

bf63e1a

fixup!: cpp lint fix

b9eb514

panva requested a review from tniessen July 19, 2021 12:46

This comment has been minimized.

Sign in to view

panva removed the needs-ci PRs that need a full CI run. label Jul 19, 2021

This comment has been minimized.

Sign in to view

panva approved these changes Jul 20, 2021

View reviewed changes

panva added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 20, 2021

jasnell approved these changes Jul 26, 2021

View reviewed changes

panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 26, 2021

jasnell closed this Jul 26, 2021

jasnell pushed a commit that referenced this pull request Jul 26, 2021

crypto: fix generateKeyPair with encoding 'jwk'

257312a

Fixes: #39205 PR-URL: #39319 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

panva removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 26, 2021

himself65 deleted the 20210709-crypto-fix branch July 30, 2021 12:50

targos pushed a commit that referenced this pull request Aug 2, 2021

crypto: fix generateKeyPair with encoding 'jwk'

cbf6a01

Fixes: #39205 PR-URL: #39319 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

targos mentioned this pull request Aug 2, 2021

v16.6.1 release proposal #39631

Merged

codebytere added a commit to electron/electron that referenced this pull request Aug 9, 2021

crypto: fix generateKeyPair with encoding 'jwk'

542f156

nodejs/node#39319

codebytere added a commit to electron/electron that referenced this pull request Aug 9, 2021

crypto: fix generateKeyPair with encoding 'jwk'

7dc05d3

nodejs/node#39319

codebytere added a commit to electron/electron that referenced this pull request Aug 18, 2021

crypto: fix generateKeyPair with encoding 'jwk'

9001f6b

nodejs/node#39319

codebytere added a commit to electron/electron that referenced this pull request Aug 18, 2021

crypto: fix generateKeyPair with encoding 'jwk'

ee1cc59

nodejs/node#39319

panva mentioned this pull request Aug 21, 2021

crypto: fix JWK RSA-PSS SubtleCrypto.exportKey #39828

Closed

targos added the dont-land-on-v14.x label Sep 4, 2021

crypto: fix `generateKeyPair` with encoding 'jwk' by himself65 · Pull Request #39319 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: fix generateKeyPair with encoding 'jwk' - #39319

crypto: fix generateKeyPair with encoding 'jwk'#39319himself65 wants to merge 17 commits intonodejs:masternodejs/node:masterfrom himself65:20210709-crypto-fixhimself65/node:20210709-crypto-fixCopy head branch name to clipboard

Conversation

himself65 commented Jul 9, 2021

Uh oh!

Uh oh!

Uh oh!

Trott commented Jul 10, 2021

Uh oh!

panva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

himself65 commented Jul 12, 2021

Uh oh!

himself65 commented Jul 19, 2021

Uh oh!

himself65 commented Jul 19, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

himself65 commented Jul 20, 2021

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jul 20, 2021

Uh oh!

himself65 commented Jul 23, 2021

Uh oh!

panva commented Jul 24, 2021

Uh oh!

jasnell commented Jul 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crypto: fix generateKeyPair with encoding 'jwk'#39319
himself65 wants to merge 17 commits into
nodejs:masterfrom
himself65:20210709-crypto-fix

panva left a comment •
edited
Loading