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: newSession breaks with Safari and curl on OSX (but not most other clients) #7821

Closed
risacher opened this issue Jul 21, 2016 · 12 comments
Closed
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@risacher
Copy link

Version: v6.3.0
Platform: Linux 3.13.0-83-generic #127-Ubuntu SMP Fri Mar 11 00:25:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
Subsystem: TLS

I used the sample code from https://strongloop.com/strongblog/improve-the-performance-of-the-node-js-https-server/ to implement simple TLS sessions:

var tlsSessionStore = {};
httpsServer.on('newSession', function(id, data) {
    console.log('NEW HTTPS session - %s', id.toString('hex'));
    tlsSessionStore[id] = data;
});
httpsServer.on('resumeSession', function(id, cb) {
    console.log('RESUME HTTPS session - %s', id.toString('hex'));
    cb(null, tlsSessionStore[id] || null);
});

I found that this would cause Safari 9.1.1 to hang during TLS handshaking

curl 7.43.0 (x86_64-apple-darwin14.0) libcurl/7.43.0 SecureTransport zlib/1.2.5. hangs as soon as it connects.

curl 7.35.0 (x86_64-pc-linux-gnu) libcurl/7.35.0 OpenSSL/1.0.1f zlib/1.2.8 libidn/1.28 librtmp/2.3 seems to complete handshaking and then hangs.

If I remove the event handlers for newSession and resumeSession, this behavior goes away.

@risacher
Copy link
Author

Oh, and works with Firefox and Chrome either way.

I also had problems with IE11 on Win7 (but not Win8 or Win10) - but I'm not sure that's the same bug yet.

@risacher risacher changed the title TLS: newSession breaks with Safari and curl on IOS (but not most other clients) TLS: newSession breaks with Safari and curl on OSX (but not most other clients) Jul 21, 2016
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Jul 21, 2016
@mscdex
Copy link
Contributor

mscdex commented Jul 21, 2016

/cc @nodejs/crypto

@indutny
Copy link
Member

indutny commented Jul 21, 2016

Thank you for opening this issue! May I ask you to share a reduced test case that will reproduce this problem?

@risacher
Copy link
Author

See attached. I note that I've observed the problem with both node v6.3.0 and v5.12.0

If it helps, I can spin up a ec2 server with this running to poke against.

minimal_server.js.txt

@risacher
Copy link
Author

risacher commented Jul 22, 2016

For testing purposes, https://test.risacher.org/ is up running the minimal server with a self-signed cert on node v6.3.0.

I observe that Safari gets the cert, because it complains about the trust chain, but then it hangs after I agree to an exception for the cert.

Curl-on-OSX, OTOH, looks like this:

$ curl -verbose --insecure https://test.risacher.org
* Rebuilt URL to: https://test.risacher.org/
*   Trying 54.152.201.144...
* Connected to test.risacher.org (54.152.201.144) port 443 (#0)
^C

Compared to Curl-on-Ubuntu:

$ curl -verbose --insecure https://test.risacher.org/
* Hostname was NOT found in DNS cache
*   Trying 54.152.201.144...
* Connected to test.risacher.org (54.152.201.144) port 443 (#0)
* successfully set certificate verify locations:
*   CAfile: none
  CApath: /etc/ssl/certs
* SSLv3, TLS handshake, Client hello (1):
* SSLv3, TLS handshake, Server hello (2):
* SSLv3, TLS handshake, CERT (11):
* SSLv3, TLS handshake, Server key exchange (12):
* SSLv3, TLS handshake, Server finished (14):
* SSLv3, TLS handshake, Client key exchange (16):
* SSLv3, TLS change cipher, Client hello (1):
* SSLv3, TLS handshake, Finished (20):
^C

@risacher
Copy link
Author

Oh, and I originally saw the problem with a "real" cert, so I don't think the self-signed cert is a factor.

@risacher
Copy link
Author

Just verified that IE11 on Windows 7 also will not connect. (Win 8 and later work okay.)

@indutny
Copy link
Member

indutny commented Jul 22, 2016

@risacher I believe that this happens because of the missing callback invocation for newSession event. Please try changing code to:

httpsServer.on('newSession', function(id, data, cb) {
    console.log('NEW HTTPS session - %s', id.toString('hex'));
    tlsSessionStore[id] = data;
    cb(null);
});

@indutny
Copy link
Member

indutny commented Jul 22, 2016

As per docs, newSession passes cb as a third argument.

@risacher
Copy link
Author

@indutny, your suggested change fixes IE11-on-Win7, curl-on-linux, and curl-on-osx.
Probably Safari, too, but I can't test that ATM.
I will close issue as soon as test Safari, but I think that's it.

My code - and all the examples I could find online - were from 2013, before that callback existed.

Thank you for your help.

@indutny
Copy link
Member

indutny commented Jul 22, 2016

No problem at all, glad that we figured it out! :)

@risacher
Copy link
Author

Yup. Adding callback fixes Safari too.

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

No branches or pull requests

3 participants