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

TLS Server Interface Buf #41880

Closed
prettydiff opened this issue Feb 7, 2022 · 7 comments · Fixed by #41917
Closed

TLS Server Interface Buf #41880

prettydiff opened this issue Feb 7, 2022 · 7 comments · Fixed by #41917
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@prettydiff
Copy link
Contributor

prettydiff commented Feb 7, 2022

Version

17.4.0

Platform

Windows 10

Subsystem

No response

What steps will reproduce the bug?

An insecure socket server is created as:

const connect = function (socket) {},
    server = net.createServer();
server.listen({
    port: 80
}, listener);
server.on("connection", connect);

A secure socket server is created as:

const connect = function (socket) {},
    server = tls.createServer({
        cert: "string",
        key: "string"
    }, connect);
server.listen({
    port: 443
}, listener);

The major difference is where I put the connect event handler for the "connection" event. I can execute a TLS server with the same pattern as the insecure server and the connect handler will fire, but the socket passed into the connect handler will never execute data events or error events. You can console.log that socket, but it is effectively mute. This is impossible to troubleshoot and no feedback is provided.

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior?

There is a choice of resolutions:

  1. Throw on assignment to the connection event of a TLS server, eg: server.on("connection", connect);
  2. Fully support sockets created by an assigned connection handler.

What do you see instead?

Nothing. When this error is encountered I can see the handler for the connection event fire and I can console.log the socket passed into that handler, but all events assigned to that socket are ignored and there is no feedback of any kind. Problematic code example:

const connect = function (socket) {},
    server = tls.createServer();
server.listen({
    port: 80
}, listener);
server.on("connection", connect);

Additional information

No response

@VoltrexKeyva VoltrexKeyva added the tls Issues and PRs related to the tls subsystem. label Feb 7, 2022
@bnoordhuis
Copy link
Member

tls.Server is implemented on top of net.Server. The "connection" event is emitted by the latter, with a net.Socket as its argument.

It sounds like you're looking for the "secureConnection" event. It has the tls.Socket you're interested in.

@prettydiff
Copy link
Contributor Author

prettydiff commented Feb 8, 2022

Looking at the documentation I see both a connection and secureConnection as supported events on TLS.Server. Should the connection event be removed from the documentation if it doesn't work. Or better, should the connection event redirect to the secureConnection event?

@bnoordhuis
Copy link
Member

I don't think so. The documentation for "connection" says (among other things) this:

This event can also be explicitly emitted by users to inject connections
into the TLS server. In that case, any Duplex stream can be passed.

That's valuable knowledge.

@prettydiff
Copy link
Contributor Author

That is true, except the sockets generated in those connections are completely unusable and there is no messaging to that effect.

@bnoordhuis
Copy link
Member

It also says this:

This event is emitted when a new TCP stream is established, before the TLS handshake begins. [..] Usually users will not want to access this event.

Seems clear enough to me but suggestions for improvement are of course welcome.

@prettydiff
Copy link
Contributor Author

The problem is that its only clear after the fact. What I suspect is happening is this:

  1. the connection event is inherited from the net library.
  2. the connection handler receives a socket, because that is how the net library works.
  3. the socket accepts events, such as data, exactly as document in the net library.
  4. the socket never executes events because the later created secure socket receives all such events

This is undocumented behavior. It is unclear until after it is encountered and there is no communication about this behavior anywhere in either the documentation, error messaging, or stdout.

Perhaps passing null into the TLS connection event handler, instead of a socket, would suffice. I could possibly see a testing reason to determine if a TCP connection is made before the TLS handshake if you were wanting to test Node internals, but the socket passed into the handler has no value.

@bnoordhuis
Copy link
Member

You're welcome to send a pull request that clarifies the documentation but anything that changes the runtime behavior is pretty much guaranteed to get shot down immediately.

aduh95 pushed a commit that referenced this issue Feb 12, 2022
PR-URL: #41917
Fixes: #41880
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
PR-URL: nodejs#41917
Fixes: nodejs#41880
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
PR-URL: nodejs#41917
Fixes: nodejs#41880
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
bengl pushed a commit that referenced this issue Feb 22, 2022
PR-URL: #41917
Fixes: #41880
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
PR-URL: nodejs#41917
Fixes: nodejs#41880
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this issue Apr 24, 2022
PR-URL: #41917
Fixes: #41880
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants