tls_wrap: use localhost if options.host is empty #1493

sitegui · 2015-04-21T21:39:32Z

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

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 ;)

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) See #1489

Fishrock123 · 2015-04-21T21:40:35Z

lib/_tls_wrap.js

@@ -858,7 +858,8 @@ exports.connect = function(/* [port, host], options, cb */) {

  var hostname = options.servername ||
                 options.host ||
-                 options.socket && options.socket._host,
+                 options.socket && options.socket._host ||


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

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

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

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?

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

Fishrock123 · 2015-04-21T21:41:24Z

test/parallel/test-tls-connect-no-host.js

+    ca: cert,
+    // No host set here. 'localhost' is the default,
+    // but tls.checkServerIdentity() breaks before the fix with:
+    // Error: Hostname/IP doesn't match certificate's altnames: "Host: undefined. is not cert's CN: localhost"


wrap at 80 columns please :)

brendanashworth · 2015-04-22T01:30:55Z

test/parallel/test-tls-connect-no-host.js

+}, function () {
+    console.log('OK');
+    process.exit();
+});


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

sitegui · 2015-04-22T02:45:10Z

Sorry for the style inconsistencies.

Can someone else confirm this patch fixes the problem?

chrisdickinson · 2015-04-22T04:08:29Z

lib/_tls_wrap.js

@@ -858,7 +858,8 @@ exports.connect = function(/* [port, host], options, cb */) {

  var hostname = options.servername ||
                 options.host ||
-                 options.socket && options.socket._host,
+                 (options.socket && options.socket._host) ||


Parens should be unnecessary here.

silverwind · 2015-04-22T16:49:48Z

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 · 2015-04-22T16:50:25Z

Otherwise, LGTM

sitegui · 2015-04-23T00:10:20Z

Thanks for the suggestions @silverwind

Should I squash all fix commits into the initial one?

brendanashworth · 2015-04-23T00:46:37Z

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

silverwind · 2015-04-23T04:26:49Z

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

brendanashworth · 2015-04-23T05:07:57Z

This LGTM too.

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) PR-URL: #1493 Fixes: #1489 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

silverwind · 2015-04-23T14:54:54Z

Thanks! Merged in a7d7463

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) PR-URL: nodejs#1493 Fixes: nodejs#1489 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

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) PR-URL: nodejs#1493 PORT-PR-URL: nodejs#1560 PORT-FROM: v2.x / a7d7463 Fixes: nodejs#1489 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

Fishrock123 reviewed Apr 21, 2015
View reviewed changes

Fishrock123 added the tls Issues and PRs related to the tls subsystem. label Apr 21, 2015

Fishrock123 reviewed Apr 21, 2015
View reviewed changes

sitegui added 2 commits April 21, 2015 18:41

Silly fix for PR1493

51f90e8

Wrap at 80 columns

c3c8d6e

brendanashworth reviewed Apr 22, 2015
View reviewed changes

Style fix

e5c93eb

chrisdickinson reviewed Apr 22, 2015
View reviewed changes

Make test quiet

2d7f52c

silverwind closed this Apr 23, 2015

rvagg mentioned this pull request Apr 27, 2015

Release proposal: v2.0.0 #1532

Closed

rvagg mentioned this pull request May 2, 2015

doc: update AUTHORS list #1586

Merged

Fishrock123 mentioned this pull request May 14, 2015

Release proposal: v1.8.2 #1703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls_wrap: use localhost if options.host is empty #1493

tls_wrap: use localhost if options.host is empty #1493

sitegui commented Apr 21, 2015

Fishrock123 Apr 21, 2015

sitegui Apr 21, 2015

kkoopa Apr 21, 2015

sitegui Apr 22, 2015

silverwind Apr 22, 2015

Fishrock123 Apr 21, 2015

brendanashworth Apr 22, 2015

sitegui commented Apr 22, 2015

chrisdickinson Apr 22, 2015

silverwind commented Apr 22, 2015

silverwind commented Apr 22, 2015

sitegui commented Apr 23, 2015

brendanashworth commented Apr 23, 2015

silverwind commented Apr 23, 2015

brendanashworth commented Apr 23, 2015

silverwind commented Apr 23, 2015

tls_wrap: use localhost if options.host is empty #1493

tls_wrap: use localhost if options.host is empty #1493

Conversation

sitegui commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sitegui commented Apr 22, 2015

Choose a reason for hiding this comment

silverwind commented Apr 22, 2015

silverwind commented Apr 22, 2015

sitegui commented Apr 23, 2015

brendanashworth commented Apr 23, 2015

silverwind commented Apr 23, 2015

brendanashworth commented Apr 23, 2015

silverwind commented Apr 23, 2015