Decorate crypto openssl errors with .code, .reason, ... by sam-github · Pull Request #26868 · nodejs/node · 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

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

@sam-github build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3013/pipeline

cc @nodejs/crypto

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?

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

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.

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

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

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

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

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.

CI: https://ci.nodejs.org/job/node-test-pull-request/21895/

ci: https://ci.nodejs.org/job/node-test-pull-request/21895/

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

CI: https://ci.nodejs.org/job/node-test-pull-request/21970/

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

Suggested change

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

Suggested change

      
                  return obj;
          
                  return MaybeLocal<>();

(here and above)

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.

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

CI: https://ci.nodejs.org/job/node-test-pull-request/21972/

CI: https://ci.nodejs.org/job/node-test-pull-request/21985/

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

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

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>

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>

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>

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>

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 changed the title Decorate openssl errors Decorate openssl errors with .code, .reason, ... Mar 22, 2019

sam-github changed the title Decorate openssl errors with .code, .reason, ... Decorate crypto openssl errors with .code, .reason, ... Mar 22, 2019

mscdex reviewed Mar 22, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

mscdex reviewed Mar 22, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

mscdex reviewed Mar 22, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

addaleax reviewed Mar 22, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

Comment thread src/node_crypto.cc Outdated

Comment thread test/parallel/test-crypto.js Outdated

tniessen reviewed Mar 24, 2019

View reviewed changes

Comment thread doc/api/errors.md Outdated

bnoordhuis reviewed Mar 25, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

Comment thread src/node_crypto.cc Outdated

Comment thread src/node_crypto.cc Outdated

Comment thread src/node_crypto.cc Outdated

BridgeAR reviewed Mar 25, 2019

View reviewed changes

Comment thread test/parallel/test-crypto-stream.js Outdated

sam-github force-pushed the decorate-openssl-errors branch from a5d4adf to adb9f7c Compare March 25, 2019 20:34

sam-github force-pushed the decorate-openssl-errors branch 3 times, most recently from f48152e to f9b4d0d Compare March 26, 2019 15:10

sam-github commented Mar 26, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

sam-github force-pushed the decorate-openssl-errors branch from f9b4d0d to 4ced04e Compare March 26, 2019 15:21

sam-github commented Mar 26, 2019

View reviewed changes

Comment thread src/node_crypto.cc Outdated

sam-github force-pushed the decorate-openssl-errors branch from 453ac75 to b546c89 Compare March 27, 2019 19:11

addaleax reviewed Mar 27, 2019

View reviewed changes

sam-github added 2 commits March 27, 2019 14:13

crypto: add openssl specific error properties

c5bef6e

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

3e3e8d8

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 force-pushed the decorate-openssl-errors branch from b546c89 to 3e3e8d8 Compare March 27, 2019 21:13

tniessen mentioned this pull request Mar 28, 2019

crypto: decode missing passphrase errors #25208

Closed

4 tasks

sam-github closed this Mar 28, 2019

sam-github deleted the decorate-openssl-errors branch March 28, 2019 21:05

targos added the backport-requested-v11.x label Mar 28, 2019

sam-github mentioned this pull request Apr 9, 2019

Fix crypto abort on throwing setter #27157

Closed

4 tasks

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

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

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

Uh oh!

Uh oh!

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

Decorate crypto openssl errors with .code, .reason, ...#26868sam-github wants to merge 2 commits intonodejs:masternodejs/node:masterfrom sam-github:decorate-openssl-errorssam-github/node:decorate-openssl-errorsCopy head branch name to clipboard

Conversation

sam-github commented Mar 22, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tniessen commented Mar 24, 2019

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sam-github commented Mar 25, 2019

Uh oh!

sam-github commented Mar 25, 2019

Uh oh!

BridgeAR commented Mar 25, 2019

Uh oh!

tniessen commented Mar 25, 2019

Uh oh!

sam-github commented Mar 25, 2019

Uh oh!

sam-github commented Mar 25, 2019

Uh oh!

nodejs-github-bot commented Mar 25, 2019

Uh oh!

sam-github commented Mar 25, 2019

Uh oh!

BridgeAR commented Mar 26, 2019

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 27, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Mar 27, 2019

Choose a reason for hiding this comment

Uh oh!

sam-github commented Mar 27, 2019

Uh oh!

nodejs-github-bot commented Mar 27, 2019

Uh oh!

nodejs-github-bot commented Mar 28, 2019

Uh oh!

sam-github commented Mar 28, 2019

Uh oh!

sam-github commented Mar 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Decorate crypto openssl errors with .code, .reason, ...#26868
sam-github wants to merge 2 commits into
nodejs:masterfrom
sam-github:decorate-openssl-errors