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

Malformed crt with HTTPS SNI causes hang - no error, no result #867

Closed
coolaj86 opened this issue Feb 17, 2015 · 7 comments
Closed

Malformed crt with HTTPS SNI causes hang - no error, no result #867

coolaj86 opened this issue Feb 17, 2015 · 7 comments

Comments

@coolaj86
Copy link
Contributor

This just hangs without throwing an error or completing the request:

curl https://localhost:65443 -k

Expected result

client receives

Cannot GET /

server logs

[log] SNI: local.helloworld3000.com
[log] SNI: undefined
[log] {}
[log] request
[log] request for local.helloworld3000.com:65443/

Reduced Test Case

'use strict';

var https           = require('https');
var fs              = require('fs');
var path            = require('path');
var crypto          = require('crypto');
var connect         = require('connect');

module.exports.create = function (_securePort, _insecurePort) {
    // connect / express app
  var app             = connect();

    // SSL Server
  var secureContexts  = {};
  var dummyCerts;
  var secureOpts;
  var secureServer;
  var securePort      = _securePort || 443;

    // force SSL upgrade server
  var insecureServer;
  var insecurePort    = _insecurePort || 80;

  function loadDummyCerts() {
    var certsPath = path.join(__dirname, 'certs');
    var certs = {
      key:          fs.readFileSync(path.join(certsPath, 'server', 'dummy-server.key.pem'))
    , cert:         fs.readFileSync(path.join(certsPath, 'server', 'dummy-server.crt.pem'))
    , ca:           fs.readdirSync(path.join(certsPath, 'ca')).map(function (node) {
                      return fs.readFileSync(path.join(certsPath, 'ca', node));
                    })
    };
    secureContexts.dummy = crypto.createCredentials(certs).context;
    dummyCerts = certs;
  }
  loadDummyCerts();

  app.use(function (req, res, next) {
    console.log('[log] request for ' + req.headers.host + req.url);
    next();
  });

  function runServer() {
    //provide a SNICallback when you create the options for the https server
    secureOpts = {
      //SNICallback is passed the domain name, see NodeJS docs on TLS
      SNICallback:  function (domainname) {
                      console.log('[log] SNI:', domainname);
                      console.log('[log] SNI:', secureContexts[domainname]);
                      var secureContext = secureContexts[domainname] || secureContexts.dummy;
                      console.log('[log]', secureContext);
                      return secureContext;
                    }
                    // fallback / default dummy certs
    , key:          dummyCerts.key
    , cert:         dummyCerts.cert
    , ca:           dummyCerts.ca
    };

    secureServer = https.createServer(secureOpts);
    secureServer.on('request', function (req, res) {
      console.log('[log] request');
      app(req, res);
    });
    secureServer.listen(securePort, function () {
      console.log("Listening on https://localhost:" + secureServer.address().port);
    });
  }

  runServer();
}
module.exports.create(443, 80);

directory layout

tree
├── vhost-sni-server.js
├── certs
│   ├── ca
│   │   ├── dummy-root-ca.crt.pem
│   │   ├── my-root-ca.crt.pem
│   │   └── my-root-ca.key.pem
│   ├── README.md
│   └── server
│       ├── dummy-server.crt.pem
│       ├── dummy-server.key.pem
│       ├── my-server.crt.pem
│       └── my-server.key.pem
@coolaj86
Copy link
Contributor Author

Confirmed broken in

  • v0.12.0 on OS X
  • v1.2.0 on OS X
  • v1.2.0 on ARM6

Confirmed working in

  • v0.10.36 on OS X
  • v0.10.36 on ARM6

Perhaps there's an API change that I'm unaware of? If so it seems like this should throw an error, not just hang.

@coolaj86 coolaj86 changed the title HTTPS SNI broken on arm6l (maybe others as well) Using HTTPS SNI causes hang - no error, no result Feb 17, 2015
@coolaj86 coolaj86 changed the title Using HTTPS SNI causes hang - no error, no result Malformed crt with HTTPS SNI causes hang - no error, no result Feb 17, 2015
@coolaj86
Copy link
Contributor Author

Okay, so v0.10.36 has a flaw that allows it to work - it ignores the key file instead of erroring on it. A key file, obviously, should not get loaded into the CA chain.

node.js / io.js latest have the flaw that although they don't work with a key file in the chain (good), they don't give an error or any indication of what went wrong.

After removing the key file that was in the wrong place:

├── vhost-sni-server.js
├── certs
│   ├── ca
│   │   ├── dummy-root-ca.crt.pem
│   │   └── my-root-ca.crt.pem
│   ├── README.md
│   └── server
│       ├── dummy-server.crt.pem
│       ├── dummy-server.key.pem
│       ├── my-server.crt.pem
│       └── my-server.key.pem

It works.

# using the name on the cert
curl https://local.helloworld3000.com:65443 --cacert certs/ca/dummy-root-ca.crt.pem
> Cannot GET /

# using insecure
curl https://localhost:65443 -k
> Cannot GET /

I can upload the exact certs that I'm using upon request. They're all test / dummy certs anyway, so no risk there.

@coolaj86
Copy link
Contributor Author

Hmm... my workaround didn't work on ARM, only on OS X. There's still something wrong...

@Fishrock123
Copy link
Contributor

@coolaj86 are you still experiencing this? cc @indutny?

@coolaj86
Copy link
Contributor Author

It only happens when I return the crts immediately (not using the optional callback) AND there's a malformed crt (aka a key) in the chain.

If I use the callback or I take out the bad crt it works as expected (except I don't remember if using the callback sends an error or silently ignores the key).

And it happens on OS X, but on Raspberry Pi returning immediately doesn't work at all anyway.

I'd have to reinstall v1.2 or master to be certain, but I had a test case On v1.2 at the time I created the issue.

@indutny
Copy link
Member

indutny commented Feb 26, 2015

@coolaj86 returning context is not supported anymore, I'm not sure how it could be working :)

@coolaj86
Copy link
Contributor Author

Maybe I had my node versions mixed up. Maybe I was actually on 0.11.

In that case, sorry for the noise, I'll close this and the related issue and reopen if I can reconfirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants