Skip to content

Crypto add opensslerror stack #15518

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.

Sign up for GitHub

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

Closed
gla5001 wants to merge 8 commits into from
Closed

Crypto add opensslerror stack #15518

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f2c9e49
crypto: better crypto error messages
gla5001 Aug 10, 2017
dc2165a
Review updates and tests
gla5001 Aug 11, 2017
5c24f07
Additional review updates.
gla5001 Aug 15, 2017
d1296cf
Review update. Use the overload of Set()
gla5001 Aug 15, 2017
2b177cb
Nit fix: changing local variable to snake_case
gla5001 Aug 16, 2017
bbaea8b
PR review updates
gla5001 Sep 21, 2017
a5a2f64
changing openSSLErrorStack property name to opensslErrorStack
gla5001 Sep 22, 2017
2ad7440
fixing long doc line
gla5001 Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
changing openSSLErrorStack property name to opensslErrorStack
  • Loading branch information
@gla5001
gla5001 committed Sep 28, 2017
commit a5a2f64b94467836a53c91a1d1058af1213f7319
2 changes: 1 addition & 1 deletion doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ detailing the point in the code at which the `Error` was instantiated, and may
provide a text description of the error.
Copy link
Copy Markdown
Contributor Author

@gla5001 gla5001 Sep 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit has the latest review comment fixes.
@bnoordhuis, i believe i've addressed all of your comments. Thanks for the review.


For crypto only, `Error` objects will include the OpenSSL error stack in a
separate property called `openSSLErrorStack` if it is available when the error is thrown.
Copy link
Copy Markdown
Member

@jasnell jasnell Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long line here?

separate property called `opensslErrorStack` if it is available when the error is thrown.

All errors generated by Node.js, including all System and JavaScript errors,
will either be instances of, or inherit from, the `Error` class.
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ class ModuleWrap;
V(onstreamclose_string, "onstreamclose") \
V(ontrailers_string, "ontrailers") \
V(onwrite_string, "onwrite") \
V(openssl_error_stack, "openSSLErrorStack") \
V(openssl_error_stack, "opensslErrorStack") \
V(output_string, "output") \
V(order_string, "order") \
V(owner_string, "owner") \
Expand Down
6 changes: 3 additions & 3 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ void ThrowCryptoError(Environment* env,
Local<Array> error_stack = Array::New(env->isolate());
int top = es->top;

Copy link
Copy Markdown
Contributor Author

@gla5001 gla5001 Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, we wont add the property to the exception every time.

// Build the error_stack array to be added to openSSLErrorStack property.
// Build the error_stack array to be added to opensslErrorStack property.
for (unsigned int i = 0; es->bottom != es->top;) {
unsigned long err_buf = es->err_buffer[es->top]; // NOLINT(runtime/int)
// Only add error string if there is valid err_buffer.
Expand All @@ -303,9 +303,9 @@ void ThrowCryptoError(Environment* env,
// Restore top.
es->top = top;

// Add the openSSLErrorStack property to the exception object.
// Add the opensslErrorStack property to the exception object.
// The new property will look like the following:
// openSSLErrorStack: [
// opensslErrorStack: [
// 'error:0906700D:PEM routines:PEM_ASN1_read_bio:ASN1 lib',
// 'error:0D07803A:asn1 encoding routines:ASN1_ITEM_EX_D2I:nested asn1 err'
// ]
Expand Down
50 changes: 25 additions & 25 deletions test/parallel/test-crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ assert.throws(function() {
const notcontext = { setOptions: context.setOptions, setKey: context.setKey };
tls.createSecureContext({ secureOptions: 1 }, notcontext);
}, (err) => {
// Throws TypeError, so there is no openSSLErrorStack property.
// Throws TypeError, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^TypeError: Illegal invocation$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});
Expand All @@ -63,32 +63,32 @@ assert.doesNotThrow(function() {
assert.throws(function() {
tls.createSecureContext({ pfx: certPfx });
}, (err) => {
// Throws general Error, so there is no openSSLErrorStack property.
// Throws general Error, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^Error: mac verify failure$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});

assert.throws(function() {
tls.createSecureContext({ pfx: certPfx, passphrase: 'test' });
}, (err) => {
// Throws general Error, so there is no openSSLErrorStack property.
// Throws general Error, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^Error: mac verify failure$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});

assert.throws(function() {
tls.createSecureContext({ pfx: 'sample', passphrase: 'test' });
}, (err) => {
// Throws general Error, so there is no openSSLErrorStack property.
// Throws general Error, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^Error: not enough data$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});
Expand Down Expand Up @@ -169,10 +169,10 @@ assert.throws(function() {
}, (err) => {
const errorMessage =
common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/;
// Throws general Error, so there is no openSSLErrorStack property.
// Throws general Error, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
errorMessage.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});
Expand All @@ -182,43 +182,43 @@ assert.throws(function() {
}, (err) => {
const errorMessage =
common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/;
// Throws general Error, so there is no openSSLErrorStack property.
// Throws general Error, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
errorMessage.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});

assert.throws(function() {
crypto.createHash('sha1').update('0', 'hex');
}, (err) => {
// Throws TypeError, so there is no openSSLErrorStack property.
// Throws TypeError, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^TypeError: Bad input string$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});

assert.throws(function() {
crypto.createSign('SHA1').update('0', 'hex');
}, (err) => {
// Throws TypeError, so there is no openSSLErrorStack property.
// Throws TypeError, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^TypeError: Bad input string$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});

assert.throws(function() {
crypto.createVerify('SHA1').update('0', 'hex');
}, (err) => {
// Throws TypeError, so there is no openSSLErrorStack property.
// Throws TypeError, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^TypeError: Bad input string$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});
Expand All @@ -237,7 +237,7 @@ assert.throws(function() {
}, (err) => {
if ((err instanceof Error) &&
/digest too big for rsa key$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
Copy link
Copy Markdown
Contributor Author

@gla5001 gla5001 Sep 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update test since the openSSLErrorStack is not always added

}
});
Expand All @@ -257,13 +257,13 @@ assert.throws(function() {
// this would inject errors onto OpenSSL's error stack
crypto.createSign('sha1').sign(sha1_privateKey);
}, (err) => {
// Throws crypto error, so there is an openSSLErrorStack property.
// Throws crypto error, so there is an opensslErrorStack property.
// The openSSL stack should have content.
if ((err instanceof Error) &&
/asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag/.test(err) &&
err.openSSLErrorStack !== undefined &&
Array.isArray(err.openSSLErrorStack) &&
err.openSSLErrorStack.length > 0) {
err.opensslErrorStack !== undefined &&
Array.isArray(err.opensslErrorStack) &&
err.opensslErrorStack.length > 0) {
return true;
}
});
Expand All @@ -274,10 +274,10 @@ console.log(crypto.randomBytes(16));
assert.throws(function() {
tls.createSecureContext({ crl: 'not a CRL' });
}, (err) => {
// Throws general error, so there is no openSSLErrorStack property.
// Throws general error, so there is no opensslErrorStack property.
if ((err instanceof Error) &&
/^Error: Failed to parse CRL$/.test(err) &&
err.openSSLErrorStack === undefined) {
err.opensslErrorStack === undefined) {
return true;
}
});
Expand Down