Decorate crypto openssl errors with .code, .reason, ... #26868
Conversation
|
cc @nodejs/crypto |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@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 All the crypto errors that use There are also many places |
|
@sam-github especially the Lines 516 to 519 in 97737fd So far we've always documented all Node.js errors in |
|
@sam-github Sounds good! I didn't mean to use |
Not always. We don't do it for system error codes, the original ones for which 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 I guess I could just set |
OK, I looked at fs. I'm not introducing the throwing of errors to node_crypto.cc, its usage of 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. |
a5d4adf to
adb9f7c
Compare
True, I forgot about those! Thanks for looking into this. It seems like it makes perfectly sense the way you implemented it. |
f48152e to
f9b4d0d
Compare
f9b4d0d to
4ced04e
Compare
453ac75 to
b546c89
Compare
There was a problem hiding this comment.
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
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
| Local<Object> Decorate(Environment* env, Local<Object> obj, | |
| MaybeLocal<Object> Decorate(Environment* env, Local<Object> obj, |
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
| return obj; | |
| return MaybeLocal<>(); |
(here and above)
src/node_crypto.cc
Outdated
There was a problem hiding this comment.
| 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.
b546c89 to
3e3e8d8
Compare
|
@addaleax |
|
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>
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), orvcbuild test(Windows) passes