New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto: add sign/verify support for RSASSA-PSS #11705

Closed
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@tniessen
Member

tniessen commented Mar 6, 2017

Adds support for the PSS padding scheme. Until now, the sign/verify functions used the old EVP_Sign*/EVP_Verify* OpenSSL API, making it impossible to change the padding scheme. Fixed by first computing the message digest and then signing/verifying with a custom EVP_PKEY_CTX, allowing us to specify options such as the padding scheme and the PSS salt length.

Fixes: #1127

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto, src, doc, test

@addaleax

Thanks for picking this up!

/cc @nodejs/crypto

Show outdated Hide outdated doc/api/crypto.md
The optional `options` argument is an object which specifies additional
cryptographic parameters. Currently, the following options are supported:
* `padding` : {String} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

This comment has been minimized.

@tniessen

tniessen Mar 6, 2017

Member

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

@tniessen

tniessen Mar 6, 2017

Member

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

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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.

@sam-github

sam-github Mar 6, 2017

Member

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.

Show outdated Hide outdated doc/api/crypto.md
(default) or `'pss'` for RSASSA-PSS
* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. If this is set to `-2`
(default), the salt length will be set to the maximum permissible value.

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

This comment has been minimized.

@tniessen

tniessen Mar 6, 2017

Member

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?

@tniessen

tniessen Mar 6, 2017

Member

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?

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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.

@addaleax

addaleax Mar 6, 2017

Member

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.

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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)

@sam-github

sam-github Mar 6, 2017

Member

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)

Show outdated Hide outdated src/node_crypto.cc
int SignBase::GetRSAOptions(Environment *env, v8::Local<v8::Object> options,
int *padding, int *saltlen) {
Local<Value> key = String::NewFromUtf8(env->isolate(), "padding");
MaybeLocal<Value> maybePadding = options->Get(key);

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
int *padding, int *saltlen) {
Local<Value> key = String::NewFromUtf8(env->isolate(), "padding");
MaybeLocal<Value> maybePadding = options->Get(key);
if (!maybePadding.IsEmpty()) {

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

If maybePadding is empty you can just return 0

@addaleax

addaleax Mar 6, 2017

Member

If maybePadding is empty you can just return 0

This comment has been minimized.

@tniessen

tniessen Mar 6, 2017

Member

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.

@tniessen

tniessen Mar 6, 2017

Member

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.

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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.

@addaleax

addaleax Mar 6, 2017

Member

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.

This comment has been minimized.

@tniessen

tniessen Mar 6, 2017

Member

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!

@tniessen

tniessen Mar 6, 2017

Member

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!

Show outdated Hide outdated src/node_crypto.cc
Local<Value> paddingValue = maybePadding.ToLocalChecked();
if (paddingValue->IsString()) {
v8::String::Utf8Value paddingUtf8(paddingValue);
const char *paddingName = *paddingUtf8;

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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?

@addaleax

addaleax Mar 6, 2017

Member

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?

This comment has been minimized.

@tniessen

tniessen Mar 6, 2017

Member

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?

@tniessen

tniessen Mar 6, 2017

Member

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?

This comment has been minimized.

@tniessen

tniessen Mar 7, 2017

Member

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

@tniessen

tniessen Mar 7, 2017

Member

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

This comment has been minimized.

@sam-github

sam-github Mar 7, 2017

Member

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

@sam-github

sam-github Mar 7, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
if (*padding == RSA_PKCS1_PSS_PADDING) {
key = String::NewFromUtf8(env->isolate(), "saltLength");
MaybeLocal<Value> maybeSaltlen = options->Get(key);
if (!maybeSaltlen.IsEmpty()) {

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
else if (strcmp(paddingName, "pss") == 0) {
*padding = RSA_PKCS1_PSS_PADDING;
}
else {

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

style nit: } else { on one line

@addaleax

addaleax Mar 6, 2017

Member

style nit: } else { on one line

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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]
@shigeki

shigeki Mar 6, 2017

Contributor

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]
Show outdated Hide outdated src/node_crypto.cc
goto err;
if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = (size_t)EVP_PKEY_size(pkey);

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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

shigeki Mar 6, 2017

Contributor

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);
            ^
Show outdated Hide outdated src/node_crypto.cc
goto err;
if (padding == RSA_PKCS1_PSS_PADDING)
if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkctx, saltlen) <= 0)
goto err;

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
@@ -4357,8 +4477,15 @@ void Verify::VerifyFinal(const FunctionCallbackInfo<Value>& args) {
hbuf = Buffer::Data(args[1]);
}
int padding = RSA_PKCS1_PADDING;
int saltlen = -2;
if (args.Length() >= 4 && args[3]->IsObject()) {

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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.

@addaleax

addaleax Mar 6, 2017

Member

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.

Show outdated Hide outdated src/node_crypto.cc
int padding = RSA_PKCS1_PADDING;
int saltlen = -2;
if (args.Length() >= 4 && args[3]->IsObject()) {
verify->GetRSAOptions(env, args[3]->ToObject(), &padding, &saltlen);

This comment has been minimized.

@addaleax

addaleax Mar 6, 2017

Member

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

@addaleax

addaleax Mar 6, 2017

Member

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

Show outdated Hide outdated lib/crypto.js
var key = options.key || options;
var passphrase = options.passphrase || null;
var ret = this._handle.sign(toBuf(key), null, passphrase);
if (typeof encoding == 'object' && !options) {

This comment has been minimized.

@hiroppy

hiroppy Mar 6, 2017

Member

I think === is better than ==.

@hiroppy

hiroppy Mar 6, 2017

Member

I think === is better than ==.

This comment has been minimized.

@tniessen

tniessen Mar 6, 2017

Member

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

@tniessen

tniessen Mar 6, 2017

Member

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

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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)

@sam-github

sam-github Mar 6, 2017

Member

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)

Show outdated Hide outdated lib/crypto.js
Verify.prototype.verify = function verify(object, signature, sigEncoding) {
Verify.prototype.verify = function verify(object, signature, sigEncoding,
options) {
if (typeof sigEncoding == 'object' && !options) {

This comment has been minimized.

@hiroppy

hiroppy Mar 6, 2017

Member

same here

@hiroppy

hiroppy Mar 6, 2017

Member

same here

Show outdated Hide outdated src/node_crypto.cc
goto err;
if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = (size_t)EVP_PKEY_size(pkey);

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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

shigeki Mar 6, 2017

Contributor

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);
            ^
Show outdated Hide outdated src/node_crypto.cc
if (strcmp(paddingName, "pkcs1") == 0) {
*padding = RSA_PKCS1_PADDING;
}
else if (strcmp(paddingName, "pss") == 0) {

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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]
@shigeki

shigeki Mar 6, 2017

Contributor

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]
Show outdated Hide outdated src/node_crypto.cc
else if (strcmp(paddingName, "pss") == 0) {
*padding = RSA_PKCS1_PSS_PADDING;
}
else {

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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]
@shigeki

shigeki Mar 6, 2017

Contributor

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]
Show outdated Hide outdated doc/api/crypto.md
(default) or `'pss'` for RSASSA-PSS
* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. If this is set to `-2`
(default), the salt length will be set to the maximum permissible value.

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

Show outdated Hide outdated doc/api/crypto.md
* `saltLength` : {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. If this is set to `-2`
(default), the salt length will be set to the maximum permissible value.

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

Show outdated Hide outdated test/parallel/test-crypto-sign-verify.js
});
});
}

This comment has been minimized.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

@shigeki

shigeki Mar 6, 2017

Contributor

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.

Show outdated Hide outdated doc/api/crypto.md
@@ -1039,6 +1048,16 @@ the `signature_format` which can be `'latin1'`, `'hex'` or `'base64'`.
If a `signature_format` is specified, the `signature` is expected to be a
string; otherwise `signature` is expected to be a [`Buffer`][].
The optional `options` argument is an object which specifies additional

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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.

@sam-github

sam-github Mar 6, 2017

Member

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.

@sam-github

needs more tests for the various argument pemutations (with options, without options, options in the various argument positions)

Show outdated Hide outdated lib/crypto.js
var key = options.key || options;
var passphrase = options.passphrase || null;
var ret = this._handle.sign(toBuf(key), null, passphrase);
if (typeof encoding == 'object' && !options) {

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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

@sam-github

sam-github Mar 6, 2017

Member

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

Show outdated Hide outdated lib/crypto.js
}
var key = privateKey.key || privateKey;
var passphrase = privateKey.passphrase || null;

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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

@sam-github

sam-github Mar 6, 2017

Member

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

This comment has been minimized.

@tniessen

tniessen Mar 7, 2017

Member

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?

@tniessen

tniessen Mar 7, 2017

Member

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?

This comment has been minimized.

@sam-github

sam-github Mar 7, 2017

Member

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

@sam-github

sam-github Mar 7, 2017

Member

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

Show outdated Hide outdated lib/crypto.js
var key = options.key || options;
var passphrase = options.passphrase || null;
var ret = this._handle.sign(toBuf(key), null, passphrase);
if (typeof encoding == 'object' && !options) {

This comment has been minimized.

@sam-github

sam-github Mar 6, 2017

Member

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)

@sam-github

sam-github Mar 6, 2017

Member

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)

@addaleax

The code mostly looks good to me, I’ll just refrain from approving the PR because I don’t understand the subject matter well enough

Show outdated Hide outdated doc/api/crypto.md
* `passphrase`: {string} - passphrase for the private key
* `padding`: {String} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
`RSA_PKCS1_PSS_PADDING`
* `saltLength`: {number} - salt length for RSASSA-PSS. The special value

This comment has been minimized.

@addaleax

addaleax Mar 7, 2017

Member

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)

@addaleax

addaleax Mar 7, 2017

Member

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 comment has been minimized.

@tniessen

tniessen Mar 8, 2017

Member

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

@tniessen

tniessen Mar 8, 2017

Member

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

Show outdated Hide outdated doc/api/crypto.md
* `key`: {string} - PEM encoded private key (required)
* `padding`: {string} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
(default) or `'pss'` for RSASSA-PSS
* `saltLength`: {number} - salt length for RSASSA-PSS. If this is set to `-1`,

This comment has been minimized.

@addaleax

addaleax Mar 7, 2017

Member

ditto

@addaleax

addaleax Mar 7, 2017

Member

ditto

Show outdated Hide outdated doc/api/crypto.md
* `padding`: {string} - RSA padding, either `'pkcs1'` for RSASSA-PKCS1-v1_5
(default) or `'pss'` for RSASSA-PSS
* `saltLength`: {number} - salt length for RSASSA-PSS. If this is set to `-1`,
the salt length will be set to the digest size. A value of `-2` (default)

This comment has been minimized.

@addaleax

addaleax Mar 7, 2017

Member

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

@addaleax

addaleax Mar 7, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
*padding = paddingValue->Int32Value();
break;
default:
env->ThrowError("Padding must be RSA_PKCS1_PADDING or "

This comment has been minimized.

@addaleax

addaleax Mar 7, 2017

Member

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

@addaleax

addaleax Mar 7, 2017

Member

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

Show outdated Hide outdated src/node_constants.cc
#ifndef RSA_PSS_SALTLEN_AUTO
#define RSA_PSS_SALTLEN_AUTO -2
#endif

This comment has been minimized.

@addaleax

addaleax Mar 7, 2017

Member

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.

@addaleax

addaleax Mar 7, 2017

Member

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.

Show outdated Hide outdated src/node_crypto.cc
EVP_PKEY_CTX *pkctx = nullptr;
*s = 0;
if (!EVP_DigestFinal_ex(mdctx, &(m[0]), &m_len))

This comment has been minimized.

@addaleax

addaleax Mar 7, 2017

Member

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

@addaleax

addaleax Mar 7, 2017

Member

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

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Mar 8, 2017

Member

@addaleax I noticed, thought I had changed both occurrences to ::Cast before the first commit. You are right, ::Cast is what I wanted.

Member

tniessen commented Mar 8, 2017

@addaleax I noticed, thought I had changed both occurrences to ::Cast before the first commit. You are right, ::Cast is what I wanted.

Show outdated Hide outdated src/node_crypto.cc
@@ -4132,6 +4212,14 @@ void Sign::SignFinal(const FunctionCallbackInfo<Value>& args) {
size_t buf_len = Buffer::Length(args[0]);
char* buf = Buffer::Data(args[0]);
int padding = RSA_PKCS1_PADDING;
int saltlen = -2;

This comment has been minimized.

@addaleax

addaleax Mar 8, 2017

Member

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

@addaleax

addaleax Mar 8, 2017

Member

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

Show outdated Hide outdated doc/api/crypto.md
* `passphrase`: {string} - passphrase for the private key
* `padding`: {integer} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
`RSA_PKCS1_PSS_PADDING`
* `saltLength`: {integer} - salt length for RSASSA-PSS. The special value

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

replace RSASSA-PSS with "when padding is RSA_PKCS1_PSS_PADDING"

@sam-github

sam-github Mar 8, 2017

Member

replace RSASSA-PSS with "when padding is RSA_PKCS1_PSS_PADDING"

Show outdated Hide outdated doc/api/crypto.md
one an RSA public key, a DSA public key, or an X.509 certificate.
The `object` argument can be either a string containing a PEM encoded object,
which can be one an RSA public key, a DSA public key, or an X.509 certificate,
or an object with some of the following properties:

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

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.

@sam-github

sam-github Mar 8, 2017

Member

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.

Show outdated Hide outdated doc/api/crypto.md
* `key`: {string} - PEM encoded private key (required)
* `padding`: {integer} - RSA padding, either `RSA_PKCS1_PADDING` (default) or
`RSA_PKCS1_PSS_PADDING`
* `saltLength`: {integer} - salt length for RSASSA-PSS. The special value

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

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

@sam-github

sam-github Mar 8, 2017

Member

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

Show outdated Hide outdated doc/api/crypto.md
@@ -1966,6 +1981,18 @@ the `crypto`, `tls`, and `https` modules and are generally specific to OpenSSL.
<td></td>
</tr>
<tr>
<td><code>RSA_PSS_SALTLEN_DIGEST</code></td>
<td></td>

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

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.

@sam-github

sam-github Mar 8, 2017

Member

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.

Show outdated Hide outdated src/node_crypto.cc
Local<Value> paddingValue = maybePadding.ToLocalChecked();
if (paddingValue->IsNumber()) {
switch (paddingValue->Int32Value()) {
case RSA_PKCS1_PADDING:

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

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

@sam-github

sam-github Mar 8, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
unsigned int *sig_len) {
unsigned int *sig_len,
int padding,
int saltlen) {

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

sig_len, therefor salt_len

@sam-github

sam-github Mar 8, 2017

Member

sig_len, therefor salt_len

Show outdated Hide outdated src/node_crypto.cc
if (EVP_PKEY_CTX_set_signature_md(pkctx, mdctx_.digest) <= 0)
goto err;
r = EVP_PKEY_verify(pkctx,
reinterpret_cast<const unsigned char*>(sig),

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

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 (

@sam-github

sam-github Mar 8, 2017

Member

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 (

Show outdated Hide outdated src/node_crypto.cc
bool verify_result;
Error err = verify->VerifyFinal(kbuf, klen, hbuf, hlen, &verify_result);
Error err = verify->VerifyFinal(kbuf, klen, hbuf, hlen, padding, saltlen,

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

verify_result, therefor salt_len

@sam-github

sam-github Mar 8, 2017

Member

verify_result, therefor salt_len

Show outdated Hide outdated src/node_crypto.h
@@ -556,6 +556,8 @@ class SignBase : public BaseObject {
protected:
void CheckThrow(Error error);
int GetRSAOptions(Environment *env, v8::Local<v8::Object> options,
int *padding, int *saltlen);

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

salt_len

@sam-github

sam-github Mar 8, 2017

Member

salt_len

Show outdated Hide outdated test/parallel/test-crypto-sign-verify.js
@@ -71,6 +71,54 @@ const keyPem = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
assert.strictEqual(verified, true, 'sign and verify (stream)');
}
{
[ 'RSA-SHA1', 'RSA-SHA256' ].forEach((algo) => {

This comment has been minimized.

@sam-github

sam-github Mar 8, 2017

Member

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

@sam-github

sam-github Mar 8, 2017

Member

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

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Mar 8, 2017

Member

coming along well, I like the new method calling convention

Member

sam-github commented Mar 8, 2017

coming along well, I like the new method calling convention

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 9, 2017

Member

@tniessen Looks like this needs to be rebased against master, can you do that? I’d like to start CI for this but I don’t think that works well if there are merge conflicts

Member

addaleax commented Mar 9, 2017

@tniessen Looks like this needs to be rebased against master, can you do that? I’d like to start CI for this but I don’t think that works well if there are merge conflicts

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Mar 9, 2017

Member

@addaleax That should do it, hope I did not mess anything up.

Member

tniessen commented Mar 9, 2017

@addaleax That should do it, hope I did not mess anything up.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 9, 2017

Member

CI is green except for a weird OS X failure @ https://ci.nodejs.org/job/node-test-commit-osx/8287/nodes=osx1010/console @nodejs/build

Another attempt to be sure it’s unrelated: https://ci.nodejs.org/job/node-test-commit/8353/

Member

addaleax commented Mar 9, 2017

CI is green except for a weird OS X failure @ https://ci.nodejs.org/job/node-test-commit-osx/8287/nodes=osx1010/console @nodejs/build

Another attempt to be sure it’s unrelated: https://ci.nodejs.org/job/node-test-commit/8353/

Show outdated Hide outdated doc/api/crypto.md
@@ -935,10 +935,16 @@ Calculates the signature on all the data passed through using either
The `private_key` argument can be an object or a string. If `private_key` is a
string, it is treated as a raw key with no passphrase. If `private_key` is an
object, it is interpreted as a hash containing two properties:
object, it is interpreted as a hash containing some of these properties:

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

"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.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

"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.

Show outdated Hide outdated doc/api/crypto.md
* `saltLength`: {integer} - salt length for when padding is
`RSA_PKCS1_PSS_PADDING`. The special value `RSA_PSS_SALTLEN_DIGEST` sets the
salt length to the digest size, `RSA_PSS_SALTLEN_MAX_SIGN` (default) sets it
to the maximum permissible value.

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
@@ -3952,6 +3954,37 @@ void SignBase::CheckThrow(SignBase::Error error) {
}
}
int SignBase::GetRSAOptions(Environment* env, v8::Local<v8::Object> options,

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

This comment has been minimized.

@tniessen

tniessen Mar 11, 2017

Member

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.

@tniessen

tniessen Mar 11, 2017

Member

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.

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 12, 2017

Member

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

bnoordhuis Mar 12, 2017

Member

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.

This comment has been minimized.

@addaleax

addaleax Mar 14, 2017

Member

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

@addaleax

addaleax Mar 14, 2017

Member

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

This comment has been minimized.

@tniessen

tniessen Mar 14, 2017

Member

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

@tniessen

tniessen Mar 14, 2017

Member

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

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 14, 2017

Member

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.

@bnoordhuis

bnoordhuis Mar 14, 2017

Member

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.

This comment has been minimized.

@tniessen

tniessen Mar 14, 2017

Member

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

@tniessen

tniessen Mar 14, 2017

Member

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

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 14, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 14, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
@@ -3952,6 +3954,37 @@ void SignBase::CheckThrow(SignBase::Error error) {
}
}
int SignBase::GetRSAOptions(Environment* env, v8::Local<v8::Object> options,
int* padding, int* saltlen) {
MaybeLocal<Value> maybePadding = options->Get(env->padding_string());

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
Local<Value> paddingValue = maybePadding.ToLocalChecked();
if (paddingValue->IsNumber()) {
*padding = paddingValue->Int32Value();

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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?

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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?

Show outdated Hide outdated src/node_crypto.cc
if (mdctx->digest->flags & EVP_MD_FLAG_PKEY_METHOD_SIGNATURE) {
size_t sltmp = static_cast<size_t>(EVP_PKEY_size(pkey));
pkctx = EVP_PKEY_CTX_new(pkey, nullptr);
if (!pkctx)

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

Please use pkctx == nullptr.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

Please use pkctx == nullptr.

Show outdated Hide outdated src/node_crypto.cc
return rv;
}
if (!mdctx->digest->sign) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
}
return (mdctx->digest->sign(mdctx->digest->type, m, m_len, md, sig_len,
pkey->pkey.ptr));

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
int padding = RSA_PKCS1_PADDING;
int salt_len = RSA_PSS_SALTLEN_MAX_SIGN;
if (args[3]->IsObject()) {
Local<Object> options = Local<Object>::Cast(args[3]);

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

Show outdated Hide outdated src/node_crypto.cc
if (EVP_PKEY_CTX_set_rsa_pss_saltlen(pkctx, saltlen) <= 0)
goto err;
}
}

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 10, 2017

Member

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

This comment has been minimized.

@tniessen

tniessen Mar 11, 2017

Member

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?

@tniessen

tniessen Mar 11, 2017

Member

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?

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 12, 2017

Member

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

@bnoordhuis

bnoordhuis Mar 12, 2017

Member

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

@addaleax

This comment has been minimized.

Show comment
Hide comment
@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Mar 14, 2017

Member

@bnoordhuis I think I addressed all requested changes with the last commits.

@addaleax I do not really understand the build failures. Let's see what failed in build #8393:

node-test-binary-arm/6624:

warning: failed to remove out/Release/.nfs00000000002419ff00000223
Build step 'Execute shell' marked build as failure

node-test-commit-linux/8401

not ok 1266 parallel/test-writeuint
  ---
  duration_ms: 0.115
  severity: fail
  stack: |-
    module.js:487
        throw err;
        ^
    
    Error: Cannot find module '/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-writeuint.js'
        at Function.Module._resolveFilename (module.js:485:15)
        at Function.Module._load (module.js:437:25)
        at Module.runMain (module.js:620:10)
        at run (bootstrap_node.js:425:7)
        at startup (bootstrap_node.js:146:9)
        at bootstrap_node.js:540:3
  ...
make[1]: *** [test-ci] Error 1
make[1]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32'
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure

node-compile-windows/7507 (VS2015)

c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src/counters.h(825): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-intrinsic-lowering.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src/utils.h(307): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-inlining.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xutility(299): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-global-object-specialization.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]

And later:

cl : Command line error D8040: error creating or communicating with child process [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_3.vcxproj]
  Internal Compiler Error in C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\CL.exe.  You will be prompted to send an error report to Microsoft later.
  INTERNAL COMPILER ERROR in 'C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\CL.exe'
      Please choose the Technical Support command on the Visual C++
      Help menu, or open the Technical Support help file for more information

node-compile-windows/7507 (VCBT2015)

 > git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-7601:refs/remotes/jenkins_tmp/_jenkins_local_branch # timeout=20
ERROR: Error fetching remote repo 'jenkins_tmp'
hudson.plugins.git.GitException: Failed to fetch from git@github.com:janeasystems/node_binary_tmp.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:806)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1070)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1101)
	at hudson.scm.SCM.checkout(SCM.java:495)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1278)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:604)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)
Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-7601:refs/remotes/jenkins_tmp/_jenkins_local_branch" returned status code 128:
stdout: 
stderr: remote: Counting objects: 1173, done.        
remote: Compressing objects:   1% (1/78)           
...
remote: Compressing objects: 100% (78/78), done.        
Receiving objects:   0% (1/1173)   
Receiving objects:   1% (12/1173)   
fatal: mmap failed: No error
fatal: index-pack failed

	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1793)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:1519)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$300(CliGitAPIImpl.java:64)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:315)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:153)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:146)
	at hudson.remoting.UserRequest.perform(UserRequest.java:153)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:336)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at hudson.remoting.Engine$1$1.run(Engine.java:94)
	at java.lang.Thread.run(Unknown Source)
	at ......remote call to Channel to /162.242.156.145(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1537)
	at hudson.remoting.UserResponse.retrieve(UserRequest.java:253)
	at hudson.remoting.Channel.call(Channel.java:822)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.execute(RemoteGitImpl.java:146)
	at sun.reflect.GeneratedMethodAccessor586.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.invoke(RemoteGitImpl.java:132)
	at com.sun.proxy.$Proxy86.execute(Unknown Source)
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:804)
	... 11 more
ERROR: null

I do not see any relation to my changes. Maybe some problems with the build system?

Member

tniessen commented Mar 14, 2017

@bnoordhuis I think I addressed all requested changes with the last commits.

@addaleax I do not really understand the build failures. Let's see what failed in build #8393:

node-test-binary-arm/6624:

warning: failed to remove out/Release/.nfs00000000002419ff00000223
Build step 'Execute shell' marked build as failure

node-test-commit-linux/8401

not ok 1266 parallel/test-writeuint
  ---
  duration_ms: 0.115
  severity: fail
  stack: |-
    module.js:487
        throw err;
        ^
    
    Error: Cannot find module '/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-writeuint.js'
        at Function.Module._resolveFilename (module.js:485:15)
        at Function.Module._load (module.js:437:25)
        at Module.runMain (module.js:620:10)
        at run (bootstrap_node.js:425:7)
        at startup (bootstrap_node.js:146:9)
        at bootstrap_node.js:540:3
  ...
make[1]: *** [test-ci] Error 1
make[1]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32'
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure

node-compile-windows/7507 (VS2015)

c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src/counters.h(825): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-intrinsic-lowering.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src/utils.h(307): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-inlining.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]
C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include\xutility(299): fatal error C1060: compiler is out of heap space (compiling source file compiler\js-global-object-specialization.cc) [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_0.vcxproj]

And later:

cl : Command line error D8040: error creating or communicating with child process [c:\workspace\node-compile-windows\label\win-vs2015\deps\v8\src\v8_base_3.vcxproj]
  Internal Compiler Error in C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\CL.exe.  You will be prompted to send an error report to Microsoft later.
  INTERNAL COMPILER ERROR in 'C:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\bin\amd64\CL.exe'
      Please choose the Technical Support command on the Visual C++
      Help menu, or open the Technical Support help file for more information

node-compile-windows/7507 (VCBT2015)

 > git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-7601:refs/remotes/jenkins_tmp/_jenkins_local_branch # timeout=20
ERROR: Error fetching remote repo 'jenkins_tmp'
hudson.plugins.git.GitException: Failed to fetch from git@github.com:janeasystems/node_binary_tmp.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:806)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1070)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1101)
	at hudson.scm.SCM.checkout(SCM.java:495)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1278)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:604)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:529)
	at hudson.model.Run.execute(Run.java:1728)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:98)
	at hudson.model.Executor.run(Executor.java:404)
Caused by: hudson.plugins.git.GitException: Command "git fetch --tags --progress git@github.com:janeasystems/node_binary_tmp.git +refs/heads/jenkins-node-test-commit-windows-fanned-7601:refs/remotes/jenkins_tmp/_jenkins_local_branch" returned status code 128:
stdout: 
stderr: remote: Counting objects: 1173, done.        
remote: Compressing objects:   1% (1/78)           
...
remote: Compressing objects: 100% (78/78), done.        
Receiving objects:   0% (1/1173)   
Receiving objects:   1% (12/1173)   
fatal: mmap failed: No error
fatal: index-pack failed

	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1793)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandWithCredentials(CliGitAPIImpl.java:1519)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.access$300(CliGitAPIImpl.java:64)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$1.execute(CliGitAPIImpl.java:315)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:153)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$1.call(RemoteGitImpl.java:146)
	at hudson.remoting.UserRequest.perform(UserRequest.java:153)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:336)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
	at hudson.remoting.Engine$1$1.run(Engine.java:94)
	at java.lang.Thread.run(Unknown Source)
	at ......remote call to Channel to /162.242.156.145(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1537)
	at hudson.remoting.UserResponse.retrieve(UserRequest.java:253)
	at hudson.remoting.Channel.call(Channel.java:822)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.execute(RemoteGitImpl.java:146)
	at sun.reflect.GeneratedMethodAccessor586.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler.invoke(RemoteGitImpl.java:132)
	at com.sun.proxy.$Proxy86.execute(Unknown Source)
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:804)
	... 11 more
ERROR: null

I do not see any relation to my changes. Maybe some problems with the build system?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 14, 2017

Member

@tniessen Yeah, all of these look like infrastructure-related problems.

CI: https://ci.nodejs.org/job/node-test-commit/8440/

Member

addaleax commented Mar 14, 2017

@tniessen Yeah, all of these look like infrastructure-related problems.

CI: https://ci.nodejs.org/job/node-test-commit/8440/

Show outdated Hide outdated src/node_crypto.cc
@@ -3994,6 +3994,19 @@ bool SignBase::GetRSAOptions(Environment* env, v8::Local<v8::Object> options,
return true;
}
static bool ApplyRSAOptions(EVP_PKEY* pkey, EVP_PKEY_CTX* pkctx, int padding, int salt_len) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Mar 14, 2017

Member

Long line. Can you run make test?

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

@bnoordhuis

bnoordhuis Mar 14, 2017

Member

Long line. Can you run make test?

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

This comment has been minimized.

@tniessen

tniessen Mar 14, 2017

Member

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.

@tniessen

tniessen Mar 14, 2017

Member

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.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis
Member

bnoordhuis commented Mar 14, 2017

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Mar 14, 2017

Member

@addaleax Looks better, I just don't get why GitHub thinks test/arm failed. According to Jenkins, the build was successful and all tests passed. Am I missing something?

Member

tniessen commented Mar 14, 2017

@addaleax Looks better, I just don't get why GitHub thinks test/arm failed. According to Jenkins, the build was successful and all tests passed. Am I missing something?

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Mar 14, 2017

Member

I just don't get why GitHub thinks test/arm failed

It's a known issue with the CI. The reporter doesn't always report the status correctly for some reason.

Member

bnoordhuis commented Mar 14, 2017

I just don't get why GitHub thinks test/arm failed

It's a known issue with the CI. The reporter doesn't always report the status correctly for some reason.

@bnoordhuis

Not quite there yet but it's shaping up nicely.

Show outdated Hide outdated test/parallel/test-crypto-sign-verify.js
@@ -116,7 +116,7 @@ const keyPem = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
key: keyPem,
padding: crypto.constants.RSA_PKCS1_OAEP_PADDING
});
}, /^Error: padding must be RSA_PKCS1_PADDING or RSA_PKCS1_PSS_PADDING$/);
}, /^Error:.*illegal or unsupported padding mode$/);

This comment has been minimized.