Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Servername is not set on TLS sockets if there is a TLS client error #27699

Closed
pimterry opened this issue May 14, 2019 · 1 comment
Closed

Servername is not set on TLS sockets if there is a TLS client error #27699

pimterry opened this issue May 14, 2019 · 1 comment
Labels
feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem.

Comments

@pimterry
Copy link
Member

  • Version: v10.15.3
  • Platform: Linux
  • Subsystem: TLS

I am writing an HTTPS server and I want to try & detect when clients connect but reject my certificate. For irrelevant reasons this happens often.

If this happens, it typically triggers a tlsClientError event. When that happens, even though the servername has been received and SNICallback has been called successfully, the servername field is still not set on the TLS socket provided with the event. For successful connections however (i.e. secureConnection), it is always available.

This is because it's only set on the socket in _finishInit, which is only gets called after a successful handshake has been completed:

node/lib/_tls_wrap.js

Lines 735 to 747 in 495822f

TLSSocket.prototype._finishInit = function() {
// Guard against getting onhandshakedone() after .destroy().
// * 1.2: If destroy() during onocspresponse(), then write of next handshake
// record fails, the handshake done info callbacks does not occur, and the
// socket closes.
// * 1.3: The OCSP response comes in the same record that finishes handshake,
// so even after .destroy(), the handshake done info callback occurs
// immediately after onocspresponse(). Ignore it.
if (!this._handle)
return;
this.alpnProtocol = this._handle.getALPNNegotiatedProtocol();
this.servername = this._handle.getServername();

It'd be very useful if this field was set earlier, as soon as the server name has been received, to provide extra context to TLS errors like these.

@bnoordhuis bnoordhuis added feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem. labels May 14, 2019
@bnoordhuis
Copy link
Member

I'd say you're welcome to open a pull request but there may be some edge cases you'll have to handle. At least some of the time 'tlsClientError' will be emitted before the SNI extension has been parsed.

oyyd added a commit to oyyd/node that referenced this issue May 18, 2019
This commit makes `TLSSocket` set the `servername` property on
`SSL_CTX_set_tlsext_servername_callback` so that we could get it
later even if errors happen.

Fixes: nodejs#27699
@oyyd oyyd closed this as completed in d2cabee May 23, 2019
targos pushed a commit that referenced this issue May 23, 2019
This commit makes `TLSSocket` set the `servername` property on
`SSL_CTX_set_tlsext_servername_callback` so that we could get it
later even if errors happen.

Fixes: #27699

PR-URL: #27759
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants