Skip to content

Commit

Permalink
Merge #125: enforce timeout on initial socks5 proxy connection
Browse files Browse the repository at this point in the history
d8554fb enforce timeout on initial socks5 proxy connection (conduition)

Pull request description:

  This PR fixes a bug which excepted initial SOCKS5 proxy connection attempts from the punctual enforcement of timeouts.

  Before this change, invoking `Socks5Stream::connect` (or `Socks5Stream::connect_with_password`) could block for much longer than the configured timeout. In practice, this manifested as `Client::from_config` apparently failing to respect the timeout specified in the `Config` passed to it. AFAICT this only applied to SOCKS proxy connections.

  ## Example

  To demonstrate, here is a simple example program which attempts to connect to an unreachable electrum server with a 10 second timeout.
  ```rust
  use electrum_client::{Client, ConfigBuilder, Socks5Config};

  fn main() {
      let proxy = Socks5Config::new("127.0.0.1:9050");
      let config = ConfigBuilder::new()
          .socks5(Some(proxy))
          .timeout(Some(10))
          .build();

      let start = std::time::SystemTime::now();
      let result = Client::from_config(
          "tcp://bejqtnc64qttdempkczylydg7l3ordwugbdar7yqbndck53ukx7wnwad.onion:50001",
          config,
      );
      match result {
          Ok(_) => {
              println!("Successfully connected")
          }
          Err(e) => {
              println!(
                  "failed to connect after {:.2}s: {e}",
                  start.elapsed().unwrap().as_secs_f64()
              );
          }
      };
  }
  ```

  You'd expect the connection attempt to always fail at around 10 seconds, but in fact most attempts take considerably longer.

  ```
  $ for i in {1..10} ; do cargo run -q ; done
  failed to connect after 7.65s: host unreachable
  failed to connect after 47.78s: host unreachable
  failed to connect after 18.17s: host unreachable
  failed to connect after 29.24s: host unreachable
  failed to connect after 16.15s: host unreachable
  failed to connect after 14.40s: host unreachable
  failed to connect after 16.89s: host unreachable
  failed to connect after 9.93s: host unreachable
  failed to connect after 8.81s: host unreachable
  failed to connect after 17.80s: host unreachable
  ```

  ## Cause and Fix

  This was happening because the private method `Socks5Stream::connect_raw` [only respected the `timeout` parameter for _the initial connection to the proxy address_](https://github.com/bitcoindevkit/rust-electrum-client/blob/5ecb26fd7dab552657be511b9c05f7e50cec4cb8/src/socks/v5.rs#L200-L205).

  Once that TCP socket is established, the SOCKS5 client code must exchange a couple of messages with the proxy itself: One request/response cycle to authenticate, and then another request/response cycle to configure the forward proxy to the ultimate destination address. The latter of these two request/response cycles could block for long periods of time, in the case where the proxy was responsive but the ultimate destination was unresponsive.

  Since no timeout was set on the socket at this stage, the `Socks5Stream` code would wait for an indefinite amount of time for a reply from the proxy, usually only once the proxy itself times out and finally sends a reply.

  My suggested fix in this PR is to set the read/write timeouts immediately on the socket connecting to the proxy, so that if the proxy doesn't reply in time, we return an error to the caller promptly.

ACKs for top commit:
  RCasatta:
    utACK d8554fb

Tree-SHA512: 4bc0ca203465c0d9722680de7251ee49dbea6d5a2b2a833a1ed42e792342a7ac977ad72649603caacd3e58d233b1a516af1ad40c6935addb8165b720d823cf42
  • Loading branch information
notmandatory committed Dec 8, 2023
2 parents 91228df + d8554fb commit fd81717
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions src/socks/v5.rs
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,9 @@ impl Socks5Stream {
TcpStream::connect(proxy)?
};

socket.set_read_timeout(timeout)?;
socket.set_write_timeout(timeout)?;

let target = target.to_target_addr()?;

let packet_len = if auth.is_no_auth() { 3 } else { 4 };
Expand Down

0 comments on commit fd81717

Please sign in to comment.