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

Add an easy way to start a TLS communication on top of a plain *server side* Socket #13368

Closed
paleolitico opened this issue Jun 1, 2017 · 7 comments
Labels
feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem.

Comments

@paleolitico
Copy link

  • Version: Any
  • Platform: Any
  • Subsystem: tls

The recommended way to upgrade a plain socket to a TLS socket is to wrap
it with new TLSSocket(...).
But this wrapping doesn't check certificates
and doesn't emit secureConnect event.
There seem to be no public API to do that checking
(at least I couldn't find it).

Method tls.connect can be used to wrap socket to TLSSockets,
with certificate checking and with secureConnect emision.
But it can be used only for client side sockets (isServer == false)
and not for server side sockets got at a net.Socket connection event.

Reading the source code, I found that checking certificates is done with
socket._handle.verifyError(), both for TLSSocket produced by tls.Server and
created with tls.connect.
This is not a documented API, and although the verifyError method has been
there for a long time, I am not sure if I should use it in user space code.

I think it would be nice to have a way to fully wrap
a plain server side socket,
with certificate checking and secureConnect emission.
Or a public and documented API to do certificate checking.

Also, it would be nice if the documentation clearly explained
when certificates are checked and secureConnect is emitted.

@sam-github
Copy link
Contributor

I agree, I tried to document the APIs, but couldn't get progress on it: #10846

At the moment there is no documented way to use new TLSSocket securely.

@mscdex mscdex added feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem. labels Jun 1, 2017
@alexfernandez
Copy link

Any news on this issue? @addaleax has done great work cleaning up the tls.createSecurePair() mess in #17882, but the situation is largely the same and docs are still painfully incomplete if I'm not wrong.

@addaleax
Copy link
Member

@alexfernandez I think #17599 documents how to get this working current the situation?

I think @pepagos assessed everything correctly – on the server side, new TLSSocket() works, on the client side tls.connect() works. It would be great to have working new TLSSocket() for either type of connection, though.

@alexfernandez
Copy link

alexfernandez commented Apr 23, 2018

@addaleax The problem is that on the server side, new TLSSocket() does not work as advertised: it does not emit secureConnect as it should according to the docs, but secure. I have to say that new TLSSocket() seems to be checking certificates, which is good news. The situation is the same with v8.11.1 and with nightly from 20180416 (the code has not changed since).

I am attaching a proof of concept that creates a clear server and wraps every connection using new TLSSocket(), and then connects to this server. I am using certificates and keys from Node.js fixtures. Just untar and run simple-tls-new.js inside the simple directory. Note that the secureConnect event is never received on the server side, while secure is. What am I doing wrong?

simple.tar.gz

@alexfernandez
Copy link

alexfernandez commented Apr 23, 2018

Note: for easier code interchange I have created a repo with the example. This is the line that is being called with the secure event.

@alexfernandez
Copy link

A test can be easily added to test/parallel/test-tls-generic-stream.js to verify that secureConnect is not emitted, while secure is.

@refack
Copy link
Contributor

refack commented Nov 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants