Could you use lowercase string here and drop the space before the :?

Sure, but note that the rest of the document seems to use the uppercase variant and has a space before the colon.

@tniessen Thanks for pointing that out – we’re moving to consistenly using lowercase in #11697, so it would be good if this PR was in line with that. I’ll comment that I’d prefer to have the space before the colon dropped there, too.

Now that an options object is introduced, it seems the current positional parameter, output_format, should be possible to express as an option. encoding would be the more common name for it, I think.

It might be nice to have these two values exposed as named constants on crypto.constants… what do you think?

Great, will do so! What is the best way to define a constant which is not #defined? I have only seen people using NODE_DEFINE_CONSTANT with existing defs. Should I just #define RSA_PKCS1_PSS_MAX_SALT_LENGTH -2 and then use NODE_DEFINE_CONSTANT?

Should I just #define RSA_PKCS1_PSS_MAX_SALT_LENGTH -2 and then use NODE_DEFINE_CONSTANT?

I don’t see any problem with that. :) It’s quite possible that somebody else here comes up with a better suggestion, but for the time being, I’d recommend just doing it the way you just described.

Latest OpenSSL (1.1.1-dev) changes its definitions as digest, max and auto(only in verification). https://github.com/openssl/openssl/blob/master/doc/man1/pkeyutl.pod#rsa-algorithm
I guess that auto can be implemented even in openssl-1.0.2. I think it is better to follow it.

named constants would be better, or perhaps strings ('digest', 'max', and 'auto'), or perhaps both (named constants with the values being strings, not magic negative numbers)

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.

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

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]

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);
            ^

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

Fwiw you don’t need the args.Length() part of the check, if args[3] is out of bounds it will just be undefined. I don’t mind keeping it if you think that helps with readability, though.

Forgot to comment … can you check the return value here and return from the function if there was an error?

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)

same 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);
            ^

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]

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]

Latest OpenSSL (1.1.1-dev) changes its definitions as digest, max and auto(only in verification). https://github.com/openssl/openssl/blob/master/doc/man1/pkeyutl.pod#rsa-algorithm
I guess that auto can be implemented even in openssl-1.0.2. I think it is better to follow it.

There are no descriptions for mgf1 and its hash. The default behavior is that the mgf1 hash is the same as the sign/verify hash. I think it needs to not to add an additional option parameter for mgf1 but its spec should be described in the doc.

There are test vectors for RSA-PSS in the link of https://www.emc.com/emc-plus/rsa-labs/standards-initiatives/pkcs-rsa-cryptography-standard.htm . We can make verify tests by using this test vectors.
If you cannot get the test vector file, it is in https://github.com/shigeki/ohtsu_rsa_pss_js/blob/master/tests/vectors/pss-vect.txt. Please refer related test vectors of RSA-PSS(example1 for RSA1024bits, and example10 for RSA2048bits) in JavaScript are in https://github.com/shigeki/ohtsu_rsa_pss_js/tree/master/tests/ .

Unfortunately, openssl cannot accept explicit salt. So we can make sign tests for RSA-PSS by writing signature data to files and verifying its signature by using openssl dgst command with -signopt rsa_padding_mode:pss.

as for sign, signature_format should be possible to pass as encoding in the options

unrelated, but

If a signature_format is specified, the signature is expected to be a
string; otherwise signature is expected to be a [Buffer][].

is a bizarre restriction, I wonder if it is even accurate? I will investigate.

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

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)

padding and saltLength should both be integer, right? (Or at least number. But integer seems to be what’s coming out of the discussion @ #11697)

This is weird, I thought I had already fixed the documentation regarding the use of integer constants. Seems like I messed that up when committing. Fixed in the latest commit :)

ditto

Can you use the constant name instead of -2 for specifying the default? Ideally, users won’t have to care what particular value the constant has

nit: lowercase padding (or generally whatever it corresponds to in the documentation)

It would be nice if you could #define these constants in a header and then include it in the source files that rely on the specific value. node_constants.h probably works for that, or maybe node_crypto.h.

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

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

replace RSASSA-PSS with "when padding is RSA_PKCS1_PSS_PADDING"

I like the way the first argument is described here much better than how it is described for sign. It would be nice to align them, though the inconsistency is pre-existing, and if you don't have the time that's OK.

"for when padding is RSA_PKCS1_PSS_PADDING" - because people may not know the relationship between RSA_PKCS1_PSS_PADDING and RSASSA-PSS

I think a little bit of docs are useful here, because the constants are asymetrical. DIGEST can be used with sign and verify, MAX_SIGN only with sign, and _AUTO only with verify. It surprised me when I saw the last two constants had the same value, and I had to go back to the docs and compare the sign and verify docs to realize that some of the PSS constants were method specific, and some weren't.

node is a bit irregular on this, but every case statement in node_crypto.cc is indented 2 spaces more than the switch, best to be consistent

sig_len, therefor salt_len

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 (

verify_result, therefor salt_len

salt_len

['RSA...SHA256'], and below

"one or more of the following" sounds a bit better to my ears.

Tangential aside: 'hash' is a somewhat unfortunate choice of words here, seeing how we describe hash functions a few lines up.

This needs to be clearer on the fact that you obtain the constants from crypto.constants. Likewise verifier.update().

Please change the return type to bool. Better yet, I'd do the validation in lib/crypto.js, not here.

But doesn't that imply passing the padding etc. from JavaScript to the binding as additional arguments instead of an options object? I considered that as well, but assuming that RSASSA-PSS is not the last addition to sign()/verify(), it decided it could become pretty clumsy to pass all options as positional parameters.

Positional arguments are easier and more efficient than wrangling objects in C++ land.

As well, it would get rid of the ThrowError(). Validation code that throws JS exceptions as a side effect tends to be pretty brittle. Callers need to be very careful not to re-enter the VM on return but that is easy to get wrong.

@bnoordhuis fwiw, I’m good with the current approach. It’s readable (imho) and microperformance probably isn’t too relevant when compared against the cost of the crypto operations itself.

@bnoordhuis I can fix this by adding more positional arguments if you think that is the reasonable way. I do not know enough about node internals to fully evaluate the effects. I guess I should move default values to JS as well, such that the native code does not need to handle undefined values.

I am still a little concerned considering that it might be necessary to add more options in the future and all of these are optional, meaning that we might end up passing useless arguments most of the time.

I guess I should move default values to JS as well, such that the native code does not need to handle undefined values.

Yes, that's a good idea.

it might be necessary to add more options in the future and all of these are optional, meaning that we might end up passing useless arguments most of the time.

Efficiency-wise a few extra parameters won't break the bank (they're just stack slots) and like Anna says, the cost of crypto itself probably dwarfs the overhead.

I guess I should move default values to JS as well, such that the native code does not need to handle undefined values.

Yes, that's a good idea.

But what if args[3]->IsInt32() etc. returns false/fails? Should I just skip these tests, throw an exception, return silently...? (Or just keep "additional" default values in the native code).

You can CHECK(args[3]->IsInt32()), it's a bug when it asserts.

Style: use snake_case (i.e., maybe_padding) for locals.

Substance: can you use options->Get(env->context(), env->padding_string()) here? You're currently using the overload that returns a Local<Value> which is then implicitly converted to MaybeLocal<Value>.

Checking IsNumber() then coercing to int32_t is debatable, not every number is an int32_t.

Can you use the overload that takes a v8::Context?

Please use pkctx == nullptr.

Please use mdctx->digest->sign == nullptr.

You can drop the extra parens and please line up the arguments.

You can use args[3].As<Object>() here, that's what we use most in core.

This could probably be shared with the same logic in Node_SignFinal() with some more effort.

I guess I could add a static function just to check whether it is RSA and set the two parameters if it is. Is that what you suggest?

Moving it into a separate function is what I had in mind, yes.

Long line. Can you run make test?

EDIT: Comment crossed with a follow-up commit, disregard.

Already fixed in the last commit, sorry. The problem is that I cannot perform linting on Windows and cannot run the tests on Linux, so I have to push from my Windows machine after running the tests before linting it on my Linux machine.