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 use tls.TLSSocket created from net.Socket after first 'data' event #8752

Closed
coolaj86 opened this issue Sep 23, 2016 · 25 comments
Closed
Labels
stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem.

Comments

@coolaj86
Copy link
Contributor

coolaj86 commented Sep 23, 2016

tried on node v6.3 and v6.6

My goal is to mux a socket to use either tcp or tls after inspecting the first packet to see if it's a hello.

The problem I have is that I could not pass the socket to tls.TLSSocket (or tls.Server, or https.Server) after the 'data' event (even though I replay it) and have it emit data.

issue-8752.js:

//
//
// SETUP
//
//
'use strict';

var net = require('net');
var tls = require('tls');
var http = require('http');
var tlsOpts = {
  key: require('fs').readFileSync('./privkey.pem')
, cert: require('fs').readFileSync('./fullchain.pem')
};
var httpServer = http.createServer(function (req, res) {
  res.end('Hello, World!');
});
var tcp3000 = net.createServer();

tcp3000.listen(3000, function () {
  console.log('listening on 3000');
});



tcp3000.on('connection', function (socket) {
  console.log('connection incoming');
  // this works when I put it here, but I don't know if it's tls yet here
  // httpsServer.emit('connection', socket);

  socket.once('data', function (chunk) {
    console.log('data incoming');

    if (/http\/1/i.test(chunk.toString())) {

      console.log("looks like http, continue");

      // this works as expected
      httpServer.emit('connection', socket);

    } else {

      //**********************************************************
      //
      // PROBLEM
      //
      //**********************************************************
      console.log("doesn't look like http, try tls");

      // none of these methods work:
      // httpsServer.emit('connection', socket);  // this didn't work
      // tlsServer.emit('connection', socket);    // this didn't work either
      var tlsSocket = new tls.TLSSocket(socket, { secureContext: tls.createSecureContext(tlsOpts) });
      tlsSocket.on('data', function (chunk) {
        // never called
        console.log('chunk', chunk);
      });

    }

    // replay first packet
    socket.emit('data', chunk);
    // I've also tried this, which didn't work at all:
    // socket.unshift(chunk);
    // socket.emit('readable');
  });

});
curl https://local.test.ppl.family
connection incoming
data incoming
looks like http, continue
curl: (35) gnutls_handshake() failed: An unexpected TLS packet was received.

From the documentation I've found I don't think I'm doing anything wrong and the same process works with a plain tcp socket, so it makes me wonder if there isn't something amiss in the internals.

Also, the following does work (even though it's useless):

'use strict';

var net = require('net');
var http = require('http');
var https = require('https');
var tlsOpts = {
  key: require('fs').readFileSync('./privkey.pem')
, cert: require('fs').readFileSync('./fullchain.pem')
};
var httpServer = http.createServer(function (req, res) {
  res.end('Hello, World!');
});
var httpsServer = https.createServer(tlsOpts, function (req, res) {
  res.end('Hello, Encrypted World!');
});
var tcp3000 = net.createServer();

tcp3000.listen(3000, function () {
  console.log('listening on 3000');
});



tcp3000.on('connection', function (socket) {
  // if I never let the 'data' event fire, it works as expected.
  httpsServer.emit('connection', socket);
});
@coolaj86
Copy link
Contributor Author

coolaj86 commented Sep 23, 2016

I found a workaround which suggests this must be a bug and not intentional behavior.

Even with the workaround, if I try to use tls.TLSSocket directly I get back an error:

curl: (35) Unknown SSL protocol error in connection to example.com:-9800

However, this allows me to at least use tls.Server.emit('connection'):

      console.log("doesn't look like http, try tls");

      var myDuplex = new Duplex();
      myDuplex._write = function (chunk, encoding, cb) {
        socket.write(chunk, encoding);
        cb();
      };
      myDuplex._read = function (size) {
        var chunk = socket.read(size);
        if (chunk) { this.push(chunk); }
      };
      socket.on('data', function (chunk) {
        myDuplex.push(chunk);
      });

      tlsServer.emit('connection', myDuplex);

And tlsServer looks like this:

var tlsServer = tls.createServer(tlsOpts, function (tlsSocket) {
  httpServer.emit('connection', tlsSocket);
});

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Sep 23, 2016
@mscdex
Copy link
Contributor

mscdex commented Sep 23, 2016

Did you try using the recommended way of upgrading a socket? For example:

var tlssocket = tls.connect({ socket: socket }, function() {
  console.log('Socket upgraded!');
  // use `tlssocket`
});

Also on a semi-related note, you shouldn't make any assumptions about chunks of data from a stream. The substring you're checking for could be cut across boundaries.

@mscdex
Copy link
Contributor

mscdex commented Sep 23, 2016

If you're wanting to serve http and https over the same port, you might try httpolyglot.

@coolaj86
Copy link
Contributor Author

coolaj86 commented Sep 27, 2016

I'm not trying to perform a socket upgrade. The socket is already tls. Nevertheless, when I replace my hacky code with your code I got this result:

events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at TLSSocket.onHangUp (_tls_wrap.js:1092:19)
    at TLSSocket.g (events.js:291:16)
    at emitNone (events.js:91:20)
    at TLSSocket.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)

I'm working on a tunneling service, which happens to include serving both http and https on the same port, but is a bit more complicated than just that.

The end goal is to allow many authenticated clients to tunnel anything - http, https, and even ssh or openvpn (wrapped in openssl as to appear to be https and to get the sni header) - and that will work in school dorms, HOAs, public libraries, corporate networks, airplanes, and other harsh network environments (and on all OSes without requiring admin privileges, iptables, etc).

Similar concept to localtunnel.me but with arbitrary encrypted traffic and eventually that could even work on a chrome book in the browser.

@mscdex
Copy link
Contributor

mscdex commented Sep 27, 2016

FWIW as far as SSH goes, you may run into issues with buggy SSH clients since I've seen some that actually wait for the server to respond with their ident string first before sending theirs (even though this kind of goes against the RFC, depending on your interpretation).

Also, tunneling OpenVPN over TCP? I think most clients would be using UDP for performance? Either way, wrapping OpenVPN traffic with encrypted OpenSSL traffic seems like a lot of overhead.

@coolaj86
Copy link
Contributor Author

I don't want to pull this thread away from the core issue of figuring out how to wrap and unwrap net and tls sockets, so I continued the tangent here: https://gist.github.com/coolaj86/f7c39baf83c32525f7c88ed13643e11d

@sam-github sam-github added the stream Issues and PRs related to the stream subsystem. label Dec 13, 2016
@Trott
Copy link
Member

Trott commented Jul 15, 2017

This is still an issue in Node.js 8.x?

@coolaj86
Copy link
Contributor Author

coolaj86 commented May 4, 2018

This is still an issues in node v9.11.1

@Trott
Copy link
Member

Trott commented May 5, 2018

@nodejs/crypto @nodejs/streams

@bnoordhuis
Copy link
Member

The reason the example doesn't work is that tls.TLSSocket short-circuits the net.Socket machinery for performance reasons. That's its sole raison d'être really, and not something we'll change.

You can use tls.createSecurePair(). It's deprecated but if it supports a use case that tls.TLSSocket does not, we can look into undeprecating it.

@coolaj86
Copy link
Contributor Author

@bnoordhuis The use case is building a net proxy service that can inspect incoming traffic and route it accordingly.

We don't know ahead of time if the incoming message is tls or not (or if it's http or https), so we need to inspect the first packet and then pump the stream through the rest of the stack.

Could you add a peek() function into the tcp API so that we can inspect and determine what it is?

We'd also like to be able to track the bytes going over the wire per-session. For some traffic we authenticate with https or wss much later, but we need access to the byte count from the tcp object (to know total packet size, including the increase caused by encryption).

@indutny
Copy link
Member

indutny commented May 12, 2018

I haven't been following this conversation closely, but it should be possible with JSStreamWrap: https://github.com/nodejs/node/blob/master/lib/internal/wrap_js_stream.js

@coolaj86
Copy link
Contributor Author

@indutny by "it" do you mean add a peek() function to a net.Socket that would allow it to be passed to create a tls.TLSSocket without crashing?

@indutny
Copy link
Member

indutny commented May 12, 2018

The original question is about prepending the data to the socket, and wrapping it with tls.Socket. This is possible through that API, you'll have to wrap the socket into a stream.Duplex, though.

@monster860
Copy link

I found a workaround that does not involve weird wrapping stuff.

So what I did is I replaced the socket.emit('data', chunk) with socket._handle.onread(chunk.length, chunk)

@coolaj86
Copy link
Contributor Author

coolaj86 commented May 24, 2018

@monster860 I'll have to try that out. Back when I created this issue that also failed in my use case, but I suspect the API has changed.

I also found that if you need an async event between the first packet and being ready to receive the next packet, you need to use socket.once('data', whatever) followed by socket.pause(), which will be resumed by the socket.on('data', whateverelse) after the callback.

// peek
socket.once('data', function (firstChunk) {
  // stop flow
  socket.pause();

  // supposing this works, as you suggest
  socket._handle.onread(firstChunk.length, firstChunk)
  // what I do presently
  // socket.unshift(firstChunk);

  // determine if this is tls or plain, and what to do about it (maybe it looks like http or tunneled ssh)
  checkItOut(firstChunk, function (err, result) {

    // do what needs to be done
    if ('doit' === result) {
      socket.on('data', doIt);
    } else if ('doother' === result) {
      socket.on('data', doother);
    } else {
      socket.on('data', dolast);
    }
    socket.resume();

  });
});

@monster860
Copy link

Hm... Here's what I'm doing:

let proxies = {
	http: http.createServer((req, res) => {
		res.writeHead(301, {"Location": "https://" + req.headers['host'] + req.url});
		res.end();
	}),
	https: https.createServer(server_config.http_opts, http_handler)
};
net.createServer((socket) => {
	socket.once('data', (buffer) => {
		socket.pause();
		let byte = buffer[0];
		let protocol = byte == 22 ? 'https' : 'http';
		let proxy = proxies[protocol];
		proxy.emit('connection', socket);
		socket._handle.onread(firstChunk.length, firstChunk);
		socket.resume();
	});
}).listen(server_config.port);

@bnoordhuis
Copy link
Member

Since we won't be making changes and workarounds exist, I'll go close this out.

Could you add a peek() function into the tcp API so that we can inspect and determine what it is?

No new API should be needed, you can peek by inserting e.g. a stream.Duplex between the net.Socket and tls.TLSSocket.

@coolaj86
Copy link
Contributor Author

coolaj86 commented May 25, 2018

@monster860 It looks like you and I are working on similar things right now. You might find Greenlock for Express.js or Greenlock for node.js to be valuable.

@coolaj86
Copy link
Contributor Author

coolaj86 commented Jun 9, 2018

@monster860 Have you tested your solution with connections that persist and data that is larger than the size of a single TCP packet?

When I remove my workaround I'm still seeing the same behavior where the second data packet never comes through.

@coolaj86
Copy link
Contributor Author

coolaj86 commented Jun 9, 2018

@bnoordhuis Does piping it through a duplex make it slower to decode the TLS than passing a net socket directly? It seems like there's a lot of latency.

@bnoordhuis
Copy link
Member

I doubt it makes much of a difference, I would expect the cost of encryption/decryption to dominate. Have you profiled it?

@monster860
Copy link

monster860 commented Jun 9, 2018

@coolaj86 My application involves websockets, so yes.

I've also tried piping through a duplex and that introduces a noticable lag.

@coolaj86
Copy link
Contributor Author

coolaj86 commented Aug 31, 2018

@monster860 Thought you might like to know that there's an almost undocumented readable event that will save you all sorts of trouble - no need to pause() or resume() and you'll have far fewer issues with waterlines in buffers.

https://nodejs.org/api/stream.html#stream_event_readable

I wish I had known about this years ago. I don't know why the net and tls docs haven't been updated. I can't think of any reason why a data event would be more useful than readable.

@d1m96
Copy link

d1m96 commented Jan 6, 2020

How can i write own handshake protocol with the help of sockets (and may be 'net' module) like https with request/response encryption over node.js express server logic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants