crypto: KeyObject.asymmetricKeySize API by paroga · Pull Request #26387 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

paroga

Expose the size of asymetric keys of crypto key object from the
crypto module added in v11.6.0 (#24234)

Refs: #24234

Checklist

@nodejs-github-bot nodejs-github-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.

labels

Mar 2, 2019

mscdex

bnoordhuis

@paroga

Expose the size of asymetric keys of crypto key object from the crypto module added in v11.6.0 (nodejs#24234)

bnoordhuis

Choose a reason for hiding this comment

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

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/21140/

Maybe a minor suggestion vis-a-vis the commit log summary: crypto: add KeyObject.asymmetricKeySize?

The link to the issue should be Fixes: https://github.com/nodejs/node/pull/24234 on its own line.

edit: make that Refs: https://github.com/nodejs/node/pull/24234

jasnell

BridgeAR

@BridgeAR BridgeAR added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

Mar 6, 2019

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Mar 6, 2019

@paroga @BridgeAR

Expose the size of asymetric keys of crypto key object from the crypto module added in v11.6.0.

PR-URL: nodejs#26387 Refs: nodejs#24234 Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

@BridgeAR

Landed in 4895927 🎉

I updated the commit message while landing as suggested.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request

Mar 12, 2019

@paroga @BridgeAR

Expose the size of asymetric keys of crypto key object from the crypto module added in v11.6.0.

PR-URL: nodejs#26387 Refs: nodejs#24234 Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

@panva

👋 Maybe i'm missing something but EVP_PKEY_size returns the maximum signature size in bytes, not the actual key size.

This is apparent with Ed25519 and Ed448 and possibly EC keys as well.

pem =-----BEGIN PUBLIC KEY-----\nMCowBQYDK2VwAyEAEXRYV3v5ucrHVR3mKqyPXxXqU34lASwc7Y7MoOvaqcs=\n-----END PUBLIC KEY----- k=crypto.createPublicKey(pem) // PublicKeyObject { [Symbol(kKeyType)]: 'public' } k.asymmetricKeyType // 'ed25519' k.asymmetricKeySize // 114

114 bytes is the length of a signature, key is in fact 57 bytes

@mscdex

@panva That's probably worth posting a new issue for.

@panva

Will do, wanted to check first if i missed something

@panva panva mentioned this pull request

Mar 13, 2019

@panva

@paroga

i only checked it with RSA, since there are only RSA tests for the KeyObject in the repository. Maybe we should just rename the property and/or change the documentation, to match the returned value

@tniessen tniessen added the semver-minor

PRs that contain new features and should be released in the next minor version.

label

Mar 13, 2019

@tniessen

I'm assuming this should have been marked as semver-minor.

BridgeAR added a commit that referenced this pull request

Mar 13, 2019

@BridgeAR

Notable Changes

@BridgeAR

I'll back out this PR from v11.12.0 due to the concerns by @tniessen. If this should indeed land in a PR, please update the labels accordingly.

This was referenced

Apr 23, 2019

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++.

lib / src

Issues and PRs related to general changes in the lib or src directory.

semver-minor

PRs that contain new features and should be released in the next minor version.