From d2cabee64c60a7d6e2023dfb60c26cb66dc2eaa4 Mon Sep 17 00:00:00 2001
From: oyyd <oyydoibh@gmail.com>
Date: Fri, 17 May 2019 21:19:40 +0800
Subject: [PATCH] tls: set tlsSocket.servername as early as possible

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: https://github.com/nodejs/node/issues/27699

PR-URL: https://github.com/nodejs/node/pull/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>
---
 lib/_tls_wrap.js                         |  5 ++-
 src/tls_wrap.cc                          |  8 ++++
 test/parallel/test-tls-sni-servername.js | 56 ++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-tls-sni-servername.js

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 618f3892cee83b..4f46204d06ebf1 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -774,7 +774,10 @@ TLSSocket.prototype._finishInit = function() {
     return;
 
   this.alpnProtocol = this._handle.getALPNNegotiatedProtocol();
-  this.servername = this._handle.getServername();
+  // The servername could be set by TLSWrap::SelectSNIContextCallback().
+  if (this.servername === null) {
+    this.servername = this._handle.getServername();
+  }
 
   debug('%s _finishInit',
         this._tlsOptions.isServer ? 'server' : 'client',
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index cd6321b9693c3b..a1944df0563731 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -1033,6 +1033,14 @@ int TLSWrap::SelectSNIContextCallback(SSL* s, int* ad, void* arg) {
   Local<Object> object = p->object();
   Local<Value> ctx;
 
+  // Set the servername as early as possible
+  Local<Object> owner = p->GetOwner();
+  if (!owner->Set(env->context(),
+                  env->servername_string(),
+                  OneByteString(env->isolate(), servername)).FromMaybe(false)) {
+    return SSL_TLSEXT_ERR_NOACK;
+  }
+
   if (!object->Get(env->context(), env->sni_context_string()).ToLocal(&ctx))
     return SSL_TLSEXT_ERR_NOACK;
 
diff --git a/test/parallel/test-tls-sni-servername.js b/test/parallel/test-tls-sni-servername.js
new file mode 100644
index 00000000000000..2c5785df5426c9
--- /dev/null
+++ b/test/parallel/test-tls-sni-servername.js
@@ -0,0 +1,56 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const tls = require('tls');
+
+// We could get the `tlsSocket.servername` even if the event of "tlsClientError"
+// is emitted.
+
+const serverOptions = {
+  requestCert: true,
+  rejectUnauthorized: false,
+  SNICallback: function(servername, callback) {
+    if (servername === 'c.another.com') {
+      callback(null, {});
+    } else {
+      callback(new Error('Invalid SNI context'), null);
+    }
+  }
+};
+
+function test(options) {
+  const server = tls.createServer(serverOptions, common.mustNotCall());
+
+  server.on('tlsClientError', common.mustCall((err, socket) => {
+    assert.strictEqual(err.message, 'Invalid SNI context');
+    // The `servername` should match.
+    assert.strictEqual(socket.servername, options.servername);
+  }));
+
+  server.listen(0, () => {
+    options.port = server.address().port;
+    const client = tls.connect(options, common.mustNotCall());
+
+    client.on('error', common.mustCall((err) => {
+      assert.strictEqual(err.message, 'Client network socket' +
+      ' disconnected before secure TLS connection was established');
+    }));
+
+    client.on('close', common.mustCall(() => server.close()));
+  });
+}
+
+test({
+  port: undefined,
+  servername: 'c.another.com',
+  rejectUnauthorized: false
+});
+
+test({
+  port: undefined,
+  servername: 'c.wrong.com',
+  rejectUnauthorized: false
+});