Wrong and misleading error message from `crypto.sign(..., key)`

[benbucksch]

• Version: 13.2.0
• Platform: Linux on Docker
• Subsystem: crypto

Documentation

https://nodejs.org/api/crypto.html#crypto_crypto_sign_algorithm_data_key says:

crypto.sign(algorithm, data, key)
Added in: v12.0.0
...
* key <Object> | <string> | <Buffer> | <KeyObject>
...
If key is not a KeyObject, this function behaves as if key had been passed to
crypto.createPrivateKey().

and

crypto.createPrivateKey(key)
Added in: v11.6.0
* key <Object> | <string> | <Buffer>
  * key: <string> | <Buffer> The key material, either in PEM or DER format.
  * passphrase: <string> | <Buffer> The passphrase to use for decryption.

Creates and returns a new key object containing a private key.
If key is a string or Buffer, format is assumed to be 'pem';
otherwise, key must be an object with the properties described above.

This is already very unclear. There's the key parameter to crypto.sign(), which can have 4 different types. (And the docs are outdated, the implementation accepts a 5. type DataView, which is not even documented.)

We have a "KeyObject" and a "key object", which are not the same type. (!) (You have to read the previous sentence a few times, to appreciate this from an API user perspective.)

Then, that key parameter can be an object with a key property. Already at this point, you completely lost me. Why does everything have the same name?

Further, in crypto.createPrivateKey(key), it continues the same unclarity. We again have "key" twice. Consequently, the sentence "key must be an object with" is unreadable, because we cannot know whether "key" here references the parameter = the object, or the property. (In fact, if you look at the code, the key property can be a KeyObject again.)

This is just for context for the bug, not the bug itself reported here.

So, we have KeyObject type, key object, key parameter, and key property. All different.

I wish I was kidding, but I'm not. So, to get the implementation right, we need to distinguish between the key property and key parameter.

What steps will reproduce the bug?

let keyInfo = {
  padding: 6,
  // key: missing
  passphrase: "irrelevant"
}
let signed = crypto.sign("RSA-SHA256", Buffer.from("foobar", "utf8"), keyInfo).toString("hex");
console.log(signed);

How often does it reproduce?

Always

What is the expected behavior?

Exception:

TypeError [ERR_INVALID_ARG_TYPE]: The object is missing the "key" property.
The "key" property of the "key" argument must be a String, Buffer, TypedArray,
DataView, or KeyObject. Received undefined.

What do you see instead?

Exception:

TypeError [ERR_INVALID_ARG_TYPE]: The "key" argument must be of type string or
an instance of Buffer, TypedArray, DataView, or KeyObject.
Received an instance of Object

Note that this is talking specifically about a "key" argument, not "key property". The argument is an object with a key property. The key property was missing, the key argument was indeed of type object. And that is correct.

The error message says that it's wrong to pass an object as key argument. However, that error message is wrong. It's completely correct to pass in an object as key parameter. But the object must have a key property. So, the error message is definitely wrong and is making false statements that match neither the documentation nor the implementaton.

This is particularly bad given that the API is already that muddy and confusing. Giving false error messages that blame the wrong thing then make the API impossible to work with.

Fix

Caused by keys.js line 270:

      throw new ERR_INVALID_ARG_TYPE(
        'key',
        ['string', 'Buffer', 'TypedArray', 'DataView',
         ...(ctx !== kCreatePrivate ? ['KeyObject'] : [])],
        key);
    }

which looks innocent and fine, but creates the wrong textual error message.

[benbucksch]

This seems like a copy&paste bug. Somebody copied line 280, where this is correct, to line 270, where it's wrong.

Fix:

      throw new ERR_INVALID_ARG_TYPE(
        'key.key property',
        ['string', 'Buffer', 'TypedArray', 'DataView',
         ...(ctx !== kCreatePrivate ? ['KeyObject'] : [])],
        key.key);