Skip to content

Commit

Permalink
tls: return an OpenSSL error from renegotiate
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
  • Loading branch information
sam-github committed Mar 28, 2019
1 parent bcbd35a commit 8c69e06
Show file tree
Hide file tree
Showing 6 changed files with 17 additions and 17 deletions.
5 changes: 0 additions & 5 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
7 changes: 4 additions & 3 deletions doc/api/tls.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
7 changes: 4 additions & 3 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down
1 change: 0 additions & 1 deletion lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
6 changes: 3 additions & 3 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}


Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-tls-client-renegotiation-13.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));

Expand Down

0 comments on commit 8c69e06

Please sign in to comment.