workers-rs icon indicating copy to clipboard operation
workers-rs copied to clipboard

[BUG] TCP Connect does not error if connection not able to be created

Open cmmarslender opened this issue 1 year ago • 8 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

What version of workers-rs are you using?

main

What version of wrangler are you using?

3.26.0

Describe the bug

The tldr of this issue is a request to make connect() fail if the connection could not be established due to a closed port, or some other reason. See below for details

It seems that socket connect() doesn't actually return an error if a connection is unable to be created for some reason (port closed, etc). Trying to build a worker that as quickly and simply as possible can let a user know if a port on their IP is open or not.

Generally, I'd expect an error from the connect() call if the port is closed, but in the workers env, this doesn't seem to hold true. Here's the code that I would expect to fail on a closed port that just continues past a closed port:

let mut socket = Socket::builder()
        .secure_transport(SecureTransport::Off)
        .connect(&connecting_ip, port)
        .map_err(|_| worker::Error::from("Could not create socket"))?;

For the most part, I can reliably tell if the port is open or closed by attempting to write some data AND then read a byte from the socket after the connect call (write isn't enough, and also doesn't seem to fail if the port is closed - have to add the read step). This isn't particularly ideal though, since I have to rely on binding setTimeout into rust and setting a timeout on the read to determine if its not going to succeed. This method also doesn't work for all protocols that I need to check for open ports.

I was curious to see how this worked in a non-workers rust environment, and was able to reliably determine if a port is open at the connect call, using the following code:

use std::net::{SocketAddr, TcpStream};
use std::str::FromStr;
use std::time::Duration;

fn main() {
    let remote_ip = "1.2.3.4";
    let port = 123;
    let timeout = Duration::from_secs(1);

    // Construct the remote address from the IP and port
    let remote_addr = match SocketAddr::from_str(&format!("{}:{}", remote_ip, port)) {
        Ok(addr) => addr,
        Err(e) => {
            eprintln!("Error constructing remote address: {}", e);
            return;
        }
    };

    // Attempt to connect with a timeout
    match TcpStream::connect_timeout(&remote_addr, timeout) {
        Ok(_) => println!("Port {} is open on {}", port, remote_ip),
        Err(e) => println!("Failed to connect to port {} on {}: {}", port, remote_ip, e),
    }
}

Steps To Reproduce

No response

cmmarslender avatar Feb 07 '24 16:02 cmmarslender

This matches the behavior of the JavaScript API, which returns from connect immediately (it is not a Promise), and thus does not indicate whether the connection has been established with the remote server. Errors are then propagated when trying to read / write from the socket.

kflansburg avatar Feb 07 '24 18:02 kflansburg

This matches the behavior of the JavaScript API, which returns from connect immediately (it is not a Promise), and thus does not indicate whether the connection has been established with the remote server. Errors are then propagated when trying to read / write from the socket.

So thats generally fine, but I was also seeing a write not fail on a socket that didn't actually have an open port, unless I did this wrong somehow.

    let mut socket = Socket::builder()
        .secure_transport(SecureTransport::Off)
        .connect(&connecting_ip, port)
        .map_err(|_| worker::Error::from("Could not create socket"))?;

    socket
        .write(b"GET / HTTP/1.0\r\nHost: example.com\r\n\r\n")
        .await
        .map_err(|_| worker::Error::from("Could not write to socket"))?;

This write doesn't fail for me, even on a closed port, and it sounds like it should err?

cmmarslender avatar Feb 07 '24 18:02 cmmarslender

Does reading return an error? I think it should error, but I will have to check the JavaScript behavior.

kflansburg avatar Feb 08 '24 02:02 kflansburg

Reading returns an error in most cases. There are certain types of applications where this doesn't seem to be particularly reliable though.

I did some testing in node earlier, and while the socket.connect call itself does not return a promise, you are able to attach event listeners to events like connect, timeout, and error which enable getting at this info more reliably than writing then trying to read. It would be nice to have some way to get at this in Rust (and I was tinkering a bit in JS with workers, and wasn't able to get it to work in that context either). Node test code here https://gist.github.com/cmmarslender/145a929163d8f3501490bf00bed2e993

I think my current rust implementation covers most of the cases I need it to cover, but it would be nice if this could eventually work with just connect and an error there, or attaching an on connect/error/timeout handler, or perhaps some other solution, that doesn't require having to write and read.

cmmarslender avatar Feb 08 '24 03:02 cmmarslender

@dom96 thoughts on this behavior?

kflansburg avatar Feb 08 '24 15:02 kflansburg

I did some testing in node earlier, and while the socket.connect call itself does not return a promise, you are able to attach event listeners to events like connect, timeout, and error which enable getting at this info more reliably than writing then trying to read

We now have an opened promise that can be used for this purpose. It should get rejected if the connection fails to be opened and that shouldn't require any reads/writes to happen.

As for the write not failing, I believe that is expected (for the same reason connect doesn't return a promise which waits for the connection result) and the error ends up in the closed promise instead.

dom96 avatar Feb 08 '24 19:02 dom96

I did some testing in node earlier, and while the socket.connect call itself does not return a promise, you are able to attach event listeners to events like connect, timeout, and error which enable getting at this info more reliably than writing then trying to read

We now have an opened promise that can be used for this purpose. It should get rejected if the connection fails to be opened and that shouldn't require any reads/writes to happen.

As for the write not failing, I believe that is expected (for the same reason connect doesn't return a promise which waits for the connection result) and the error ends up in the closed promise instead.

is it possible to get at the opened promise in some way from Rust, and if so, are there docs or an example on this somewhere? I haven't seen anything, but may be looking in the wrong places.

cmmarslender avatar Feb 08 '24 21:02 cmmarslender

Yep, I think it would be reasonable to add that to the Rust API. Also looks like we need to add it to our JavaScript docs.

kflansburg avatar Feb 08 '24 21:02 kflansburg