tls: server.addContext does not update the context if there was one registered for a given servername wildcard.

[mkrawczuk]

• Version: v15.0.0-pre (node master branch)
• Platform: irrelevant
• Subsystem: TLS

What steps will reproduce the bug?

I am having a hard time writing a test that showcases this behavior. Will try to deliver tomorrow. Meanwhile, an explaination with code snippets so I don't forget until morning:

in lib/_tls_wrap.js, the definition of server.addContext is pretty concise:

Server.prototype.addContext = function(servername, context) {
  if (!servername) {
    throw new ERR_TLS_REQUIRED_SERVER_NAME();
  }

  const re = new RegExp('^' +
                      servername.replace(/([.^$+?\-\\[\]{}])/g, '\\$1')
                                .replace(/\*/g, '[^.]*') +
                      '$');
  this._contexts.push([re, tls.createSecureContext(context).context]);
};

As we can see, the new context is pushed to this._contexts array.
Then, a few lines below, the definition of SNICallback, a default SNICallback used when no custom SNICallback is provided for the server:

function SNICallback(servername, callback) {
  const contexts = this.server._contexts;

  for (const elem of contexts) {
    if (elem[0].test(servername)) {
      callback(null, elem[1]);
      return;
    }
  }

  callback(null, undefined);
}

We can observe that always the first, 'oldest' matching context will be used, no matter if a new context has been added for a matching servername.
This might be a security threat - the API user might be expecting the updated SecureContext to be used for a given servername after server.addContext call, but it is not true, because the 'oldest' SecureContext is chosen.

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Probably the iteration in SNICallback should be reversed to use the most recent matching SecureContext. Using a dictionary or updating values for already existing wildcards would be a little tricky, but maybe possible.
Proposed solution would then be:

function SNICallback(servername, callback) {
  const contexts = this.server._contexts.reverse(); // reverse the contexts
  // iterate latest first
  for (const elem of contexts) {
    if (elem[0].test(servername)) {
      callback(null, elem[1]);
      return;
    }
  }

  callback(null, undefined);
}

What do you see instead?

The first added SecureContext that matches servername is used.

Additional information

servername should also be case-insensitive.

I am working on it and should soon open a PR with a proposed fix. Please let me know if you agree on the reversed iteration change.