ecdh.setPublicKey is actually useful and should be undeprecated

[skerit]

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.

[MylesBorins]

/cc @nodejs/crypto

[bnoordhuis]

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.

[skerit]

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 :)

[bnoordhuis]

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

[wuweiweiwu]

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

[bnoordhuis]

@wuweiweiwu Sure thing.

[wuweiweiwu]

@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

[bnoordhuis]

@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.)

[wuweiweiwu]

@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?

[bnoordhuis]

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

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

Yep.

[wuweiweiwu]

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!

[bnoordhuis]

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.

[wuweiweiwu]

Sounds good! On it