Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

sitegui
Copy link
Contributor

@sitegui sitegui commented Apr 21, 2015

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

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
@@ -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 ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Fishrock123 Fishrock123 added the tls Issues and PRs related to the tls subsystem. label Apr 21, 2015
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap at 80 columns please :)

}, function () {
console.log('OK');
process.exit();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sitegui
Copy link
Contributor Author

sitegui commented Apr 22, 2015

Sorry for the style inconsistencies.

Can someone else confirm this patch fixes the problem?

@@ -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) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parens should be unnecessary here.

@silverwind
Copy link
Contributor

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
Copy link
Contributor

Otherwise, LGTM

@sitegui
Copy link
Contributor Author

sitegui commented Apr 23, 2015

Thanks for the suggestions @silverwind

Should I squash all fix commits into the initial one?

@brendanashworth
Copy link
Contributor

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

@silverwind
Copy link
Contributor

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

@brendanashworth
Copy link
Contributor

This LGTM too.

silverwind pushed a commit that referenced this pull request Apr 23, 2015
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
Copy link
Contributor

Thanks! Merged in a7d7463

@silverwind silverwind closed this Apr 23, 2015
@rvagg rvagg mentioned this pull request Apr 27, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Apr 29, 2015
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]>
@rvagg rvagg mentioned this pull request May 2, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 14, 2015
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants