FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: add generatePublicKey to ECDH, fix corner cases by mbullington · Pull Request #1020 · nodejs/node · GitHub

/ node Public

crypto: add generatePublicKey to ECDH, fix corner cases - #1020

Closed
mbullington wants to merge 2 commits into
nodejs:v1.xfrom
mbullington:ecdh-fixes
ConversationCommits (2)ChecksFiles changed
Closed

crypto: add generatePublicKey to ECDH, fix corner cases#1020
mbullington wants to merge 2 commits into
nodejs:v1.xfrom
mbullington:ecdh-fixes

Conversation

mbullington commented Mar 1, 2015

Copy link
Copy Markdown

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.

crypto: add generatePublicKey to ECDH, fix corner cases
6c03548 
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.
tellnes added the crypto Issues and PRs related to the crypto subsystem. label Mar 1, 2015
brendanashworth reviewed Mar 2, 2015
View reviewed changes
Comment thread lib/crypto.js Outdated

brendanashworth Mar 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the function name here needs to be changed from generateKeys to generatePublicKey

brendanashworth added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 2, 2015
crypto: change function name
076917d 
Change the function name of generatePublicKey to be 
generatePublicKey instead of generateKeys.

brendanashworth commented Mar 6, 2015

Copy link
Copy Markdown
Contributor

cc'ing @iojs/crypto

bnoordhuis reviewed Mar 6, 2015
View reviewed changes
Comment thread src/node_crypto.cc Outdated

bnoordhuis Mar 6, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Style: space after 'if' (here and below.)

bnoordhuis commented Mar 6, 2015

Copy link
Copy Markdown
Member

LGTM sans nits.

indutny reviewed Mar 6, 2015
View reviewed changes
Comment thread src/node_crypto.cc Outdated

indutny Mar 6, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why?

indutny Mar 6, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I mean, perhaps it should be set from GeneratePublicKey too? Or be a flag field with two possible flags: GeneratedPrivate and GeneratedPublic?

mbullington Mar 6, 2015

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If I have something like the following code sample, getPrivateKey() will return an error if you haven't generated the keys, which is a pain for imported keys. I'd have to generate a keypair, then overwrite the keys (wasting a keypair just to toggle the bool).

var ecdh = crypto.createECDH(curve);
ecdh.setPrivateKey(privateKey);
ecdh.setPublicKey(publicKey);

ecdh.getPrivateKey(); // error

I could also import only the private key then use generatePublicKey to toggle the bool, but the getPrivateKey/getPublicKey functionality with the bool initially can be unexpected behavior.

Perhaps computeSecret could just check for the private key and throw an error based on that?

indutny Mar 6, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ok, if setPrivateKey will set both Private and Public flag - will that work?

brendanashworth commented Apr 4, 2015

Copy link
Copy Markdown
Contributor

Reiterating, @mbullington if you could make the requested changes, I could try and get this merged in soon!

Qard commented Jun 26, 2015

Copy link
Copy Markdown
Member

This PR is getting stale.

@mbullington Can you update the PR based on the feedback provided?

mbullington commented Jun 26, 2015

Copy link
Copy Markdown
Author

I'll update the PR based on master and the feedback provided. Thank you for being patient, I'm very sorry if having this PR open for this long caused any inconvenience.

Qard commented Jun 26, 2015

Copy link
Copy Markdown
Member

No worries. I just want to make sure you get your contributions in and that they aren't forgotten. :)

Qard commented Aug 8, 2015

Copy link
Copy Markdown
Member

@nodejs/crypto Anyone want to update and retarget this?

Fishrock123 commented Oct 19, 2015

Copy link
Copy Markdown
Contributor

@mbullington could you make a new PR against master? Thanks!

mruddy mentioned this pull request Oct 25, 2015
Closed

mruddy commented Oct 25, 2015

Copy link
Copy Markdown

Please see #3511. It's a little different, but solves many of the same things.

Trott mentioned this pull request Dec 3, 2015
Closed

Trott commented Dec 3, 2015

Copy link
Copy Markdown
Member

I rebased against master (only conflicts were in the markdown file), fixed up the nits (except for the ecdh->generated_ = true; removal that @indutny calls out) as well as some linting errors and re-animated this in #4124. Hope that's OK (and happy to close it if it's not for some reason).

bnoordhuis commented Dec 7, 2015

Copy link
Copy Markdown
Member

Closing, obsolete now that #3511 has landed.

bnoordhuis closed this Dec 7, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL