Skip to content

crypto: fix key object wrapping in sync keygen#25326

Closed
tniessen wants to merge 2 commits intonodejs:masterfrom
tniessen:fix-25322
Closed

crypto: fix key object wrapping in sync keygen#25326
tniessen wants to merge 2 commits intonodejs:masterfrom
tniessen:fix-25322

Conversation

@tniessen
Copy link
Member

@tniessen tniessen commented Jan 3, 2019

generateKeyPairSync incorrectly returns the native key object handle instead of the higher-level JS key object. This change fixes that and aligns the documentation with generateKeyPair.

Fixes: #25322

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 3, 2019

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2132/pipeline

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Jan 3, 2019
cjihrig
cjihrig approved these changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. Would there be any benefit to calling getSymmetricKeySize() in the tests since that triggered the crash in #25322?

sam-github
sam-github approved these changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

lgtm, modulo a nit about the security advice.

@tniessen
Copy link
Member Author

tniessen commented Jan 4, 2019

Would there be any benefit to calling getSymmetricKeySize() in the tests since that triggered the crash in #25322?

@cjihrig Good question! The function getSymmetricKeySize() is a native function and should not have been exposed to users, instead, the properties of the key object (such as type, asymmetricKeyType, symmetricKeySize) are exposed. I believe that testing these properties is enough, the only valid assertion for getSymmetricKeySize would be its nonexistence.

@tniessen
Copy link
Member Author

tniessen commented Jan 4, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/19928/
Resumed in https://ci.nodejs.org/job/node-test-pull-request/19930/ ✔️

@danbev
Copy link
Contributor

danbev commented Jan 7, 2019

Landed in 7afdfae.

@danbev danbev closed this Jan 7, 2019
danbev pushed a commit that referenced this pull request Jan 7, 2019
@tniessen @danbev
crypto: fix key object wrapping in sync keygen
7afdfae 
PR-URL: #25326
Fixes: #25322
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 9, 2019
@tniessen @addaleax
crypto: fix key object wrapping in sync keygen
32e45b2 
PR-URL: #25326
Fixes: #25322
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@tniessen @refack
crypto: fix key object wrapping in sync keygen
c8c9290 
PR-URL: nodejs#25326
Fixes: nodejs#25322
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@tniessen @BridgeAR
crypto: fix key object wrapping in sync keygen
79b089f 
PR-URL: nodejs#25326
Fixes: nodejs#25322
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+1 more reviewer

@sam-github sam-github sam-github approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

generateKeyPairSync doesn't return KeyObject

7 participants

@tniessen @nodejs-github-bot @danbev @sam-github @bnoordhuis @jasnell @cjihrig