doc: for `tls.createServer` options, providing `ctx` to `SNICallback` `cb` call is optionary.

[mkrawczuk]

📗 API Reference Docs Problem

• Version: v15.0.0-pre
• Platform: 4.15.0-108-generic from here Friday yeah Soma guys and 20 from K yeah #109-Ubuntu SMP Fri Jun 19 11:33:10 UTC 2020 x86_64 GNU/Linux
• Subsystem: TLS

Location

tls.createServer -> options -> SNICallback description
Affected URL(s):
https://nodejs.org/api/tls.html#tls_tls_createserver_options_secureconnectionlistener

Problem description

Turns out that ctx is not mandatory when calling cb passed to SNICallback, contrary to what the documentation states. It appears that cb falls back to the server's SecureContext if the second parameter (ctx) is not provided.

I assume this is a documentation bug since such behavior makes sense to me.

Below is a test that proves my point:

'use strict';
const common = require('../common');
if (!common.hasCrypto)
  common.skip('missing crypto');
const fixtures = require('../common/fixtures');
const assert = require('assert');
const tls = require('tls');
const { spawn } = require('child_process');

if (!common.opensslCli)
  common.skip('node compiled without OpenSSL CLI.');

const key = fixtures.readKey('rsa_private.pem');
const cert = fixtures.readKey('rsa_cert.crt');
const options = {
  key,
  cert,
  ca: [cert],
  requestCert: true,
  rejectUnauthorized: false,
  secureProtocol: 'TLS_method',
  // Ensure that the SNICallback is actually called.
  // Notice that 'cb' is called with only one parameter.
  SNICallback: common.mustCall((servername, cb) => cb(null)),
};

const server = tls.createServer(options, function(cleartext) {
  cleartext.on('error', function(er) {
    // We're ok with getting ECONNRESET in this test, but it's
    // timing-dependent, and thus unreliable. Any other errors
    // are just failures, though.
    if (er.code !== 'ECONNRESET')
      throw er;
  });
  cleartext.end('');
});

// Ensure that a secure connection has been estabilished even though here is
// no secure context passed to SNICallback's `cb` call.
server.on('secureConnection', common.mustCall());
// Ensure that a client error does not occur even though there is no secure
// context passed to SNICallback's `cb` call.
server.on('tlsClientError', common.mustNotCall());

server.listen(0, function() {
  const args = [
    's_client',
    '-tls1',
    '-connect', `localhost:${this.address().port}`,
    '-servername', 'hurrmm',
    '-key', fixtures.path('keys/rsa_private.pem'),
    '-cert', fixtures.path('keys/rsa_cert.crt'),
  ];

  function spawnClient() {
    const client = spawn(common.opensslCli, args, {
      stdio: [ 0, 1, 'pipe' ]
    });
    let err = '';
    client.stderr.setEncoding('utf8');
    client.stderr.on('data', function(chunk) {
      err += chunk;
    });

    client.on('exit', common.mustCall(function(code, signal) {
      if (code !== 0) {
        // If SmartOS and connection refused, then retry. See
        // https://github.com/nodejs/node/issues/2663.
        if (common.isSunOS && err.includes('Connection refused')) {
          requestCount = 0;
          spawnClient();
          return;
        }
        assert.fail(`code: ${code}, signal: ${signal}, output: ${err}`);
      }
      assert.strictEqual(code, 0);

      server.close();
    }));
  }

  spawnClient();
});

• I would like to work on this issue and submit a pull request.

[lpinca]

If you call the callback with no SecureContext why using it in the first place?

[mkrawczuk]

@lpinca If you are asking why is cb called inside SNICallback, then it is mandatory - transmission hangs otherwise (see doc).
If you are asking why would anyone register an SNICallback without providing a SecureContext to cb call - then one of the use cases might be for metrics. Someone might be interested in measuring which servernames are requested by clients.

[lpinca]

If you are asking why would anyone register an SNICallback without providing a SecureContext to cb call - then one of the use cases might be for metrics.

Yes this was my question.