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

Cannot reuse an estabilished TLS socket for a HTTP/2 session #35475

Closed
szmarczak opened this issue Oct 3, 2020 · 6 comments
Closed

Cannot reuse an estabilished TLS socket for a HTTP/2 session #35475

szmarczak opened this issue Oct 3, 2020 · 6 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@szmarczak
Copy link
Member

  • Version: 14.12.0
  • Platform: Linux solus 5.6.19-158.current #1 SMP PREEMPT Sun Jul 26 14:17:01 UTC 2020 x86_64 GNU/Linux
  • Subsystem: http2

What steps will reproduce the bug?

const http2 = require('http2');
const tls = require('tls');

const options = {
  ALPNProtocols: ['h2'],
  host: 'nghttp2.org',
  servername: 'nghttp2.org',
  port: 443
};

const socket = tls.connect(options, async () => {
    console.log('Connected!');
    
    await new Promise(resolve => setTimeout(resolve, 1000));
    
    const session = http2.connect('https://nghttp2.org', {
        createConnection: () => socket
    });

    session.once('remoteSettings', () => {
        console.log('Received remote settings!');
    });
});

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Connected!
Received remote settings!

What do you see instead?

Connected!
NghttpError: Protocol error

Additional information

This is similar to #33343

@szmarczak
Copy link
Member Author

What does Protocol error mean? It could be anything.

@szmarczak
Copy link
Member Author

szmarczak commented Oct 3, 2020

Ah, because the server sends us data and I don't catch that... Actually no because there's nothing to read directly from the socket... What's happening?

@szmarczak szmarczak reopened this Oct 3, 2020
@lpinca lpinca added the http2 Issues or PRs related to the http2 subsystem. label Oct 4, 2020
@szmarczak
Copy link
Member Author

I think that the socket must have no data buffered.

Putting console.log(socket.readableLength); after the await tells us that there is some data to be read. But if we lower the timeout to 100ms then there's no queued data to be read and it passes.

@szmarczak
Copy link
Member Author

Dunno... Wrapping `socket` like this makes things work
const http2 = require('http2');
const tls = require('tls');
const stream = require('stream');

const options = {
  ALPNProtocols: ['h2'],
  host: 'nghttp2.org',
  servername: 'nghttp2.org',
  port: 443
};

const socket = tls.connect(options, async () => {
    console.log('Connected!');
    
    await new Promise(resolve => setTimeout(resolve, 1000));
    
    const proxy = new stream.Duplex({
        read(...args) {
            const x = socket.read();

            if (x !== null) {
                this.push(x);
            }
        },
        
        final(...args) {
            socket.end(...args);
        },
        
        write(...args) {
            socket.write(...args);
        },
        
        destroy(...args) {
            socket.destroy(...args);
        }
    });
    
    const session = http2.connect('https://nghttp2.org', {
        createConnection: () => proxy
    });

    session.once('remoteSettings', () => {
        console.log('Received remote settings!');
    });
});

@mmomtchev
Copy link
Contributor

@szmarczak your analysis is correct, the problem is the data awaiting in the socket on the JS side
wrapping the socket makes the stream pass through JS which solves the problem

When the TLS code establishes the connections, it asks for HTTP2 via APLN. The remote server sends immediately its SETTINGS frame which is received by the TLS code and kept in its buffer.
Then HTTP2 client attaches its Listener and sends its client SETTINGS frame and waits for a read callback.
The first read callback will be for the second frame of the server and this triggers a 505, "Remote peer returned unexpected data while we expected SETTINGS frame. Perhaps, peer does not support HTTP/2 properly." which is transformed into a PROTOCOL_ERROR on the user side.

Now, the question is how is this supposed to work 😃
The TLS code has this:

  // Socket already has some buffered data - emulate receiving it
  if (socket && socket.readableLength) {
    let buf;
    while ((buf = socket.read()) !== null)
      tlsSocket._handle.receive(buf);
  }

and this:

void TLSWrap::Receive(const FunctionCallbackInfo<Value>& args) {
  TLSWrap* wrap;
  ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

  ArrayBufferViewContents<char> buffer(args[0]);
  const char* data = buffer.data();
  size_t len = buffer.length();
  Debug(wrap, "Receiving %zu bytes injected from JS", len);

The HTTP2 code doesn't seem to support injection from JS - or I was unable to find it?
@jasnell?

mmomtchev added a commit to mmomtchev/node that referenced this issue Oct 16, 2020
Reinject the data already received from the TLS
socket when the HTTP2 client is attached with a
delay

Fixes: nodejs#35475
mmomtchev added a commit to mmomtchev/node that referenced this issue Oct 16, 2020
Reinject the data already received from the TLS
socket when the HTTP2 client is attached with a
delay

Fixes: nodejs#35475
@mutza97
Copy link

mutza97 commented Oct 25, 2020

  • Version: 14.12.0
  • Platform: Linux solus 5.6.19-158.current #1 SMP PREEMPT Sun Jul 26 14:17:01 UTC 2020 x86_64 GNU/Linux
  • Subsystem: http2

What steps will reproduce the bug?

const http2 = require('http2');
const tls = require('tls');

const options = {
  ALPNProtocols: ['h2'],
  host: 'nghttp2.org',
  servername: 'nghttp2.org',
  port: 443
};

const socket = tls.connect(options, async () => {
    console.log('Connected!');
    
    await new Promise(resolve => setTimeout(resolve, 1000));
    
    const session = http2.connect('https://nghttp2.org', {
        createConnection: () => socket
    });

    session.once('remoteSettings', () => {
        console.log('Received remote settings!');
    });
});

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Connected!
Received remote settings!

What do you see instead?

Connected!
NghttpError: Protocol error

Additional information

This is similar to #33343

targos pushed a commit that referenced this issue Nov 3, 2020
Reinject the data already received from the TLS
socket when the HTTP2 client is attached with a
delay

Fixes: #35475

PR-URL: #35678
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alba Mendez <me@alba.sh>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 8, 2020
Reinject the data already received from the TLS
socket when the HTTP2 client is attached with a
delay

Fixes: #35475

PR-URL: #35678
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alba Mendez <me@alba.sh>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 10, 2020
Reinject the data already received from the TLS
socket when the HTTP2 client is attached with a
delay

Fixes: #35475

PR-URL: #35678
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alba Mendez <me@alba.sh>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
BethGriggs pushed a commit that referenced this issue Dec 15, 2020
Reinject the data already received from the TLS
socket when the HTTP2 client is attached with a
delay

Fixes: #35475

PR-URL: #35678
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Alba Mendez <me@alba.sh>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants