I think === is better than ==.

I guess it does not make a difference when using typeof, but I can surely change it.

yes, doesn't make a difference, but node style is currently to always use ===, unless using == is necessary (mostly for comparisons against unsigned and null in a single operation)

if typeof encoding == object, && there are also options, that should be an error

what about when privateKey is passed in options? It would not make sense that .passPhrase is supported when the options object is first argument, but not supported when it is second or third argument

Is this still relevant when changing the options object to be the first argument, such that the first argument is either the privateKey itself or an options object with properties key and optionally passphrase, padding and/or saltLength?

no, it would not be, if there is only one options object, there can be no inconsistency between the options allowed in the various options objects

same here

You might want to add this to the PER_ISOLATE_STRING_PROPERTIES table in env.h and then simply use options->Get(env->padding_string()) instead

If maybePadding is empty you can just return 0

Considering that I return 0 after throwing an exception (so 0 <=> error), I should probably return 1 here as it should be optional to specify the padding. Apart from that, I agree.

Considering that I return 0 after throwing an exception (so 0 <=> error), I should probably return 1 here as it should be optional to specify the padding.

In the case where maybePadding is empty you can safely assume that an exception is pending (e.g. if the getter for "padding" threw an error – which is obviously mostly hypothetical, but since it’s an object passed in from userland, it’s possible). So, returning 0 and returning back to JS as quickly as possible should be the right thing to do.

If the property is missing, you’ll get an Undefined value, not an empty MaybeLocal.

If the property is missing, you’ll get an Undefined value, not an empty MaybeLocal.

Oh, that makes it clear. I did not know that, sorry!

style nit: const char* (I know we’re not being consistent in the code base 😄)

Actually… crypto.constants.RSA_PKCS1_PSS_PADDING and crypto.constants.RSA_PKCS1_PADDING already exist and are accessible from userland. Maybe we could use these constants instead of a string?

Nice, must have missed those when I skimmed through the constants. I just noticed that the asymmetric encrypt/decrypt API allows to specify padding as part of the first argument (where key, passphrase and padding are properties of the object). Should I remove the additional options object and add the properties to the first argument for sign() / verify() as well in order to keep it consistent?

@sam-github @addaleax @bnoordhuis Thoughts on this? I have no problem keeping it as it is, with options as an additional parameter, but you might prefer to keep it consistent with publicEncrypt etc.

Yes, it should be like publicEncrypt, etc, with .padding part of the first argument.

Lint error.

src/node_crypto.cc:3968:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
src/node_crypto.cc:3968:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

style nit: } else { on one line

ditto.

src/node_crypto.cc:3971:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
src/node_crypto.cc:3971:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

ditto, if (maybeSaltlen.IsEmpty()) return 0;

EVP_MD_CTX* mdctx, etc., for consistency with rest of file

s is an unnecessarily cryptic name for sig_len

this isn't an index, node_crypto.cc usually calls this usagerv, can you rename it?

I’d personally prefer just m instead of &(m[0]) here

style nit: static_cast<size_t> instead of (size_t) (assuming that works here)

This causes s cross initialization error during build. Move its definition before goto.

../src/node_crypto.cc: In function ‘int node::crypto::Node_SignFinal(EVP_MD_CTX*, unsigned char*, unsigned int*, EVP_PKEY*, int, int)’:
../src/node_crypto.cc:4109:4: error: jump to label ‘err’ [-fpermissive]
    err:
    ^
../src/node_crypto.cc:4086:10: note:   from here
     goto err;
          ^
../src/node_crypto.cc:4089:12: note:   crosses initialization of ‘size_t sltmp’
     size_t sltmp = (size_t)EVP_PKEY_size(pkey);
            ^

would this be a bug in our javascript layer, defensively being checked for in the binding layer, or is it something that happens occaisonally?

As far as I know, this can occur if a hash algorithm is used that does not support signing. When signing with RSA etc., this code will not even be reached (EVP_MD_FLAG_PKEY_METHOD_SIGNATURE means "Digest uses EVP_PKEY_METHOD for signing instead of MD specific signing").

Would be good to add a test for this, if its possible to reach from calling the js API. If not, defensive code is still good.

pre-existing, but can you move the *? it was on the opposite side from the other arguments

sig_len, therefor salt_len

Mh, sorry if I wasn’t clear, but my idea was that you could add #include "node_constants.h" at the top of this file and then use the constants here where you currently use the magic numbers directly

style nit: can you add {} if the body of an if spans more than a single line?

node_crypto.cc is a bit schismatic, but style is either to have all the args aligned four spaces from start of line, or to align
them with the column after the (