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: fix generateKeyPair with encoding 'jwk' #39319

Closed
wants to merge 17 commits into from

Conversation

himself65
Copy link
Member

Fixes: #39205

@himself65 himself65 added the crypto Issues and PRs related to the crypto subsystem. label Jul 9, 2021
@himself65 himself65 requested a review from jasnell July 9, 2021 04:09
@github-actions 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 himself65 changed the title crypto: fix enerateKeyPair with encoding 'jwk' crypto: fix generateKeyPair with encoding 'jwk' Jul 9, 2021
@himself65 himself65 force-pushed the 20210709-crypto-fix branch 2 times, most recently from 8050957 to 68df3f7 Compare July 9, 2021 04:52
@Trott
Copy link
Member

Trott commented Jul 10, 2021

@nodejs/crypto

@panva panva self-requested a review July 10, 2021 21:03
Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

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

@himself65

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

test/parallel/test-crypto-keygen.js Outdated Show resolved Hide resolved
panva
panva previously requested changes Jul 11, 2021
test/parallel/test-crypto-keygen.js Outdated Show resolved Hide resolved
@himself65
Copy link
Member Author

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.

@himself65 himself65 force-pushed the 20210709-crypto-fix branch 2 times, most recently from da40de3 to 790e4ff Compare July 14, 2021 02:51
@himself65 himself65 force-pushed the 20210709-crypto-fix branch from 790e4ff to 5d0b081 Compare July 19, 2021 03:01
@himself65
Copy link
Member Author

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

@himself65
Copy link
Member Author

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>();
}

@panva panva requested a review from tniessen July 19, 2021 12:46
@panva panva dismissed their stale review July 19, 2021 13:28

addressed

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

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

This comment has been minimized.

@himself65
Copy link
Member Author

/cc @nodejs/crypto

@nodejs-github-bot

This comment has been minimized.

@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2021
@nodejs-github-bot
Copy link
Collaborator

@himself65
Copy link
Member Author

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

@panva
Copy link
Member

panva commented Jul 24, 2021

cc @nodejs/crypto

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

jasnell commented Jul 26, 2021

Landed in 257312a

@jasnell jasnell closed this Jul 26, 2021
jasnell pushed a commit that referenced this pull request Jul 26, 2021
Fixes: #39205

PR-URL: #39319
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@panva panva removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 26, 2021
@himself65 himself65 deleted the 20210709-crypto-fix branch July 30, 2021 12:50
targos pushed a commit that referenced this pull request Aug 2, 2021
Fixes: #39205

PR-URL: #39319
Reviewed-By: Filip Skokan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Aug 2, 2021
codebytere added a commit to electron/electron that referenced this pull request Aug 9, 2021
codebytere added a commit to electron/electron that referenced this pull request Aug 9, 2021
codebytere added a commit to electron/electron that referenced this pull request Aug 18, 2021
codebytere added a commit to electron/electron that referenced this pull request Aug 18, 2021
codebytere added a commit to electron/electron that referenced this pull request Aug 20, 2021
* 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 <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
* 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 <[email protected]>
Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
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++. crypto Issues and PRs related to the crypto subsystem. 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.

Using JWK as encoding format for public key in crypto.generateKeyPairSync() throws an error
7 participants