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