Reusing TLS sessions causes to socket.servername being false
#28985
Comments
9 tasks
|
I can't reproduce this with node master, v12.7.0, or v8.x. I can only reproduce it with node v10.x. Also, the runkit link is using node v10.16.0. |
|
On my machine the example shows |
|
Do: Here's what I get every time: |
|
The issue still occurs on |
const http2 = require('http2');
const session = http2.connect('https://google.com', {servername: 'google.com'});
session.socket.once('secureConnect', () => {
const tlsSession = session.socket.getSession();
session.close();
session.once('close', () => {
const secondSession = http2.connect('https://google.com', {servername: 'google.com', session: tlsSession});
secondSession.socket.once('secureConnect', () => {
console.log(secondSession.socket.servername);
});
secondSession.once('localSettings', () => {
console.log(secondSession.originSet);
});
});
});Very rarely shows |
|
Any news on this? |
|
Hello? Anybody there? It's been a month since the creation of this issue. The bug breaks the whole |
|
/cc @nodejs/crypto |
|
I suspect it's this line that's at fault: node/lib/internal/http2/core.js Line 560 in e585caa It should probably be something like if (typeof socket.servername === 'string') {
I'm changing the labels because it's not a tls bug: the servername doesn't need to be present, it's optionally sent by the client as an extension in the ClientHello. The http2 code should handle that. |
bnoordhuis
added
http2
|
@bnoordhuis That would case to |
|
@bnoordhuis I think if it hasn't received the servername yet, it should show the requested authority in the |
|
Any updates on this? It's been over two months already. |
|
@nodejs/http2 |
|
I don’t understand what is the problem here, and what it is causing to the end user. There are some mentioning of tls, and then http2. Can you please explain what is the problem here? (As a side note, you seems to have investigated this problem in great detail.. so you might be best equipped to send a PR to fix it). |
|
Okay, I can reproduce it every time now (v12.10.0). On v11.15.0 it works as expected tho: https://runkit.com/szmarczak/5d7e73e9896e0f001a92c20d
The problem here is an invalid
Almost. I have got to the source of it, but haven't figured anything out to fix it yet. (maybe it's a regression? haven't checked that) |
|
It's definitely a regression (? or maybe it's the correct behavior now): after a second: |
|
@szmarczak This seems to reproduce on RunKit for me every time, but never locally… do you have any idea why? I wouldn’t really know how to start debugging there, unfortunately… |
|
If I had to venture a guess it's because runkit.com does some kind of reverse-proxying over HTTPS and doesn't forward the servername extension. |
Nah. The same code if I run locally also gives me
@addaleax Are you running Node v12.10.0? I'll debug it today and let you know anyway. |
|
@szmarczak Yes, I’m on x64 Linux too, and all of Node v12.10.0, v11.15.0 and master give the same result, with and without timeout. :/ |
|
@szmarczak Can you run the failing code locally with |
|
Also, just curious, does pinning the TLS version via |
Yes, actually it works as expected with TLS 1.2. I guess the problem is not in Node itself but in TLS 1.3 or something has changed with TLS 1.3 and Node hasn't updated that yet. |
This comment has been minimized.
This comment has been minimized.
|
I ran https://runkit.com/szmarczak/5d7e73e9896e0f001a92c20d locally and here's the results: macOS 10.14.6 |
|
I'm watching this, but haven't had time to try to debug it. Is it possible that TLS 1.3 servername messages come across with different timing than TLS1.2, that http/2 depends on the TLS1.2 timing? |
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Synchronised ( |
|
So I tested it on Node 11 with TLS 1.3, and the issue still exists. |
|
|
|
I have analyzed the source code and I figured out that's not possible to fail. So I went through openssl issues and found this: Yeah, you were right, it's an openssl issue. |
|
@szmarczak Okay, thanks for doing all the digging here. I’ll take a deeper look at the openssl source. |
|
From what I can tell, Stack trace
|
|
@szmarczak It looks like the How did you generate those Buffers? That might be useful to know. Generally, I can see a few ways that this goes forward:
I don’t really know how TLS v1.3 comes into play here. The SSL version stored in your |
@addaleax I just simply connect, save the session buffer, close the connection and reconnect: https://runkit.com/szmarczak/5d851a78b56e55001a047b7d
👍
👍
👍
And it started to also not work even if using TLS v1.2. It's getting weirder and weirder. |
|
I have come to the conclusion that this involves some kind of witchcraft – the same test file that would reproduce the issue consistently for me yesterday does not do that today. (My best guess would be that behaviour varies depending on what remote host exactly is being reached.) That being said, @szmarczak, could you try to apply this diff to Node.js master and verify that it doesn’t work before and works after, since you seem to be the person who has the easiest time reproducing this consistently? diff --git a/deps/openssl/openssl/ssl/ssl_lib.c b/deps/openssl/openssl/ssl/ssl_lib.c
index f559bc10eff4..0aa4ceb954f2 100644
--- a/deps/openssl/openssl/ssl/ssl_lib.c
+++ b/deps/openssl/openssl/ssl/ssl_lib.c
@@ -2630,7 +2630,7 @@ const char *SSL_get_servername(const SSL *s, const int type)
* call the relevant callbacks for such resumption flows, and callbacks
* might error out if there is not a SNI value available.
*/
- if (s->hit)
+ if (s->hit && s->session->ext.hostname != NULL)
return s->session->ext.hostname;
return s->ext.hostname;
} |
szm@solus ~/Desktop $ node -v
v12.10.0
szm@solus ~/Desktop $ ./node -v
v13.0.0-pre
szm@solus ~/Desktop $ node bug.js
false
szm@solus ~/Desktop $ ./node bug.js
google.comI confirm your patch fixes the bug. 😃 |
addaleax
added a commit
to addaleax/openssl
that referenced
this issue
Sep 21, 2019
In `SSL_get_servername()`, fall back to returning the hostname set earlier via `SSL_set_tlsext_host_name()` when the resumed session data does not include a host name field. Refs: openssl#8822 Fixes: nodejs/node#28985 CLA: Trivial
1 task
|
I’ve opened a PR against OpenSSL, let’s see where that leads us. Even if it doesn’t land we can easily work around that in Node.js, but I’m not sure if the approach is okay as-is. |
|
It is interesting to note in all of the problematic traces given above the server_name extension that exists in the ClientHello is not acknowledged by the server in the ServerHello/EncryptedExtensions which concurs with my expectations from my comments in openssl/openssl#9964 |
|
@addaleax @szmarczak Could you please explain a bit how badly this impairs HTTP/2 support? Does this ruin the HTTP/2 benefits of parallel requests? Does this break HTTP/2 completely? |
|
@o2genum It breaks the |
addaleax
added
blocked
|
Marking this as blocked until openssl/openssl#10018 is merged |
|
openssl/openssl#10018 is now merged. |
|
@addaleax You may want to remove the |
|
@szmarczak So … maybe you have been more active in following the OpenSSL discussion than I have, but our primary options are either a) waiting for the next OpenSSL release and picking those changes up, b) cherry-picking the relevant commit(s) and hoping that that doesn’t work out too badly for downstream vendors (e.g. Linux distros that make Node.js use the system OpenSSL) or c) implementing a temporary fix in our TLS code that make the code work as expected. I think we’re unlikely to do b), and I don’t know what would be involved in c) – if you do know what we’d need to do (or are able to open a PR yourself), great, but otherwise only a) really remains and in that case we’re still blocked on upstream OpenSSL… |
|
Closing since Node.js upgraded |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
https://runkit.com/szmarczak/5d48640bd3cab4001399357c
The issue happens randomly. Sometimes
socket.servernameis set tofalse, sometimes it's a string. I tracked the source down to this C++ code:node/src/tls_wrap.cc
Lines 991 to 1006 in 0481a7f
servernamesometimes is a null pointer. This causeshttp2session.originSetto look like:The text was updated successfully, but these errors were encountered: