diff mbox series

[Openvpn-devel,1/1] Start TLS after connection established without waiting.

Message ID CAL3L06G=w44OuBMUbV1YJSZ9vKb5oJOkwDbCMjPH3ALFSP+cXw@mail.gmail.com
State Changes Requested
Headers show
Series [Openvpn-devel,1/1] Start TLS after connection established without waiting. | expand

Commit Message

Daniel Kaldor July 24, 2019, 1:41 a.m. UTC 
There is a ~1s delay between establishing connection with remote
server and starting TLS handshake.
This change removes delay and improves connection time.

---
 src/openvpn/forward.c | 61 +++++++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

--
2.9.3 (Apple Git-75)

Comments

Antonio Quartulli July 24, 2019, 1:46 a.m. UTC | #1 
Hi,

this patch has been mangled by your e-mail client.

Could you please re-send it using git send-email?

Thanks a lot

On 24/07/2019 13:41, Daniel Kaldor wrote:
> There is a ~1s delay between establishing connection with remote
> server and starting TLS handshake.
> This change removes delay and improves connection time.
> 
> ---
>  src/openvpn/forward.c | 61 +++++++++++++++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 35df089..2deaf93 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -433,28 +433,6 @@ check_connection_established_dowork(struct context *c)
>      {
>          if (CONNECTION_ESTABLISHED(c))
>          {
> -#if P2MP
> -            /* if --pull was specified, send a push request to server */
> -            if (c->c2.tls_multi && c->options.pull)
> -            {
> -#ifdef ENABLE_MANAGEMENT
> -                if (management)
> -                {
> -                    management_set_state(management,
> -                                         OPENVPN_STATE_GET_CONFIG,
> -                                         NULL,
> -                                         NULL,
> -                                         NULL,
> -                                         NULL,
> -                                         NULL);
> -                }
> -#endif
> -                /* fire up push request right away (already 1s delayed) */
> -                event_timeout_init(&c->c2.push_request_interval, 0, now);
> -                reset_coarse_timers(c);
> -            }
> -            else
> -#endif /* if P2MP */
>              {
>                  do_up(c, false, 0);
>              }
> @@ -1943,17 +1921,34 @@ pre_select(struct context *c)
>          }
>      }
>  #endif
> -
> -    /* check coarse timers? */
> -    check_coarse_timers(c);
> -    if (c->sig->signal_received)
> -    {
> -        return;
> -    }
> +
> +    bool pre_connection_state = CONNECTION_ESTABLISHED(c);
> 
>      /* Does TLS need service? */
>      check_tls(c);
> 
> +    bool post_connection_state = CONNECTION_ESTABLISHED(c);
> +
> +    if(!pre_connection_state && post_connection_state){
> +
> +        if (c->c2.tls_multi && c->options.pull)
> +        {
> +#ifdef ENABLE_MANAGEMENT
> +            if (management)
> +            {
> +                    management_set_state(management,
> +                                         OPENVPN_STATE_GET_CONFIG,
> +                                         NULL,
> +                                         NULL,
> +                                         NULL,
> +                                         NULL,
> +                                         NULL);
> +            }
> +#endif
> +            check_push_request_dowork(c);
> +        }
> +    }
> +
>      /* In certain cases, TLS errors will require a restart */
>      check_tls_errors(c);
>      if (c->sig->signal_received)
> @@ -1961,6 +1956,14 @@ pre_select(struct context *c)
>          return;
>      }
> 
> +    /* check coarse timers */
> +    check_coarse_timers(c);
> +    if (c->sig->signal_received)
> +    {
> +        return;
> +    }
> +
> +
>      /* check for incoming configuration info on the control channel */
>      check_incoming_control_channel(c);
> 
> --
> 2.9.3 (Apple Git-75)
> 
> 
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
Gert Doering July 24, 2019, 1:57 a.m. UTC | #2 
Hi,

On Wed, Jul 24, 2019 at 01:46:36PM +0200, Antonio Quartulli wrote:
> this patch has been mangled by your e-mail client.
> 
> Could you please re-send it using git send-email?

That seems to have been a false positive on your side... git applies
this patch just fine, so at least here it arrived without mangling
(= nothing wrong on the sender side).

gert
Antonio Quartulli July 24, 2019, 1:57 a.m. UTC | #3 
Hi,

On 24/07/2019 13:57, Gert Doering wrote:
> Hi,
> 
> On Wed, Jul 24, 2019 at 01:46:36PM +0200, Antonio Quartulli wrote:
>> this patch has been mangled by your e-mail client.
>>
>> Could you please re-send it using git send-email?
> 
> That seems to have been a false positive on your side... git applies
> this patch just fine, so at least here it arrived without mangling
> (= nothing wrong on the sender side).
> 

ops, sorry then. It probably was my local parser (used to highlight
diffs) that failed.

Cheers,
Steffan Karger Aug. 21, 2019, 3:56 a.m. UTC | #4 
Hi,

On 24-07-19 13:41, Daniel Kaldor wrote:
> There is a ~1s delay between establishing connection with remote
> server and starting TLS handshake.
> This change removes delay and improves connection time.

While this sounds like something we'd like to have (i.e, feature-ACK),
this doesn't really explain why this change resolves the issue, nor why
this is the right fix.

Can you please clarify what it changes in the connection sequence and
why you believe this is the right way to get rid of the 1s second delay?

Also, how did you test this patch? (TCP, UDP, with or without
management, on fast/slow links, even running in production maybe? Etc.)

Thanks,
-Steffan
Gert Doering Aug. 21, 2019, 9:09 a.m. UTC | #5 
Hi,

On Wed, Aug 21, 2019 at 03:56:50PM +0200, Steffan Karger wrote:
> While this sounds like something we'd like to have (i.e, feature-ACK),
> this doesn't really explain why this change resolves the issue, nor why
> this is the right fix.

As a bit of background why I assume this will fix "things" - we have a
few places in the connection sequence where we do something, and when
it's finished, set up a coarse timer to "go on with the next step" -
which always means "1 second delay" because that's how the coarse
timers work.

I've dug into this a bit when figuring out commit afb93fac803fbab7 -
which helped a bit (this was setting up an "in 1 second" coarse timer, 
which always got an extra second due to scheduling), but I'm sure there
is more room for improvement.

OTOH this is all tricky code, with client and server and tcp and udp
all intermixed.

gert
diff mbox series

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 35df089..2deaf93 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -433,28 +433,6 @@  check_connection_established_dowork(struct context *c)
     {
         if (CONNECTION_ESTABLISHED(c))
         {
-#if P2MP
-            /* if --pull was specified, send a push request to server */
-            if (c->c2.tls_multi && c->options.pull)
-            {
-#ifdef ENABLE_MANAGEMENT
-                if (management)
-                {
-                    management_set_state(management,
-                                         OPENVPN_STATE_GET_CONFIG,
-                                         NULL,
-                                         NULL,
-                                         NULL,
-                                         NULL,
-                                         NULL);
-                }
-#endif
-                /* fire up push request right away (already 1s delayed) */
-                event_timeout_init(&c->c2.push_request_interval, 0, now);
-                reset_coarse_timers(c);
-            }
-            else
-#endif /* if P2MP */
             {
                 do_up(c, false, 0);
             }
@@ -1943,17 +1921,34 @@  pre_select(struct context *c)
         }
     }
 #endif
-
-    /* check coarse timers? */
-    check_coarse_timers(c);
-    if (c->sig->signal_received)
-    {
-        return;
-    }
+
+    bool pre_connection_state = CONNECTION_ESTABLISHED(c);

     /* Does TLS need service? */
     check_tls(c);

+    bool post_connection_state = CONNECTION_ESTABLISHED(c);
+
+    if(!pre_connection_state && post_connection_state){
+
+        if (c->c2.tls_multi && c->options.pull)
+        {
+#ifdef ENABLE_MANAGEMENT
+            if (management)
+            {
+                    management_set_state(management,
+                                         OPENVPN_STATE_GET_CONFIG,
+                                         NULL,
+                                         NULL,
+                                         NULL,
+                                         NULL,
+                                         NULL);
+            }
+#endif
+            check_push_request_dowork(c);
+        }
+    }
+
     /* In certain cases, TLS errors will require a restart */
     check_tls_errors(c);
     if (c->sig->signal_received)
@@ -1961,6 +1956,14 @@  pre_select(struct context *c)
         return;
     }

+    /* check coarse timers */
+    check_coarse_timers(c);
+    if (c->sig->signal_received)
+    {
+        return;
+    }
+
+
     /* check for incoming configuration info on the control channel */
     check_incoming_control_channel(c);