SNICallback callback doesn't throw an error with invalid input

[coolaj86]

This should throw an error:

{ ...
, SNICallback: function (domainname, cb) {
    cb(null, { key: '...', cert: '...', ca: '...' });
  }
}

Instead it succeeds and then the browser barfs.

I was live coding and couldn't figure out in the 30 seconds I just had to wrap my object in require('crypto').createSecureContext(...), so I just used http. :-(

If it had thrown an error it would have made the whole world a better place.

[Fishrock123]

cc @indutny

[indutny]

Upon investigation, it turned out that it actually throws clientError on server. By default it results in destruction of the socket.

Do we actually want to throw and kill the application? I think - no.

cc @nodejs/collaborators @nodejs/crypto

[Trott]

@indutny Do I understand that this is not a bug in your opinion and can be closed? Or am I misreading?

[coolaj86]

I argue that if we're supplying an invalid object, that should fail right away. It has nothing to do with the client. It's a programmer error.

[indutny]

@Trott this is my opinion indeed.

[Trott]

@coolaj86 So somewhere around here, do some minimal and fast checking to confirm that context looks like it ought to (maybe something like instanceof SecureContext) and call cb() with an appropriate error if it fails that check?

Or do you expect it to throw and (probably) crash the server? (Not trolling, honest question, want to make sure I understand what you expect the behavior to be, you mention "throw an error" earlier, but I think you were just being imprecise.)

[jessetane]

I also ran into this and was surprised when my server did not emit an error. If your SNICallback is unable to produce the required credentials, then something important is almost certainly wrong with your program (or perhaps DNS), no?

I see that you can catch this by listening for 'tlsClientError' but agree with @coolaj86 that this doesn't seem related to the client.

[jessetane]

To be clear, in my case the SNICallback was actually passing an error to its callback, not just passing an invalid context. Definitely would have expected my program to crash in that case.

[coolaj86]

@Trott I like the first idea that you suggest with doing some minimal checking and "throw an error" by calling a callback with an error (or throwing if no error handler exists).

[Trott]

It seems like perhaps this should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. Here's my reasoning:

• Throwing is almost certainly out of the question. This is an error that will be discovered asynchronously. We don't know that you have an invalid context until you're in the land of callbacks. Throw and you crash the whole server. Emitting an error event and hanging up the socket is what we do now and seems to be the way to go.

• So the error emitted here is a TypeError (which seems correct) via the tlsClientError event on the server. That seems right to me. The docs say that error event "is emitted when an error occurs before a secure connection is established." That's what's happening here. A case could be made that the event should be named differently, but that's a separate issue, I think.

[coolaj86]

Could we have the default behavior of unhandled SNICallback errors (or missing SNICallback) to be to print to the console instead of just being swallowed?

It's just one of those stupid things that you don't configure very often, but when you get it wrong, it's really non-obvious what's going on.

Getting this error in browsers is misleading, not exactly a clear "oh, I know how to fix that!"

This site can’t provide a secure connection

example.com uses an unsupported protocol.

ERR_SSL_VERSION_OR_CIPHER_MISMATCH

Unsupported protocol

The client and server don't support a common SSL protocol version or cipher suite.

[bnoordhuis]

Could we have the default behavior of unhandled SNICallback errors (or missing SNICallback) to be to print to the console instead of just being swallowed?

Unclear to me what "unhandled SNICallback errors" means. If you're talking about programmer bugs - passing an invalid context to the callback - the problem is that Node.js doesn't find out until much later.

We could do basic context sanity checks but that won't be exhaustive, it would only catch banal bugs. I'd also worry that a check might turn out too restrictive and close the door on legitimate uses.

[coolaj86]

Cases like this:

Undefined Callback

My code:

var oops;
tls.createServer({ SNICallback: oops });

Node stays silent.

The browser says:

example.com uses an unsupported protocol.

ERR_SSL_VERSION_OR_CIPHER_MISMATCH

Delima:

var crafty = {};
tls.createServer(crafty);

// this has valid use cases
crafty.SNICallback = validThingToDo;

Proposed solution:

If the first time that SNICallback should be called none of cert, key, nor SNICallback exist, log a warning message exactly that once and not again.

If the first call was successful, log every time that they are all missing. A developer at the least should implement function SNICallback(sni, cb) { cb(new Error("no certs")); }.

Throws an Error

My code:

var oops;
tls.createServer({ SNICallback: function (sni, cb) {
  // every once in a while there's a bad condition
  if (Math.round(Math.random() * 0.6)) {
    oops.toString();
  }
  getSomeContext(cb);
} });

Node stays silent.

The browser says:

TODO: what does the browser say in this case?

Delima:

If we knew that an error would happen every time, it would be logical to throw and kill the server. However, some errors happen only occasionally (perhaps a bad bot spoofing sni headers). We don't want to crash the server, especially not as new behavior.

Proposed solution:

Log each error with a stack indicating a specific file and line, and column number exactly once.

Bad TLS Context

My code:

tls.createServer({ SNICallback: function (sni, cb) {
  // oops, I forgot that I have to create a context, duh!
  cb(null, { key: 'valid key', cert: 'valid cert' });
} });

Node stays silent.

The browser says:

TODO: what does the browser say in this case?

Delima:

I passed an object, not an error. This means that, as far as I know, I've got valid certs but... oops, I did something wrong and have no way of knowing. Maybe this happens every time. Maybe it's something that only happens under certain conditions.

Proposed solution:

Log the error.

Unexpected Error

My code:

tls.createServer({ SNICallback: function (sni, cb) {
  if (Math.round(Math.random()) {
    getCertOrExpectedError(cb);
  } else {
    getCertOrUnexpectedError(cb);
  }
} });

Delima:

Sometimes I'm intentional about passing an error. Sometimes an error is happening somewhere deep, deep down and bubbling up. When my error is intentional, great. But when the error is coming from somewhere I don't expect, I need help.

Proposed solution:

Log errors just once (matched by file, line, col as mentioned before) that don't match a certain errno or code.

Maybe a built-in error like this:

tls.createServer({ SNICallback: function (sni, cb) {
  if (Math.round(Math.random()) {
    cb(new BuiltInSniError("I'm aware that I'm doing this")); // no logging
  } else {
    cb(new Error("I came from a deeply nested place")); // yes logging
  }
} });

Or a special property required like this:

tls.createServer({ SNICallback: function (sni, cb) {
  var err = new Error("bad news bears");
  err.sniCode = 'BADSNI';
  cb(err); // doesn't log, all is well... well, sort of
} });

Duplicate callback

My code:

tls.createServer({ SNICallback: function (sni, cb) {
  var oops = Math.round(Math.random());
  if (oops) {
    cb(new BuiltInSniError("I'm aware that I'm doing this"));
  }
  cb(null, { ... });
} });

Log that there was a duplicate callback.

Error Logging is Ugly!

Delima:

I don't want to see errors that I don't care about.

Proposed solution:

Just like unhandled Promises, let the user know in the error message that they should attach some handler(s) to catch the errors to prevent them from logging.

Maybe something like this?

conn.on('tlsSniError', cb);

conn.on('tlsContextError', cb);

conn.on('tlsDuplicateSniCbError', cb);

[Trott]

Related somewhat perhaps: #20969