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

Don't allow half-open TLS connections #11

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@pimterry
Copy link

commented May 15, 2019

I'm trying to detect clients rejecting my HTTPS certificate, using httpolyglot.

With the current httpolyglot server, if a client cleanly closes a TLS connection after the server hello (i.e. when it decides it doesn't like the certificate), the current httpolyglot never fires a tlsClientError event, whilst https.Server does.

I've hit this because that's the behaviour of Node 12 TLS clients - they cleanly close connections at this point (in Node 10, the handshake completes, secureConnection fires, and then the client closes the connection without sending any data - that works fine).

The below test demonstrates the issue. I haven't committed it, because it only works with Node 12, but hopefully it shows what I'm talking about.

var fs = require('fs');
var https = require('https');
var assert = require('assert');

var common = require(__dirname + '/common');
var httpolyglot = require(__dirname + '/../lib/index');

var srv = httpolyglot.createServer({
  key: fs.readFileSync(__dirname + '/fixtures/server.key'),
  cert: fs.readFileSync(__dirname + '/fixtures/server.crt')
}, function() {
  assert(false, 'Request handler should not be called');
})

srv.listen(0, '127.0.0.1', common.mustCall(function() {
  var port = this.address().port;

  var request = https.get({
    host: '127.0.0.1',
    port: port,
    rejectUnauthorized: true
  });
  request.on('error', common.mustCall(function (error) {
    assert(
      /certificate/.test(error.message),
      'Request should be rejected with a certificate error'
    );
  }));
}));

// Without this change, this is never called:
srv.on('tlsClientError', common.mustCall(function() {
  srv.close();
}));

This appears to happen because httpolyglot allows half-open TLS connections, whilst https.Server does not. As far as I can tell half-open connections are only allowed by default on http.Server, so I've changed the code here to disable it on sockets once we know that they are definitely TLS.

With this change, the above test passes.

Don't allow half-open TLS connections
This fixes an issue where TLS clients that cleanly close connections
part-way through a handshake (e.g. node 12 TLS clients) did not trigger
a tlsClientError.
@pimterry

This comment has been minimized.

Copy link
Author

commented May 15, 2019

Just wanted to highlight that this is different to #10, in that this change only disables half-open sockets for TLS connections, and leaves plain HTTP untouched.

Afaict your comment there is only half correct, and HTTPS servers don't actually set allowHalfOpen to true. Mainly because the above test passes immediately if you use https.Server instead of httpolyglot, but also based on a skim of https://github.com/nodejs/node/blob/master/lib/https.js, which doesn't do that or call the http.Server constructor that would do so either.

@r0mflip

This comment has been minimized.

Copy link

commented May 15, 2019

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.