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

Decorate crypto openssl errors with .code, .reason, ... #26868

Closed
wants to merge 2 commits into from

Conversation

sam-github
Copy link
Contributor

Crypto equivalent of this change to TLS: #25093

** crypto: add openssl specific error properties

Don't force the user to parse the long-style OpenSSL error message,
decorate the error with the library, reason, code, function.

** tls: return an OpenSSL error from renegotiate

A generic error lacks any of the context or detail of the underlying
OpenSSL error, so throw from C++, and report the OpenSSL error to the
callback.

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. tls Issues and PRs related to the tls subsystem. labels Mar 22, 2019
@sam-github sam-github changed the title Decorate openssl errors Decorate openssl errors with .code, .reason, ... Mar 22, 2019
@sam-github sam-github changed the title Decorate openssl errors with .code, .reason, ... Decorate crypto openssl errors with .code, .reason, ... Mar 22, 2019
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
test/parallel/test-crypto.js Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

cc @nodejs/crypto

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, thanks for doing this, Sam. The only thing I am mildly concerned about are the values of the .code property. These errors are not caused within node, they are caused within OpenSSL. While some codes might give some insight (e.g. ERR_CRYPTO_), others require some knowledge about cryptography (e.g. ERR_PKCS12_) or OpenSSL (ERR_BIO_) to make the connection to crypto, and some codes show no relation to crypto or OpenSSL at all (ERR_STORE_, ERR_USER_, ERR_ASYNC_, ERR_UI_). This might not be a problem immediately, but if the error is propagated to callers, I imagine it might be difficult to debug. I believe most crypto-specific errors currently use the prefix ERR_CRYPTO_. Maybe we should use a common prefix for these errors that are generated by OpenSSL, not by node?

doc/api/errors.md Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

As note: other C++ code uses a different pattern to create the errors:

const ctx = {};
if (!this._handle.renegotiate(ctx)) {
  if (callback) {
    const err = tlsRegnegotiationException(ctx);
    process.nextTick(callback, err);
  }
  return false;
}

That way all error handling is done in JS and all necessary properties are attached to the ctx object in C++.

I personally have no strong opinion either way. It would probably just be good to have a consistent way.

Ping @joyeecheung

test/parallel/test-crypto-stream.js Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor Author

As note: other C++ code uses a different pattern to create the errors:

@BridgeAR can you point me to some example code? No crypto/tls code does anything like that, and I'm not picturing how that would work.

@sam-github
Copy link
Contributor Author

@tniessen I changed the prefix so errors will now be ERR_OSSL_PKCS12_(reason string) instead of just ERR_PKCS12_(reason string), does that make the source more clear?

All the crypto errors that use ERR_CRYPTO_ are non-OpenSSL errors and, with the sole exception of ERR_CRYPTO_ECDH_INVALID_PUBLIC_KEY, come from the non-C++ lib/ code. so I think that prefix should be preserved for errors originating in node's crypto module, not 0penSSL errors.

There are also many places ThrowError() is used in node_crypto.cc, some of them in ways that discard underlying OpenSSL error information. None of the errors thrown that way have any of the standard properties (.code, etc.). I looked at some of them, but they can't be changed in bulk, they require careful thought for each one, and possibly additional test code to exercise error paths.

@BridgeAR
Copy link
Member

@sam-github especially the fs code e.g.,

node/lib/fs.js

Lines 516 to 519 in 97737fd

const ctx = {};
const result = binding.read(fd, buffer, offset, length, position,
undefined, ctx);
handleErrorFromBinding(ctx);


So far we've always documented all Node.js errors in doc/errors.md. It seems like we have no direct control over these errors and we can not list all codes here. Should we really switch the errors to our code system therefore or should be just have a single error type like ERR_CRYPTO and just add additional properties to those errors?

@tniessen
Copy link
Member

@sam-github Sounds good! I didn't mean to use ERR_CRYPTO_, I just used it as an example for a common prefix :)

@sam-github
Copy link
Contributor Author

@BridgeAR

So far we've always documented all Node.js errors in doc/errors.md.

Not always. We don't do it for system error codes, the original ones for which .code was created (IIRC): see https://nodejs.org/api/errors.html#errors_common_system_errors which only lists a handful of the many, many error codes possible.

This PR is partially motivated because I found examples in our own test suites (so I expect elsewhere) where code is running a regex against the long-style OpenSSL error string in .message to identify the reason string. The standard .code has a number of purposes, but one is to allow checks for specific error conditions to not require regex matching against the .message.

I guess I could just set .code = ERR_OPENSSL, and then people could do matches against the .reason string if they wanted to know the specific error, but it seems to me that is just annoying. The .code exists to be the specific error, why make our users look in two places to find the reason for an error?

@sam-github
Copy link
Contributor Author

As note: other C++ code uses a different pattern to create the errors:

OK, I looked at fs. I'm not introducing the throwing of errors to node_crypto.cc, its usage of ThrowCryptoError() and ThrowError() are pre-existing. Reworking the entire error handling system is unrelated to adding a .code property. Someone else will have to step up to do that kind of major refactor if it needs doing.

wrt. renegotiate() specifically, its an odd API, due to the underlying protocol support which is out of our control. The error could be thrown directly instead of caught and forwarded to the callback, but that would semver-major. Worse, whether the API can error sync or not isn't under the direct control of the user, unlike most sync errors thrown by async Node.js APIs. It only fails sync if TLS1.3 got negotiated, so its partly based on the capabilities of the peer. It seems better to me to keep this semver-minor, and keep the error async as it has been in the past.

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor Author

@BridgeAR
Copy link
Member

@sam-github

Not always. We don't do it for system error codes

True, I forgot about those! Thanks for looking into this. It seems like it makes perfectly sense the way you implemented it.

@sam-github sam-github force-pushed the decorate-openssl-errors branch 3 times, most recently from f48152e to f9b4d0d Compare March 26, 2019 15:10
src/node_crypto.cc Outdated Show resolved Hide resolved
src/node_crypto.cc Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Sorry, missed this during the last review – It’s not blocking, but I’ve suggested some changes that currently don’t have much practical impact, but make the code follow the style of a “typical” Maybe API that uses empty Maybes/MaybeLocals to indicate a pending exception

@@ -212,6 +212,115 @@ static int NoPasswordCallback(char* buf, int size, int rwflag, void* u) {
}


// namespace node::crypto::error
namespace error {
Local<Object> Decorate(Environment* env, Local<Object> obj,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Local<Object> Decorate(Environment* env, Local<Object> obj,
MaybeLocal<Object> Decorate(Environment* env, Local<Object> obj,

if (obj->Set(env->isolate()->GetCurrentContext(),
env->code_string(),
OneByteString(env->isolate(), code)).IsNothing())
return obj;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return obj;
return MaybeLocal<>();

(here and above)

Local<Object> obj;
if (!exception->ToObject(env->context()).ToLocal(&obj))
return;
error::Decorate(env, obj, err);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error::Decorate(env, obj, err);
if (error::Decorate(env, obj, err).IsEmpty())
return;

Don't force the user to parse the long-style OpenSSL error message,
decorate the error with the library, reason, code, function.
A generic error lacks any of the context or detail of the underlying
OpenSSL error, so throw from C++, and report the OpenSSL error to the
callback.
@sam-github
Copy link
Contributor Author

@addaleax Decorate()'s return value was unused, I just deleted the unused code.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@sam-github
Copy link
Contributor Author

test/parallel/test-gc-http-client-onerror.js on freebsd failed, which doesn't involve crypto at all. resumed.

@sam-github
Copy link
Contributor Author

Landed in 805e614...8c69e06, thanks for all the help.

@sam-github sam-github closed this Mar 28, 2019
@sam-github sam-github deleted the decorate-openssl-errors branch March 28, 2019 21:05
sam-github added a commit that referenced this pull request Mar 28, 2019
Don't force the user to parse the long-style OpenSSL error message,
decorate the error with the library, reason, code, function.

PR-URL: #26868
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
sam-github added a commit that referenced this pull request Mar 28, 2019
A generic error lacks any of the context or detail of the underlying
OpenSSL error, so throw from C++, and report the OpenSSL error to the
callback.

PR-URL: #26868
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
Don't force the user to parse the long-style OpenSSL error message,
decorate the error with the library, reason, code, function.

PR-URL: #26868
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 5, 2019
A generic error lacks any of the context or detail of the underlying
OpenSSL error, so throw from C++, and report the OpenSSL error to the
callback.

PR-URL: #26868
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants