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: add generatePublicKey to ECDH, fix corner cases #4124

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 3, 2015

This is #1020 rebased against master and with the nits fixed up.

From original PR:

ECDH.generatePublicKey can get the public key using the curve
and private key. This allows usage of the crypto module where
a developer imports a private key, and then generates a public
key without needing it stored.

Removed the generated_ boolean from the ECDH class, and stopped
checking for it with getPrivateKey() and getPublicKey(). This
allows you to import public and private keys without having to
generate first, which would just rewrite the generated keys
regardless.

An error message was changed to accurately reflect what the error
was.

mbullington and others added 3 commits December 2, 2015 13:26
ECDH.generatePublicKey can get the public key using the curve
and private key. This allows usage of the crypto module where
a developer imports a private key, and then generates a public
key without needing it stored.

Removed the generated_ boolean from the ECDH class, and stopped
checking for it with getPrivateKey() and getPublicKey(). This
allows you to import public and private keys without having to
generate first, which would just rewrite the generated keys
regardless.

An error message was changed to accurately reflect what the error
was.
Change the function name of generatePublicKey to be 
generatePublicKey instead of generateKeys.
@Trott Trott added crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 3, 2015
@mscdex
Copy link
Contributor

mscdex commented Dec 3, 2015

Would it be possible to implicitly cache the generated keys to avoid repeated calculations?

@jasnell
Copy link
Member

jasnell commented Dec 3, 2015

@nodejs/crypto

@Trott
Copy link
Member Author

Trott commented Dec 4, 2015

@mscdex I want to make sure I understand what you're asking. You're talking about basically the same thing as the conversation starting at https://github.com/nodejs/node/pull/1020/files#r25941472 was about, right? It's about how to make sure we don't needlessly go through the work of generating the public key from the private key more than once?

@mscdex
Copy link
Contributor

mscdex commented Dec 4, 2015

@Trott Pretty much.

@mruddy
Copy link

mruddy commented Dec 4, 2015

You guys should check what we've got ready for #3511 to make sure you're not repeating effort.

@Trott
Copy link
Member Author

Trott commented Dec 5, 2015

@mruddy It does seem that #3511 handles more-or-less all the same issues as this one. Let's see if that one can get landed and then maybe this and #1020 can be closed.

@mruddy
Copy link

mruddy commented Dec 5, 2015

@Trott Yep, agreed, exactly what I was thinking.

@bnoordhuis
Copy link
Member

FWIW, I just landed #3511. Should this be closed?

@mruddy
Copy link

mruddy commented Dec 7, 2015

@Trott @bnoordhuis Yep, with the landing of #3511, I believe that this, #4124, and #1020 can now be closed.

@bnoordhuis
Copy link
Member

Okay, closing. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants