Skip to content

ecdh.setPublicKey is actually useful and should be undeprecated #18977

Closed
Closed
ecdh.setPublicKey is actually useful and should be undeprecated#18977
Labels
c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.feature requestIssues requesting new Node.js features.good first issueIssues that are suitable for first-time contributors.

Description

@skerit
skerit
opened on Feb 24, 2018

So ecdh.setPublicKey has been deprecated since v5.2.0, but there is a certain use case where it is very useful: when you need to uncompress a compressed public key.

Here's how I would uncompress such a public key:

const crypto = require('crypto');

let ecdh = crypto.createECDH('secp256k1');
ecdh.generateKeys();

let private_key_compressed = ecdh.getPrivateKey(null, 'compressed');
let public_key_compressed = ecdh.getPublicKey(null, 'compressed');

// Public key is a buffer of 33 bytes
console.log('Compressed public key:', public_key_compressed.length, public_key_compressed);

// Create a new ecdh object
ecdh = crypto.createECDH('secp256k1')

// Use the compressed public key to set the public key
ecdh.setPublicKey(public_key_compressed);

// Get the uncompressed get
let public_key_uncompressed = ecdh.getPublicKey();

// The returned key is a buffer of 66 bytes long
console.log('Uncomrpessed public key:', public_key_uncompressed.length, public_key_uncompressed);

If ecdh.setPublicKey where to disappear I would require another library just for this simple task.

Activity

  1. mscdex
    added
    cryptoIssues and PRs related to the crypto subsystem.
    on Feb 24, 2018

  2. MylesBorins commented on Feb 25, 2018

    @MylesBorins
    MylesBorins
    on Feb 25, 2018
    Contributor

    /cc @nodejs/crypto

  3. bnoordhuis commented on Feb 26, 2018

    @bnoordhuis
    bnoordhuis
    on Feb 26, 2018
    Member

    Technically correct but it's probably better to add an explicit conversion method instead of undeprecating ECDH#setPublicKey().

    @skerit When does this come up? I'd expect most applications to be agnostic to the point format.

  4. skerit commented on Feb 26, 2018

    @skerit
    skerit
    on Feb 26, 2018
    Author

    Ah well, this is related to my comment on issue #18147 "Signing with a ecdh type private key":

    Since crypto's Sign#sign method needs a valid ASN.1 wrapper, but currently there is no built-in way of generating this, I'm using the jsrsasign module to do the signing, and that requires an uncompressed key.

    But yeah, an new method for converting the compressed key would be better :)

  5. bnoordhuis
    added
    c++Issues and PRs that require attention from people who are familiar with C++.
    good first issueIssues that are suitable for first-time contributors.
    feature requestIssues requesting new Node.js features.
    mentor-available
    on Feb 26, 2018

  6. bnoordhuis commented on Feb 26, 2018

    @bnoordhuis
    bnoordhuis
    on Feb 26, 2018
    Member

    Okay, I've added labels. Pull request welcome and happy to answer questions.

  7. wuweiweiwu commented on Feb 27, 2018

    @wuweiweiwu
    wuweiweiwu
    on Feb 27, 2018
    Contributor

    @bnoordhuis Can I pick this up? I think it'll be a good first issue :)

  8. bnoordhuis commented on Feb 27, 2018

    @bnoordhuis
    bnoordhuis
    on Feb 27, 2018
    Member

    @wuweiweiwu Sure thing.

  9. wuweiweiwu commented on Feb 28, 2018

    @wuweiweiwu
    wuweiweiwu
    on Feb 28, 2018
    Contributor

    @bnoordhuis I was thinking of adding another method uncompressKey in the ECDH class in https://github.com/nodejs/node/blob/master/src/node_crypto.cc

    Is that a good place to start?

    Thank you

  10. bnoordhuis commented on Mar 1, 2018

    @bnoordhuis
    bnoordhuis
    on Mar 1, 2018
    Member

    @wuweiweiwu I'd make it a static method (i.e. on ECDH, not ECDH.prototype) and you should name it something like convertKey() because the conversion goes both ways.

    (Three ways actually because node can also convert to a hybrid format.)

  11. wuweiweiwu commented on Mar 1, 2018

    @wuweiweiwu
    wuweiweiwu
    on Mar 1, 2018
    Contributor

    @bnoordhuis sounds good! I will work on the compress and uncompress. Where can I find more information on the hybrid format?

    And is it ok if I put the tests in test/parallel?

  12. bnoordhuis commented on Mar 1, 2018

    @bnoordhuis
    bnoordhuis
    on Mar 1, 2018
    Member

    It's the 'hybrid' option to ECDH.prototype.getPublicKey.

    And is it ok if I put the tests in test/parallel?

    Yep.

  13. wuweiweiwu commented on Mar 1, 2018

    @wuweiweiwu
    wuweiweiwu
    on Mar 1, 2018
    Contributor

    Just a question about converting buffer to point. Currently I have ConvertKey in node_crypto.cc as a static method. I was planning on using ECDH::BufferToPoint to convert the input key to EC_POINT However that function is a protected function in ECDH class.

    Should I declare ConvertKey as protected function of ECDH as well and then just bind it as a static method in diffiehellman.js or should I figure out a way without using the ECDH member functions since ConvertKey is going to be a static method?

    Perhaps using EC_GROUP_new_by_curve_name(nid) and have the user input the ECDH curve name associated with the key?

    Thank you!

  14. bnoordhuis commented on Mar 2, 2018

    @bnoordhuis
    bnoordhuis
    on Mar 2, 2018
    Member

    I'd probably move the shared logic into a static ECDH member function. It doesn't matter if it's not a member, as long as there isn't code duplication.

  15. wuweiweiwu commented on Mar 2, 2018

    @wuweiweiwu
    wuweiweiwu
    on Mar 2, 2018
    Contributor

    Sounds good! On it

  16. wuweiweiwu
    added a commit that references this issue on Mar 10, 2018

    crypto: ECDH function to convert public keys

    41a17db
  17. tniessen
    closed this as completedin f2e0288on Mar 23, 2018
  18. targos
    added a commit that references this issue on Apr 12, 2018

    crypto: add ECDH.convertKey to convert public keys

    f06b41e
  19. sjarmak
    added a commit that references this issue on Jul 27, 2026

    crypto: add ECDH.convertKey to convert public keys

    f2e0288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.feature requestIssues requesting new Node.js features.good first issueIssues that are suitable for first-time contributors.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions