Decorate crypto openssl errors with .code, .reason, ... by sam-github · Pull Request #26868 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation68 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

sam-github

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

@nodejs-github-bot

@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 titleDecorate openssl errors Decorate openssl errors with .code, .reason, ...

Mar 22, 2019

@sam-github sam-github changed the titleDecorate openssl errors with .code, .reason, ... Decorate crypto openssl errors with .code, .reason, ...

Mar 22, 2019

mscdex

mscdex

mscdex

addaleax

@tniessen

tniessen

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?

bnoordhuis

BridgeAR

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

@sam-github

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

@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

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

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

@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

@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

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

@sam-github

@BridgeAR

@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

@nodejs-github-bot

addaleax

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 Decorate(Environment* env, Local obj,

Choose a reason for hiding this comment

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

Local Decorate(Environment* env, Local obj,
MaybeLocal Decorate(Environment* env, Local obj,
if (obj->Set(env->isolate()->GetCurrentContext(),
env->code_string(),
OneByteString(env->isolate(), code)).IsNothing())
return obj;

Choose a reason for hiding this comment

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

return obj;
return MaybeLocal<>();

(here and above)

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

Choose a reason for hiding this comment

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

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

@sam-github

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

@sam-github

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

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

@nodejs-github-bot

@nodejs-github-bot

@sam-github

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

@sam-github

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

sam-github added a commit that referenced this pull request

Mar 28, 2019

@sam-github

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 anna@addaleax.net Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Tobias Nießen tniessen@tnie.de

sam-github added a commit that referenced this pull request

Mar 28, 2019

@sam-github

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 anna@addaleax.net Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Tobias Nießen tniessen@tnie.de

BethGriggs pushed a commit that referenced this pull request

Apr 5, 2019

@sam-github @BethGriggs

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 anna@addaleax.net Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Tobias Nießen tniessen@tnie.de

BethGriggs pushed a commit that referenced this pull request

Apr 5, 2019

@sam-github @BethGriggs

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 anna@addaleax.net Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Tobias Nießen tniessen@tnie.de

This was referenced

Apr 23, 2019

Reviewers

@mscdex mscdex mscdex left review comments

@addaleax addaleax addaleax approved these changes

@targos targos targos left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@tniessen tniessen tniessen approved these changes

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.