tls_wrap: use localhost if options.host is empty

[sitegui]

That's my PR for #1489. Thanks @Fishrock123 for the actual fix idea

cc: @silverwind @Fishrock123

tls.connect(options) with no options.host should accept a certificate
with CN: 'localhost'. Fix Error: Hostname/IP doesn't match
certificate's altnames: "Host: undefined. is not cert's CN: localhost"

'localhost' is not added directly to defaults because that is not
always desired (for example, when using options.socket)

This is my first PR here, I would really appreciate a careful review ;)

[Fishrock123]

Don't these need to be in parenthesis? Otherwise it will overlap, no?

[sitegui]

I don't think they will overlap, but it's much better with () for sure

[kkoopa]

No, Boolean logic defines and at higher precedence than or. So does JavaScript. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

[sitegui]

It is unnecessary, as @kkoopa said: && has higher precedence. I just added the ( ) to make the intent clearer to the eye, since the precendence is not obvious.

What's the recommended style for this situation?

[silverwind]

Generally we prefer shorter I think, but no harm in adding a few clarity parens, I'd say.

[Fishrock123]

wrap at 80 columns please :)

[brendanashworth]

Could you add a terminating newline here so we don't get annoying diff changes later?

[sitegui]

Sorry for the style inconsistencies.

Can someone else confirm this patch fixes the problem?

[chrisdickinson]

Parens should be unnecessary here.

[silverwind]

The patch works, but I'd prefer the test itself to be silent on success, I'd suggest this change:

diff --git a/test/parallel/test-tls-connect-no-host.js b/test/parallel/test-tls-connect-no-host.js
index 1740b24..41aac1a 100644
--- a/test/parallel/test-tls-connect-no-host.js
+++ b/test/parallel/test-tls-connect-no-host.js
@@ -6,6 +6,7 @@ if (!common.hasCrypto) {
 }
 var tls = require('tls');

+var assert = require('assert');
 var fs = require('fs');
 var path = require('path');

@@ -20,7 +21,7 @@ tls.createServer({
   cert: cert
 }).listen(common.PORT);

-tls.connect({
+var socket = tls.connect({
     port: common.PORT,
     ca: cert,
     // No host set here. 'localhost' is the default,
@@ -28,6 +29,6 @@ tls.connect({
     // Error: Hostname/IP doesn't match certificate's altnames:
     //   "Host: undefined. is not cert's CN: localhost"
 }, function() {
-    console.log('OK');
+    assert(socket.authorized);
     process.exit();
 });

[silverwind]

Otherwise, LGTM

[sitegui]

Thanks for the suggestions @silverwind

Should I squash all fix commits into the initial one?

[brendanashworth]

You don't have to worry about that, a collaborator will do it when they
merge it.

[silverwind]

I'll do it for you in this case, everything looks fine. Will merge later today.

[brendanashworth]

This LGTM too.

[silverwind]

Thanks! Merged in a7d7463