Skip to content
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 opensslerror stack #15518

Closed
wants to merge 8 commits into from

Conversation

@gla5001
Copy link
Contributor

gla5001 commented Sep 21, 2017

Feature request to add openSSL error stack to the exception object
thrown from crypto. New exception property only added to object
if the error stack has not cleared out prior to calling
ThrowCryptoError.

I did something very wrong when trying to rebase, so i just created a new branch and a new PR. This PR has all the changes requested from #14725. I will close the other one.

Refs: #5444

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

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

This comment has been minimized.

Copy link
@gla5001

gla5001 Sep 21, 2017

Author Contributor

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

if (es->bottom != es->top) {
Local<Array> error_stack = Array::New(env->isolate());
int top = es->top;

This comment has been minimized.

Copy link
@gla5001

gla5001 Sep 21, 2017

Author Contributor

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

// arithmetic to loop back around in the case where bottom is after top.
// Using ERR_NUM_ERRORS macro defined in openssl.
es->top = (((es->top - 1) % ERR_NUM_ERRORS) + ERR_NUM_ERRORS) %
ERR_NUM_ERRORS;
}

This comment has been minimized.

Copy link
@gla5001

gla5001 Sep 21, 2017

Author Contributor

handle the ring buffer with modular arithmetic

@@ -4326,8 +4339,6 @@ SignBase::Error Verify::VerifyFinal(const char* key_pem,
int r = 0;
EVP_PKEY_CTX* pkctx = nullptr;

ERR_set_mark();

bp = BIO_new_mem_buf(const_cast<char*>(key_pem), key_pem_len);

This comment has been minimized.

Copy link
@gla5001

gla5001 Sep 21, 2017

Author Contributor

this is not needed

err.openSSLErrorStack !== undefined &&
Array.isArray(err.openSSLErrorStack) &&
err.openSSLErrorStack.length === 0) {
err.openSSLErrorStack === undefined) {
return true;

This comment has been minimized.

Copy link
@gla5001

gla5001 Sep 21, 2017

Author Contributor

update test since the openSSLErrorStack is not always added

@gla5001 gla5001 mentioned this pull request Sep 21, 2017
4 of 4 tasks complete
@gla5001

This comment has been minimized.

Copy link
Contributor Author

gla5001 commented Sep 21, 2017

The only new commit is 720a5af. Everything else was reviewed in #14725.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Sep 21, 2017

@jasnell jasnell requested review from bnoordhuis and targos Sep 21, 2017
@@ -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.

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.

This comment has been minimized.

Copy link
@jasnell

jasnell Sep 25, 2017

Member

long line here?

Copy link
Member

indutny left a comment

LGTM

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Sep 27, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Sep 27, 2017

@gla5001 when trying to rebase this on the CI it gets a conflict. I am a bit surprised that this is not shown here but would you be so kind and rebase this nevertheless?

@gla5001

This comment has been minimized.

Copy link
Contributor Author

gla5001 commented Sep 28, 2017

@BridgeAR sure thing

gla5001 added 8 commits Aug 10, 2017
@gla5001 gla5001 force-pushed the gla5001:crypto-add-opensslerror-stack branch from 537284e to 2ad7440 Sep 28, 2017
@gla5001

This comment has been minimized.

Copy link
Contributor Author

gla5001 commented Sep 28, 2017

@BridgeAR rebased and pushed. Could you let me know if this resolves the issue?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Sep 28, 2017

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Sep 28, 2017

Landed in ccfcd88

@BridgeAR BridgeAR closed this Sep 28, 2017
BridgeAR added a commit that referenced this pull request Sep 28, 2017
Add openSSL error stack to the exception object thrown from crypto.
The new exception property is only added to the object if the error
stack has not cleared out prior to calling ThrowCryptoError.

PR-URL: #15518
Refs: #5444
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins added a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
Add openSSL error stack to the exception object thrown from crypto.
The new exception property is only added to the object if the error
stack has not cleared out prior to calling ThrowCryptoError.

PR-URL: nodejs#15518
Refs: nodejs#5444
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Sep 28, 2017

The commit log could have had a link to #14725, that's where 95% of the review took place. This comment will have to do.

MylesBorins added a commit that referenced this pull request Sep 29, 2017
Add openSSL error stack to the exception object thrown from crypto.
The new exception property is only added to the object if the error
stack has not cleared out prior to calling ThrowCryptoError.

PR-URL: #15518
Refs: #5444
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
addaleax added a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
Add openSSL error stack to the exception object thrown from crypto.
The new exception property is only added to the object if the error
stack has not cleared out prior to calling ThrowCryptoError.

PR-URL: nodejs/node#15518
Refs: nodejs/node#5444
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 3, 2017
Add openSSL error stack to the exception object thrown from crypto.
The new exception property is only added to the object if the error
stack has not cleared out prior to calling ThrowCryptoError.

PR-URL: #15518
Refs: #5444
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins added a commit that referenced this pull request Oct 3, 2017
Add openSSL error stack to the exception object thrown from crypto.
The new exception property is only added to the object if the error
stack has not cleared out prior to calling ThrowCryptoError.

PR-URL: #15518
Refs: #5444
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Oct 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

edit: this likely shouldn'y be backported if it is changing error messages prior to error codes... but I wanted to confirm

@gla5001

This comment has been minimized.

Copy link
Contributor Author

gla5001 commented Oct 17, 2017

@MylesBorins I believe it should not be backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.