Repository: mcollina/autocannon
Pull request #277 (2020-08-20 15:48:17)

Enable SNI servername by default
Default to sending the SNI servername when the `servername` option is not present or falsy. This behaviour matches browsers as well as popular tools like `curl` or `openssl s_client`.

Fixes: https://github.com/mcollina/autocannon/issues/233

silverwind: Looks like some extra logic is needed to filter out IP adresses.

````
(node:1530) [DEP0123] DeprecationWarning: Setting the TLS ServerName to an IP address is not permitted by RFC 6066. This will be ignored in a future version.
````

Also, I'm wondering if `''` vs `undefined` servername makes a difference. If the empty string results in the extension being disabled in node, it'd provide a convenient way to opt-out of SNI.
silverwind: Fixed that (imho questionable) deprecation by not inferring servername from IP addresses in the hostname. It'll still be possible to specify them explicitely as I don't want to prevent users doing that (IP adresses itself are not per-se invalid servernames).

Also fixed a bug related to variable closure in `startTlsServer` where one test case would see the `servername` of another.

Patch:
diff --git a/README.md b/README.md
index b048f60..0e7e203 100644
--- a/README.md
+++ b/README.md
@@ -109,7 +109,7 @@ Available options:
   -f/--forever
         Run the benchmark forever. Efficiently restarts the benchmark on completion. default: false.
   -s/--servername
-        Server name for the SNI (Server Name Indication) TLS extension.
+        Server name for the SNI (Server Name Indication) TLS extension. Defaults to the hostname of the URL when it is not an IP address.
   -x/--excludeErrorStats
         Exclude error statistics (non 2xx http responses) from the final latency and bytes per second averages. default: false.
   -E/--expectBody EXPECTED
@@ -254,7 +254,7 @@ Start autocannon against the given target.
        * `onResponse`: A `Function` you may provide to process the received response. It takes `status` (Number), `body` (String) and `context` (Object) parameters. _OPTIONAL_.
     * `idReplacement`: A `Boolean` which enables the replacement of `[<id>]` tags within the request body with a randomly generated ID, allowing for unique fields to be sent with requests. Check out [an example of programmatic usage](./samples/using-id-replacement.js) can be found in the samples. _OPTIONAL_ default: `false`
     * `forever`: A `Boolean` which allows you to setup an instance of autocannon that restarts indefinitely after emiting results with the `done` event. Useful for efficiently restarting your instance. To stop running forever, you must cause a `SIGINT` or call the `.stop()` function on your instance. _OPTIONAL_ default: `false`
-    * `servername`: A `String` identifying the server name for the SNI (Server Name Indication) TLS extension. _OPTIONAL_ default: `undefined`.
+    * `servername`: A `String` identifying the server name for the SNI (Server Name Indication) TLS extension. _OPTIONAL_ default: Defaults to the hostname of the URL when it is not an IP address.
     * `excludeErrorStats`: A `Boolean` which allows you to disable tracking non 2xx code responses in latency and bytes per second calculations. _OPTIONAL_ default: `false`.
     * `expectBody`: A `String` representing the expected response body. Each request whose response body is not equal to `expectBody`is counted in `mismatches`. If enabled, mismatches count towards bailout. _OPTIONAL_  
 * `cb`: The callback which is called on completion of a benchmark. Takes the following params. _OPTIONAL_.
diff --git a/help.txt b/help.txt
index da10543..499069a 100644
--- a/help.txt
+++ b/help.txt
@@ -74,7 +74,7 @@ Available options:
   -f/--forever
         Run the benchmark forever. Efficiently restarts the benchmark on completion. default: false.
   -s/--servername
-        Server name for the SNI (Server Name Indication) TLS extension.
+        Server name for the SNI (Server Name Indication) TLS extension. Defaults to the hostname of the URL when it is not an IP address.
   -x/--excludeErrorStats
         Exclude error statistics (non 2xx http responses) from the final latency and bytes per second averages. default: false.
   -E/--expectBody EXPECTED
diff --git a/lib/httpClient.js b/lib/httpClient.js
index 5bd2d94..74cb341 100644
--- a/lib/httpClient.js
+++ b/lib/httpClient.js
@@ -123,10 +123,17 @@ inherits(Client, EE)
 
 Client.prototype._connect = function () {
   if (this.secure) {
+    let servername
+    if (this.opts.servername) {
+      servername = this.opts.servername
+    } else if (!net.isIP(this.opts.hostname)) {
+      servername = this.opts.hostname
+    }
+
     if (this.ipc) {
       this.conn = tls.connect(this.opts.socketPath, { rejectUnauthorized: false })
     } else {
-      this.conn = tls.connect(this.opts.port, this.opts.hostname, { rejectUnauthorized: false, servername: this.opts.servername })
+      this.conn = tls.connect(this.opts.port, this.opts.hostname, { rejectUnauthorized: false, servername })
     }
   } else {
     if (this.ipc) {


Test patch:
diff --git a/test/helper.js b/test/helper.js
index 8b4361b..289a594 100644
--- a/test/helper.js
+++ b/test/helper.js
@@ -96,20 +96,11 @@ function startTlsServer () {
   const key = fs.readFileSync(path.join(__dirname, '/key.pem'))
   const cert = fs.readFileSync(path.join(__dirname, '/cert.pem'))
   const passphrase = 'test'
-  var servername = ''
 
   const options = {
     key,
     cert,
-    passphrase,
-    SNICallback: function (name, cb) {
-      servername = name
-      cb(null, tls.createSecureContext({
-        key,
-        cert,
-        passphrase
-      }))
-    }
+    passphrase
   }
 
   const server = tls.createServer(options, handle)
@@ -117,6 +108,7 @@ function startTlsServer () {
   server.listen(0)
 
   function handle (socket) {
+    const servername = socket.servername || ''
     socket.on('data', function (data) {
       // Assume this is a http get request and send back the servername in an otherwise empty reponse.
       socket.write('HTTP/1.1 200 OK\nX-servername: ' + servername + '\nContent-Length: 0\n\n')
diff --git a/test/httpClient.test.js b/test/httpClient.test.js
index f089b96..9015f93 100644
--- a/test/httpClient.test.js
+++ b/test/httpClient.test.js
@@ -96,6 +96,79 @@ test('client calls a tls server with SNI servername twice', (t) => {
   })
 })
 
+test('client uses SNI servername from URL hostname by default', (t) => {
+  t.plan(4)
+
+  var opts = tlsServer.address()
+  opts.protocol = 'https:'
+  opts.hostname = 'localhost'
+  const client = new Client(opts)
+  let count = 0
+
+  client.on('headers', (response) => {
+    t.equal(response.statusCode, 200, 'status code matches')
+    t.deepEqual(response.headers, ['X-servername', opts.hostname, 'Content-Length', '0'])
+    if (count++ > 0) {
+      client.destroy()
+    }
+  })
+})
+
+test('client prefers SNI servername from opts over URL hostname', (t) => {
+  t.plan(4)
+
+  var opts = tlsServer.address()
+  opts.protocol = 'https:'
+  opts.hostname = 'localhost'
+  opts.servername = 'example.com'
+  const client = new Client(opts)
+  let count = 0
+
+  client.on('headers', (response) => {
+    t.equal(response.statusCode, 200, 'status code matches')
+    t.deepEqual(response.headers, ['X-servername', opts.servername, 'Content-Length', '0'])
+    if (count++ > 0) {
+      client.destroy()
+    }
+  })
+})
+
+test('client ignores IP address in hostname-derived SNI servername', (t) => {
+  t.plan(4)
+
+  var opts = tlsServer.address()
+  opts.protocol = 'https:'
+  opts.hostname = opts.address
+  const client = new Client(opts)
+  let count = 0
+
+  client.on('headers', (response) => {
+    t.equal(response.statusCode, 200, 'status code matches')
+    t.deepEqual(response.headers, ['X-servername', '', 'Content-Length', '0'])
+    if (count++ > 0) {
+      client.destroy()
+    }
+  })
+})
+
+test('client ignores falsy SNI servername', (t) => {
+  t.plan(4)
+
+  var opts = tlsServer.address()
+  opts.protocol = 'https:'
+  opts.servername = ''
+  const client = new Client(opts)
+  let count = 0
+
+  client.on('headers', (response) => {
+    t.equal(response.statusCode, 200, 'status code matches')
+    t.deepEqual(response.headers, ['X-servername', '', 'Content-Length', '0'])
+    if (count++ > 0) {
+      client.destroy()
+    }
+  })
+})
+
 test('http client automatically reconnects', (t) => {
   t.plan(4)