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

SNICallback doesn't appear to work with tls.createSecurePair() #4878

Closed
adammw opened this issue Jan 26, 2016 · 13 comments
Closed

SNICallback doesn't appear to work with tls.createSecurePair() #4878

adammw opened this issue Jan 26, 2016 · 13 comments
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.

Comments

@adammw
Copy link
Contributor

adammw commented Jan 26, 2016

Test case: https://gist.github.com/adammw/cf4327506d4293e69014

Testing hitting the server with cURL:

curl -v --resolve www.example.com:4443:127.0.0.1 https://www.example.com:4443/

Docs say that there should be two arguments passed to SNICallback, the servername and the callback to call with the secure context. The second argument doesn't appear to be passed through when the TLS connection is created as a stream with tls.createSecurePair().

I get the error:

TypeError: cb is not a function
    at Object.tls.createSecurePair.SNICallback [as onselect]

I note that when removing the callback argument I get an OpenSSL error in my terminal:

Error: 140735169483536:error:1408A0C1:SSL routines:ssl3_get_client_hello:no shared cipher:../deps/openssl/openssl/ssl/s3_srvr.c:1411:

Potentially related PR: #2441 /cc @socketpair

@evanlucas evanlucas added confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. labels Jan 26, 2016
@evanlucas
Copy link
Contributor

Yea, confirmed...looks like https://github.com/nodejs/node/blob/master/src/node_crypto.cc#L2691 could be where the issue lies? /cc @indutny

@bnoordhuis
Copy link
Member

Reproducing the test case here for posterity:

#!/usr/bin/env node
var fs = require('fs');
var net = require('net');
var tls = require('tls');

var secureContext = tls.createSecureContext({
  cert: fs.readFileSync('test_cert.pem'),
  key: fs.readFileSync('test_key.pem')
})

var server = net.Server(function(raw) {
  var pair = tls.createSecurePair(null, true, false, false, {
    SNICallback: function(servername, cb) {
      console.log('servername', servername);
      cb(null, secureContext); 
    }
  });
  raw.pipe(pair.encrypted).pipe(raw);
});

server.listen(4443, function() {
  var addr = server.address();
  console.log('Server listening on %s %s:%d', addr.family, addr.address, addr.port);
});

Here, tls.createSecurePair() calls into lib/_tls_legacy.js. That version of SNICallback never had a callback argument, IIRC. It's implemented in terms of SSL_CTX_set_tlsext_servername_callback(), its callback needs to accept or reject immediately. The non-legacy SNICallback is implemented through SSL_set_cert_cb(), which is asynchronous and can defer the action until a later time.

I think it should be possible to move the legacy implementation over to SSL_set_cert_cb(). It's a bit of work but it would reduce the line count. Alternatively, we could just document it.

@mscdex mscdex added tls Issues and PRs related to the tls subsystem. and removed crypto Issues and PRs related to the crypto subsystem. labels Jan 26, 2016
@jhamhader
Copy link
Contributor

@bnoordhuis by saying

'I think it should be possible to move the legacy implementation over to SSL_set_cert_cb()'

Do you mean moving the whole createSecurePair() from _tls_legacy?
Sounds interesting and I would really like to work on that, but might require a bit of mentoring.

@jhamhader
Copy link
Contributor

I'm on it.

@ghost
Copy link

ghost commented Feb 24, 2016

I'm having a related problem, discribed here: https://stackoverflow.com/questions/35586957/nodejs-starttls-use-sni and there seems to be no answer, I'd like to confirm if this bug might be the cause, as it's a little bit different use case.

@bnoordhuis
Copy link
Member

@webertlima Looks to be the same issue, yes.

@dcposch
Copy link
Contributor

dcposch commented Mar 7, 2016

@jhamhader how goes?

@indutny
Copy link
Member

indutny commented Mar 9, 2016

@jhamhader let me know if you need any help on this. I would be more than happy to supply any amount of hints, or pick it over from you (if you are busy with other things).

@jhamhader
Copy link
Contributor

I would like that and will contact you over IRC/mail.

@webertrlz
Copy link

Any progress on this?

@jhamhader
Copy link
Contributor

tls.createSecurePair() has been deprecated (#6063)

@ghost
Copy link

ghost commented Jun 20, 2016

@jhamhader Alright. Tried using TLSSocket, had to change a lot of code but it'll do just fine. Thanks very much for hitting back.

@indutny indutny closed this as completed Sep 16, 2016
@indutny
Copy link
Member

indutny commented Sep 16, 2016

Looks like the issue is resolved in some sense.

This was referenced Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants