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

undefined is passed as hostname to tls.checkServerIdentity #1489

Closed
sitegui opened this issue Apr 21, 2015 · 10 comments
Closed

undefined is passed as hostname to tls.checkServerIdentity #1489

sitegui opened this issue Apr 21, 2015 · 10 comments
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.

Comments

@sitegui
Copy link
Contributor

sitegui commented Apr 21, 2015

Hi all,

When using tls.connect(options) with no options.host I get this:

Hostname/IP doesn't match certificate's altnames: "Host: undefined. is not cert's CN: localhost"

I tried to track this and it seems tls.checkServerIdentity get called with hostname as undefined.

Examining the stack, it gets called by _tls_wrap.js:913 and hostname is defined in _tls_wrap.js:859.

As you can see, hostname is undefined because neither options.servername, options.host nor options.socket are set.

The tls.connect doc says: "If host is omitted, it defaults to localhost", so shouldn't it actually set options.host to "localhost" in this case?

Adding it to defaults may solve this, but I have very little understanding of this part of the code to be sure.

Is it a bug or intended?

Best regards

@sitegui
Copy link
Contributor Author

sitegui commented Apr 21, 2015

Code's better than words:

'use strict'

var tls = require('tls'),
    https = require('https')

https.createServer({
    key: key(),
    cert: cert()
}).listen(8000)

tls.connect({
    port: 8000,
    ca: cert(),
    //host: 'localhost' // <-- comment out and it will work!
}, function () {
    console.log('OK')
    process.exit()
})

function key() {
    return '-----BEGIN RSA PRIVATE KEY-----\n' +
        'MIIEowIBAAKCAQEA5m2YXNXrx4ADNNvwlMai26n5v2xVYnAPQzXZbeBD5JxaHLYB\n' +
        'RqZin/rVKNH209l3pwlgcvZ9jLIdAlyAg13gXlNCcPg2RgEjhFg0t0cf9bYizjnd\n' +
        '4LCIhwDR8fCfx8B3VWoJRmD8dl2kZuemIGzx6Zr/AVi5vpEQOSMB76m2UZZMeIKz\n' +
        'Il/uqkGVGuC83NL1/FGw4v9XqRG3/NxXmNzdRPe1D4vjn6sRoFEQ7qPL0qjmhwWZ\n' +
        'lCKW9ZglwSlochMBEnwue2eKQM87vx4MpujQksVZYd+h7vvVWTUFE06PNCFlNcSU\n' +
        'MgwasDFPqwHDb2XnMkpo+AFjCLdLQJ5qZpe9YQIDAQABAoIBAQC2UK5Ffahgn4Np\n' +
        '9j8Cp6tBW9pTv5ZLHVimF9whmFh/b8nIf6TAznKoG2E+O+osMhr+mWerbiVmBaL4\n' +
        'NFImHkegugWOtoTSnKIKW3PSMz8xPNuLCbPozCQplNeHspfpBvokJZKTEbeOu4aR\n' +
        'OOVzMF+zMkRjP10vTz1jx7QHeOLc5yrhv9PDR348BLMWRYQViqHR5WfKJ+vCyDdW\n' +
        'nPDgaCiu19xZr4tEocRFJ8XtL1r9171nVjAgQXEPBDU65vTfxWrm9AXNNf8fOzb0\n' +
        'UBP0QkS1IS1ryum9g7XD5MItz5WswU5EU4CZfaYMofHNcOW0baHD/UV+MWkJauYn\n' +
        'QZMe7L6FAoGBAPNKMpkutZUVrtJQZGnz0tw8tCtC2NjTCyFGp5o6StU5gP5U65c0\n' +
        'hgADlxFM7U5J+3zkvMCEwxZPhVQspaAaqkxVvKZ6Oc/oRbH24644bRM2+jsbOu/m\n' +
        '8SZCL+7k6lTkIXeWeFVEbHi68I/HI2Vsa/PfAQxDnklLt8g1hOJwd1JDAoGBAPJ3\n' +
        'YebboXSuoZslj6F/YP951tAX0MyAga2hWwThcO7gMqQZUGXVYTUQwGbIBdjtYmNk\n' +
        'pI0OoneHG2w2YSK10som/lYjsIIfLVka9nBceNJylpmuo+rtuaGb3OxrxRcJJL0W\n' +
        'jZKwoVaT9/2IMb+BrcGcfN9s1ImhziOEFAEz3/GLAoGASQcEmSaEKvQPPeITwhoG\n' +
        'OUWfbzzpimwO8zYaKRlGTSqtpaon7YM+ldJ+DhthQBbE/oBKiB9Rz+iexN2B+cUH\n' +
        'SVKTBgW6RMYb5YeOYEVfuFzQT92km05fJHTJnpPoIwM3aIYqKK4ZQUQb4YyM+2zI\n' +
        'GrPdxKinYqjvyZEHClFn/7ECgYBxpKzZZGW3Z8ZNDnzUh/xxoayiWhc+Upj1RaSA\n' +
        'lB23iJOTwF2jbTCji5dyVRwQgarUxS4vAwX5GfUrcg1zFF+Y6k/ZFd88DdrWYcHS\n' +
        'BjWHBbg6jdU8XnHcIk6Y7SYyVtHGYpS2hV0JVE8uoLAYf3JuRadtnPe9Dn6svNIX\n' +
        'gjXbYwKBgDnz3Vg2+MwA7d+DaJ8nazOZlYJVBfLdF9E5MoKg+CFrjbmQm3vWLlg1\n' +
        'twjP8r56/GZuPQ/NHWygX5BXAw+7xCasD5g6Ek6wDVdYzlfE9xZ5H35TP39mYY8R\n' +
        'Vyi2jyjRXvxa9BDSVTPLbxk/KZqpHQOuvvLQ/zLCBUWhBl8pC9I2\n' +
        '-----END RSA PRIVATE KEY-----\n'
}

function cert() {
    return '-----BEGIN CERTIFICATE-----\n' +
        'MIIDNDCCAhwCCQDIfMNgGGDxHjANBgkqhkiG9w0BAQsFADBcMQswCQYDVQQGEwJC\n' +
        'UjESMBAGA1UECBMJU2FvIFBhdWxvMREwDwYDVQQHEwhDYW1waW5hczESMBAGA1UE\n' +
        'ChMJbG9jYWxob3N0MRIwEAYDVQQDEwlsb2NhbGhvc3QwHhcNMTUwNDIxMDkzOTM5\n' +
        'WhcNMjUwNDE4MDkzOTM5WjBcMQswCQYDVQQGEwJCUjESMBAGA1UECBMJU2FvIFBh\n' +
        'dWxvMREwDwYDVQQHEwhDYW1waW5hczESMBAGA1UEChMJbG9jYWxob3N0MRIwEAYD\n' +
        'VQQDEwlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDm\n' +
        'bZhc1evHgAM02/CUxqLbqfm/bFVicA9DNdlt4EPknFoctgFGpmKf+tUo0fbT2Xen\n' +
        'CWBy9n2Msh0CXICDXeBeU0Jw+DZGASOEWDS3Rx/1tiLOOd3gsIiHANHx8J/HwHdV\n' +
        'aglGYPx2XaRm56YgbPHpmv8BWLm+kRA5IwHvqbZRlkx4grMiX+6qQZUa4Lzc0vX8\n' +
        'UbDi/1epEbf83FeY3N1E97UPi+OfqxGgURDuo8vSqOaHBZmUIpb1mCXBKWhyEwES\n' +
        'fC57Z4pAzzu/Hgym6NCSxVlh36Hu+9VZNQUTTo80IWU1xJQyDBqwMU+rAcNvZecy\n' +
        'Smj4AWMIt0tAnmpml71hAgMBAAEwDQYJKoZIhvcNAQELBQADggEBAI4GtZhFqSn/\n' +
        'nZpwWFBAfLPqjo8gvhDuqDsn9HrPrlIiei2ZhMTSwZy34StRD3NRdzQe+KgZNI/c\n' +
        'MQlA6+8KDoWPlXL1Wk730WYfCHmiBtc8hZNCIPcxOukJByPJzxIojSt1Fg4RDrKk\n' +
        'qx4tUb8MHN0l+33GobwSPSAHWbOHiITKxv0lOoAtfXyArXx++K9THbohmdchSSa5\n' +
        'zIfYqp7pWaBgQRCpOyAWIHD/PSYmoIW2dbrr50W5xXQWJNeX4VdrkAjNvGBniBEV\n' +
        'wlig5Vq2YvkaGDo2fGVc++7mnkQl41aEmJ2M1OJYLWke6MOMu535VOmy0TEZu5lw\n' +
        'zIHV0FIGeGE=\n' +
        '-----END CERTIFICATE-----\n'
}

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Apr 21, 2015
@silverwind silverwind added the confirmed-bug Issues with confirmed bugs. label Apr 21, 2015
@Fishrock123
Copy link
Contributor

-----BEGIN RSA PRIVATE KEY-----

Do note this is now public. :)

Hmm, so, confirmed from that test case, and this patch appears to fix it:

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 3e091b0..c1037a7 100644
--- a/lib/_tls_wrap.js
+++ b/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) ||
+                 'localhost',
       NPN = {},
       context = tls.createSecureContext(options);
   tls.convertNPNProtocols(options.NPNProtocols, NPN);

However, our tests don't appear to specify a host and yet they don't have a problem? I'm kinda confused.

@silverwind
Copy link
Contributor

That cert is self-signed for localhost, so no issue :)

$ openssl s_client -connect localhost:8000
CONNECTED(00000003)
depth=0 /C=BR/ST=Sao Paulo/L=Campinas/O=localhost/CN=localhost
verify error:num=18:self signed certificate
verify return:1
depth=0 /C=BR/ST=Sao Paulo/L=Campinas/O=localhost/CN=localhost
verify return:1

I think the suggested fix is spot-on. @sitegui would you like to submit a PR with this patch and your test case added?

@silverwind
Copy link
Contributor

@Fishrock123's patch looks to be safer than adding it to defaults.

@Fishrock123
Copy link
Contributor

I think the suggested fix is spot-on. Guilherme Souza would you like to submit a PR with this patch and your test case added?

Again, see the above, not so sure.

For the test, I suggest just using the built in certs that are read from fs in other tests.

@silverwind
Copy link
Contributor

Yeah, something is a bit fishy here, it's strange that tls.connect does throw in this case, when the docs for the secureConnect event say

This event is emitted after a new connection has been successfully handshaked. The listener will be called no matter if the server's certificate was authorized or not.

Above self-signed cert seems to be authorized by the ca option, I'm not sure the throw is correct in this case.

cc: @indutny

@silverwind
Copy link
Contributor

@Fishrock123 Regarding existing tests: They mostly use rejectUnauthorized: false which suppresses the cert errors. test-tls-connect being the exception, it connects to an nonexistant server.

Still, the fact that tls.connect does throw on cert errors puzzles me, maybe the docs aren't up to date?

@silverwind
Copy link
Contributor

Nevermind my error ramblings, I was just confused about how errors were returned here.

The test_cert.pem our tests use are already signed to localhost, so they're fine to test this with.

@sitegui
Copy link
Contributor Author

sitegui commented Apr 21, 2015

Thanks for the quick follow up guys.

@Fishrock123

Do note this is now public. :)

It was already, no problem.

@silverwind

@sitegui would you like to submit a PR with this patch and your test case added?

Yes, I'll do this is a moment.

@silverwind

@Fishrock123's patch looks to be safer than adding it to defaults

It seems a better solution indeed, since host must be present only if neither servername nor socket are set.

silverwind pushed a commit that referenced this issue 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 <brendan.ashworth@me.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind
Copy link
Contributor

Fixed in a7d7463

Fishrock123 pushed a commit to Fishrock123/node that referenced this issue 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 <brendan.ashworth@me.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue 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 <brendan.ashworth@me.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants