From 3e3e8d890a7f64b753d145d4cf1239565766eaea Mon Sep 17 00:00:00 2001
From: Sam Roberts <vieuxtech@gmail.com>
Date: Wed, 27 Mar 2019 12:10:48 -0700
Subject: [PATCH] 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.
---
 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.
 
-<a id="ERR_TLS_RENEGOTIATE"></a>
-### ERR_TLS_RENEGOTIATE
-
-An attempt to renegotiate the TLS session failed.
-
 <a id="ERR_TLS_RENEGOTIATION_DISABLED"></a>
 ### 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 e3347374cc702b..fee313a0f466e9 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 25bd3cbf57648d..e963a50c500644 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -2339,9 +2339,9 @@ void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& 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();
   }));