[Openvpn-devel,v2] Implement server_poll_timeout for socks

Message ID 20240315162011.1661139-1-frank@lichtenheld.com
State Accepted
Headers show
Series [Openvpn-devel,v2] Implement server_poll_timeout for socks | expand

Commit Message

Frank Lichtenheld March 15, 2024, 4:20 p.m. UTC
From: 5andr0 <sandro.trianni@gmail.com>

So far --server-poll-timeout was only applied
for HTTP proxies, apply it also to SOCKS proxies.

This removes the default 5 second socks connect timeout
which can be too small depending on network setup and
replaces it with the configurable overall connect timeout
(default 120 seconds).

Trac: #328
Change-Id: I2b109f8c551c23045a1be355778b08f0fd4d309f
Signed-off-by: 5andr0 <sandro.trianni@gmail.com>
---
 src/openvpn/socket.c |  2 ++
 src/openvpn/socks.c  | 25 ++++++++++++++-----------
 src/openvpn/socks.h  |  2 ++
 3 files changed, 18 insertions(+), 11 deletions(-)

Trying to move this forward by adding a commit message.

Comments

Frank Lichtenheld March 15, 2024, 4:40 p.m. UTC | #1
On Fri, Mar 15, 2024 at 05:20:11PM +0100, Frank Lichtenheld wrote:
> From: 5andr0 <sandro.trianni@gmail.com>
> 
> So far --server-poll-timeout was only applied
> for HTTP proxies, apply it also to SOCKS proxies.
> 
> This removes the default 5 second socks connect timeout
> which can be too small depending on network setup and
> replaces it with the configurable overall connect timeout
> (default 120 seconds).
> 
> Trac: #328
> Change-Id: I2b109f8c551c23045a1be355778b08f0fd4d309f
> Signed-off-by: 5andr0 <sandro.trianni@gmail.com>
> ---
>  src/openvpn/socket.c |  2 ++
>  src/openvpn/socks.c  | 25 ++++++++++++++-----------
>  src/openvpn/socks.h  |  2 ++
>  3 files changed, 18 insertions(+), 11 deletions(-)
> 
> Trying to move this forward by adding a commit message.

Acked-by: Frank Lichtenheld <frank@lichtenheld.com>

Code looks good and I tested build and default t_client tests.
However, not sure how exactly to verify that it actually works.
The SOCKS proxy I have doesn't exhibit any problems even with
--connect-timeout 1.

Any ideas for testing welcome.

Regards,
Gert Doering March 20, 2024, 5:06 p.m. UTC | #2
Hi,

On Fri, Mar 15, 2024 at 05:40:02PM +0100, Frank Lichtenheld wrote:
> Code looks good and I tested build and default t_client tests.
> However, not sure how exactly to verify that it actually works.
> The SOCKS proxy I have doesn't exhibit any problems even with
> --connect-timeout 1.
> 
> Any ideas for testing welcome.

As far as I understand this fix, it's replacing the 5s hard coded timeout
for "handshake with the SOCKS proxy" with --connect-timeout.

So to exhibit any change in behaviour, you need a slow SOCKs proxy...

I have a few ideas how to test this, like

 - --socks-proxy <non-answering-ip>:10080
   (should fail after 5 seconds without the patch, after --connect-timeout
   with the patch)

 - hack "something that can speak SOCKS" (like, ssh) to be really slow
   in answering - like, "add a sleep(15) in the socks handshake path",
   and point OpenVPN to that.  Without the patch, this should fail, with
   the patch, it should succeeds.

   (Note: 'ssh -D nnnn' will do SOCKS, but not UDP -> TCP server needed)

I have no time just now to go hacking on this, so just putting out the
ideas...

For our regular testbed, testing all these timeouts is complicated
(and even if you succeed, it's sllloowww if you want reproduceable
results, so not "milliseconds timers").

gert
Arne Schwabe March 21, 2024, 4:38 p.m. UTC | #3
Am 20.03.24 um 13:06 schrieb Gert Doering:
> Hi,
> 
> On Fri, Mar 15, 2024 at 05:40:02PM +0100, Frank Lichtenheld wrote:
>> Code looks good and I tested build and default t_client tests.
>> However, not sure how exactly to verify that it actually works.
>> The SOCKS proxy I have doesn't exhibit any problems even with
>> --connect-timeout 1.
>>
>> Any ideas for testing welcome.
> 
> As far as I understand this fix, it's replacing the 5s hard coded timeout
> for "handshake with the SOCKS proxy" with --connect-timeout.
> 
> So to exhibit any change in behaviour, you need a slow SOCKs proxy...
> 
> I have a few ideas how to test this, like
> 
>   - --socks-proxy <non-answering-ip>:10080
>     (should fail after 5 seconds without the patch, after --connect-timeout
>     with the patch)
> 
>   - hack "something that can speak SOCKS" (like, ssh) to be really slow
>     in answering - like, "add a sleep(15) in the socks handshake path",
>     and point OpenVPN to that.  Without the patch, this should fail, with
>     the patch, it should succeeds.
> 
>     (Note: 'ssh -D nnnn' will do SOCKS, but not UDP -> TCP server needed)
> 
> I have no time just now to go hacking on this, so just putting out the
> ideas...
> 
> For our regular testbed, testing all these timeouts is complicated
> (and even if you succeed, it's sllloowww if you want reproduceable
> results, so not "milliseconds timers").
> 

Or use something like dummynet. At least on macOS it used to be easy to 
simulate a long delay with that and pfctl.

Arne
Gert Doering June 19, 2024, 9:55 a.m. UTC | #4
Took me long enough, but now it's in :-) - thanks, and thanks ValdikSS
for reporting test success.

I've run this on the server side test bed (which will not excercise SOCKS
paths, but to verify that "nothing unrelated got hit") and on the client,
with a few SOCKs proxy tests.  These are all "fast proxies" so do not
excercise the new timeout handling - and I did not find time to build
something with dummynet to make it actually test on a "really slow proxy".

Testing using an unreachable IP didn't yield proper results (because
the TCP connect is still goverened by the "base" timeout), so you
need something which answers TCP/1080, and then "is slow"...

Testing using a "dead" netcat 'socks server' ("nc -k -l 1080") leads to

  - old code: give up after 5 seconds ("TCP port read timeout"), always
  - new code: respect --connect-timeout (shorter or longer than 5s)

Your patch has been applied to the master and release/2.6 branch
(I consider it a bugfix, and while the code touches a few places, it
only really changes very localized socks.c code).

commit b3a68b85a729628ca8b97f9f0c2813f795289cfc (master)
commit 94bfb712366ece1ca3605d18e99580f482f0232b (release/2.6)
Author: 5andr0
Date:   Fri Mar 15 17:20:11 2024 +0100

     Implement server_poll_timeout for socks

     Signed-off-by: 5andr0 <sandro.trianni@gmail.com>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20240315162011.1661139-1-frank@lichtenheld.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28408.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 480f4e51..ecb408a3 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2075,6 +2075,7 @@  phase2_tcp_client(struct link_socket *sock, struct signal_info *sig_info)
                                            sock->sd,
                                            sock->proxy_dest_host,
                                            sock->proxy_dest_port,
+                                           sock->server_poll_timeout,
                                            sig_info);
         }
         if (proxy_retry)
@@ -2104,6 +2105,7 @@  phase2_socks_client(struct link_socket *sock, struct signal_info *sig_info)
                                    sock->ctrl_sd,
                                    sock->sd,
                                    &sock->socks_relay.dest,
+                                   sock->server_poll_timeout,
                                    sig_info);
 
     if (sig_info->signal_received)
diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index a29eb83a..2cb83a66 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -42,6 +42,7 @@ 
 #include "fdmisc.h"
 #include "misc.h"
 #include "proxy.h"
+#include "forward.h"
 
 #include "memdbg.h"
 
@@ -85,12 +86,12 @@  socks_proxy_close(struct socks_proxy_info *sp)
 static bool
 socks_username_password_auth(struct socks_proxy_info *p,
                              socket_descriptor_t sd,
+                             struct event_timeout *server_poll_timeout,
                              volatile int *signal_received)
 {
     char to_send[516];
     char buf[2];
     int len = 0;
-    const int timeout_sec = 5;
     struct user_pass creds;
     ssize_t size;
     bool ret = false;
@@ -129,7 +130,7 @@  socks_username_password_auth(struct socks_proxy_info *p,
 
         FD_ZERO(&reads);
         openvpn_fd_set(sd, &reads);
-        tv.tv_sec = timeout_sec;
+        tv.tv_sec = get_server_poll_remaining_time(server_poll_timeout);
         tv.tv_usec = 0;
 
         status = select(sd + 1, &reads, NULL, NULL, &tv);
@@ -185,11 +186,11 @@  cleanup:
 static bool
 socks_handshake(struct socks_proxy_info *p,
                 socket_descriptor_t sd,
+                struct event_timeout *server_poll_timeout,
                 volatile int *signal_received)
 {
     char buf[2];
     int len = 0;
-    const int timeout_sec = 5;
     ssize_t size;
 
     /* VER = 5, NMETHODS = 1, METHODS = [0 (no auth)] */
@@ -216,7 +217,7 @@  socks_handshake(struct socks_proxy_info *p,
 
         FD_ZERO(&reads);
         openvpn_fd_set(sd, &reads);
-        tv.tv_sec = timeout_sec;
+        tv.tv_sec = get_server_poll_remaining_time(server_poll_timeout);
         tv.tv_usec = 0;
 
         status = select(sd + 1, &reads, NULL, NULL, &tv);
@@ -283,7 +284,7 @@  socks_handshake(struct socks_proxy_info *p,
                 return false;
             }
 
-            if (!socks_username_password_auth(p, sd, signal_received))
+            if (!socks_username_password_auth(p, sd, server_poll_timeout, signal_received))
             {
                 return false;
             }
@@ -301,13 +302,13 @@  socks_handshake(struct socks_proxy_info *p,
 static bool
 recv_socks_reply(socket_descriptor_t sd,
                  struct openvpn_sockaddr *addr,
+                 struct event_timeout *server_poll_timeout,
                  volatile int *signal_received)
 {
     char atyp = '\0';
     int alen = 0;
     int len = 0;
     char buf[270];              /* 4 + alen(max 256) + 2 */
-    const int timeout_sec = 5;
 
     if (addr != NULL)
     {
@@ -326,7 +327,7 @@  recv_socks_reply(socket_descriptor_t sd,
 
         FD_ZERO(&reads);
         openvpn_fd_set(sd, &reads);
-        tv.tv_sec = timeout_sec;
+        tv.tv_sec = get_server_poll_remaining_time(server_poll_timeout);
         tv.tv_usec = 0;
 
         status = select(sd + 1, &reads, NULL, NULL, &tv);
@@ -451,12 +452,13 @@  establish_socks_proxy_passthru(struct socks_proxy_info *p,
                                socket_descriptor_t sd,  /* already open to proxy */
                                const char *host,        /* openvpn server remote */
                                const char *servname,    /* openvpn server port */
+                               struct event_timeout *server_poll_timeout,
                                struct signal_info *sig_info)
 {
     char buf[270];
     size_t len;
 
-    if (!socks_handshake(p, sd, &sig_info->signal_received))
+    if (!socks_handshake(p, sd, server_poll_timeout, &sig_info->signal_received))
     {
         goto error;
     }
@@ -494,7 +496,7 @@  establish_socks_proxy_passthru(struct socks_proxy_info *p,
 
 
     /* receive reply from Socks proxy and discard */
-    if (!recv_socks_reply(sd, NULL, &sig_info->signal_received))
+    if (!recv_socks_reply(sd, NULL, server_poll_timeout, &sig_info->signal_received))
     {
         goto error;
     }
@@ -512,9 +514,10 @@  establish_socks_proxy_udpassoc(struct socks_proxy_info *p,
                                socket_descriptor_t ctrl_sd,  /* already open to proxy */
                                socket_descriptor_t udp_sd,
                                struct openvpn_sockaddr *relay_addr,
+                               struct event_timeout *server_poll_timeout,
                                struct signal_info *sig_info)
 {
-    if (!socks_handshake(p, ctrl_sd, &sig_info->signal_received))
+    if (!socks_handshake(p, ctrl_sd, server_poll_timeout, &sig_info->signal_received))
     {
         goto error;
     }
@@ -535,7 +538,7 @@  establish_socks_proxy_udpassoc(struct socks_proxy_info *p,
 
     /* receive reply from Socks proxy */
     CLEAR(*relay_addr);
-    if (!recv_socks_reply(ctrl_sd, relay_addr, &sig_info->signal_received))
+    if (!recv_socks_reply(ctrl_sd, relay_addr, server_poll_timeout, &sig_info->signal_received))
     {
         goto error;
     }
diff --git a/src/openvpn/socks.h b/src/openvpn/socks.h
index 3a89245b..a7094f06 100644
--- a/src/openvpn/socks.h
+++ b/src/openvpn/socks.h
@@ -52,12 +52,14 @@  void establish_socks_proxy_passthru(struct socks_proxy_info *p,
                                     socket_descriptor_t sd,  /* already open to proxy */
                                     const char *host,        /* openvpn server remote */
                                     const char *servname,          /* openvpn server port */
+                                    struct event_timeout *server_poll_timeout,
                                     struct signal_info *sig_info);
 
 void establish_socks_proxy_udpassoc(struct socks_proxy_info *p,
                                     socket_descriptor_t ctrl_sd,  /* already open to proxy */
                                     socket_descriptor_t udp_sd,
                                     struct openvpn_sockaddr *relay_addr,
+                                    struct event_timeout *server_poll_timeout,
                                     struct signal_info *sig_info);
 
 void socks_process_incoming_udp(struct buffer *buf,