From 8c69e06972b5b0aa7e45163f860fb63d90203289 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 27 Mar 2019 12:10:48 -0700 Subject: [PATCH] tls: return an OpenSSL error from renegotiate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/26868 Reviewed-By: Anna Henningsen Reviewed-By: Ben Noordhuis Reviewed-By: Tobias Nießen --- doc/api/errors.md | 5 ----- doc/api/tls.md | 7 ++++--- lib/_tls_wrap.js | 7 ++++--- lib/internal/errors.js | 1 - src/node_crypto.cc | 6 +++--- test/parallel/test-tls-client-renegotiation-13.js | 8 ++++++-- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index d41de694961ca5..44ec5548aeab35 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1771,11 +1771,6 @@ Valid TLS protocol versions are `'TLSv1'`, `'TLSv1.1'`, or `'TLSv1.2'`. Attempting to set a TLS protocol `minVersion` or `maxVersion` conflicts with an attempt to set the `secureProtocol` explicitly. Use one mechanism or the other. - -### ERR_TLS_RENEGOTIATE - -An attempt to renegotiate the TLS session failed. - ### ERR_TLS_RENEGOTIATION_DISABLED diff --git a/doc/api/tls.md b/doc/api/tls.md index c86d2ac08768cb..ee390e1bdbc846 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -1046,9 +1046,10 @@ added: v0.11.8 `true`. * `requestCert` * `callback` {Function} If `renegotiate()` returned `true`, callback is - attached once to the `'secure'` event. If it returned `false`, it will be - called in the next tick with `ERR_TLS_RENEGOTIATE`, unless the `tlsSocket` - has been destroyed, in which case it will not be called at all. + attached once to the `'secure'` event. If `renegotiate()` returned `false`, + `callback` will be called in the next tick with an error, unless the + `tlsSocket` has been destroyed, in which case `callback` will not be called + at all. * Returns: {boolean} `true` if renegotiation was initiated, `false` otherwise. diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 5e0f648797d661..44a5994a8df9d5 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -48,7 +48,6 @@ const { ERR_SOCKET_CLOSED, ERR_TLS_DH_PARAM_SIZE, ERR_TLS_HANDSHAKE_TIMEOUT, - ERR_TLS_RENEGOTIATE, ERR_TLS_RENEGOTIATION_DISABLED, ERR_TLS_REQUIRED_SERVER_NAME, ERR_TLS_SESSION_ATTACK, @@ -661,9 +660,11 @@ TLSSocket.prototype.renegotiate = function(options, callback) { // Ensure that we'll cycle through internal openssl's state this.write(''); - if (!this._handle.renegotiate()) { + try { + this._handle.renegotiate(); + } catch (err) { if (callback) { - process.nextTick(callback, new ERR_TLS_RENEGOTIATE()); + process.nextTick(callback, err); } return false; } diff --git a/lib/internal/errors.js b/lib/internal/errors.js index ea7c5a8aa469aa..c7dac93783fed3 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1046,7 +1046,6 @@ E('ERR_TLS_INVALID_PROTOCOL_VERSION', '%j is not a valid %s TLS protocol version', TypeError); E('ERR_TLS_PROTOCOL_VERSION_CONFLICT', 'TLS protocol version %j conflicts with secureProtocol %j', TypeError); -E('ERR_TLS_RENEGOTIATE', 'Attempt to renegotiate TLS session failed', Error); E('ERR_TLS_RENEGOTIATION_DISABLED', 'TLS session renegotiation disabled for this socket', Error); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c86414fc6296b0..649ef9d5b4061e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2339,9 +2339,9 @@ void SSLWrap::Renegotiate(const FunctionCallbackInfo& args) { ClearErrorOnReturn clear_error_on_return; - // TODO(@sam-github) Return/throw an error, don't discard the SSL error info. - bool yes = SSL_renegotiate(w->ssl_.get()) == 1; - args.GetReturnValue().Set(yes); + if (SSL_renegotiate(w->ssl_.get()) != 1) { + return ThrowCryptoError(w->ssl_env(), ERR_get_error()); + } } diff --git a/test/parallel/test-tls-client-renegotiation-13.js b/test/parallel/test-tls-client-renegotiation-13.js index 8af63c4f791ddc..075eb70e917c06 100644 --- a/test/parallel/test-tls-client-renegotiation-13.js +++ b/test/parallel/test-tls-client-renegotiation-13.js @@ -28,8 +28,12 @@ connect({ assert.strictEqual(client.getProtocol(), 'TLSv1.3'); const ok = client.renegotiate({}, common.mustCall((err) => { - assert(err.code, 'ERR_TLS_RENEGOTIATE'); - assert(err.message, 'Attempt to renegotiate TLS session failed'); + assert.throws(() => { throw err; }, { + message: 'error:1420410A:SSL routines:SSL_renegotiate:wrong ssl version', + code: 'ERR_SSL_WRONG_SSL_VERSION', + library: 'SSL routines', + reason: 'wrong ssl version', + }); cleanup(); }));