[Openvpn-devel,2/2] Avoid sending push request after receving push reply

Message ID 20200725234803.22058-2-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,1/2] Simplify calling logic of check_connection_established_dowork
Related show

Commit Message

Arne Schwabe July 25, 2020, 11:48 p.m.
The introduction of IV_PROTO_REQUEST_PUSH (c290df55) sometimes causes the
server to reply before we setup the push timer. The push reply will then clear
a timer that has not been setup yet. We then start sending push
request after we have gone through the whole initialisation already.

This patch also clears the connestion_established timer that sets up the
push request timer. This lead to the

management_set_state(management,  OPENVPN_STATE_GET_CONFIG, ...)

function not being called. But a to display "waiting for configuration..." or
sending a "getting config state" after "initialisation" does not make sense
anyway.

Also add the IV_PROTO_REQUEST_PUSH feature as new feature in Changes.rst

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst           | 11 +++++++++++
 src/openvpn/forward.c |  3 +++
 src/openvpn/push.c    |  1 +
 3 files changed, 15 insertions(+)

Comments

tincanteksup July 26, 2020, 1:13 p.m. | #1
a little help..

On 26/07/2020 00:48, Arne Schwabe wrote:
> The introduction of IV_PROTO_REQUEST_PUSH (c290df55) sometimes causes the
> server to reply before we setup the push timer. The push reply will then clear
> a timer that has not been setup yet. We then start sending push
> request after we have gone through the whole initialisation already.
> 
> This patch also clears the connestion_established timer that sets up the
> push request timer. This lead to the
> 
> management_set_state(management,  OPENVPN_STATE_GET_CONFIG, ...)
> 
> function not being called. But a to display "waiting for configuration..." or

I think this was meant to read as (moved 'a'):

But to display a "waiting for configuration..."


> sending a "getting config state" after "initialisation" does not make sense
> anyway.
> 
> Also add the IV_PROTO_REQUEST_PUSH feature as new feature in Changes.rst
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   Changes.rst           | 11 +++++++++++
>   src/openvpn/forward.c |  3 +++
>   src/openvpn/push.c    |  1 +
>   3 files changed, 15 insertions(+)
> 
> diff --git a/Changes.rst b/Changes.rst
> index 8fbaf419..e377a36c 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -37,6 +37,13 @@ Deferred client-connect
>       asynchronous/deferred return of the configuration file in the same way
>       as the auth-plugin.
>   
> +Faster connection setup
> +    A client will signal in the ``IV_PROTO`` variable that is in pull

I think this is meant to read:
variable that is in pull ->
variable that it is in pull


> +    mode. This allows the server to push the configuration options to
> +    the client without waiting for a ``PULL_REQUEST`` message. The feature
> +    is automatically enabled if both client and server support it and
> +    reduces the of connection setup time by one round-trip time.
> +
>   Deprecated features
>   -------------------
>   For an up-to-date list of all deprecated options, see this wiki page:
> @@ -72,6 +79,10 @@ User-visible Changes
>   - Support for building with OpenSSL 1.0.1 has been removed. The minimum
>     supported OpenSSL version is now 1.0.2.
>   
> +- The GET_CONFIG management state is ommited if the server pushes

ommited -> ommitted


> +  the client configuration almost immediately as result of the
> +  faster connection setup feature.
> +
>   
>   Overview of changes in 2.4
>   ==========================
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 30a3fd46..759fdbe1 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -425,6 +425,9 @@ check_push_request_dowork(struct context *c)
>    *
>    * Options like --up-delay need to be triggered by this function which
>    * checks for connection establishment.
> + *
> + * Note: The process_incoming_push_reply currently assumes that this function
> + * only sets ups the pull request timer when pull is enabled.

ups -> up


>    */
>   void
>   check_connection_established(struct context *c)
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 84193afe..9c720b42 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -358,6 +358,7 @@ incoming_push_message(struct context *c, const struct buffer *buffer)
>               }
>           }
>           event_timeout_clear(&c->c2.push_request_interval);
> +        event_timeout_clear(&c->c2.wait_for_connect);
>       }
>   
>       goto cleanup;
>
Gert Doering July 26, 2020, 4:56 p.m. | #2
Acked-by: Gert Doering <gert@greenie.muc.de>

This is magic :-) - it's just a one liner (plus lots of documentation)
but the description makes sense, staring at the code for a while also
makes sense.  Plus, it works :-)

I have not tested the server side, as (to my understanding) this is 
basically "client side only" code.  I *have* tested p2p with --tls-client
(now! part of my t_client rig :-) ) and that still works - relevant, as
the code flows along the "established?  send pull?" path here.

The new code also works correctly with pre-fast-pull servers (the
"reference server" is - because I'm lazy - still running an older 
version).

I have merged the typo fixes from Richard as well (note: ommitted has
only one m!  I asked google!), and done a bit more bragging in the
Changes.rst wording ("significantly! improves connection time!").

Your patch has been applied to the master branch.

commit a3b21a76b87fedf045c409481f55c34486d8cd27
Author: Arne Schwabe
Date:   Sun Jul 26 01:48:03 2020 +0200

     Avoid sending push request after receving push reply

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200725234803.22058-2-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20589.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/Changes.rst b/Changes.rst
index 8fbaf419..e377a36c 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -37,6 +37,13 @@  Deferred client-connect
     asynchronous/deferred return of the configuration file in the same way
     as the auth-plugin.
 
+Faster connection setup
+    A client will signal in the ``IV_PROTO`` variable that is in pull
+    mode. This allows the server to push the configuration options to
+    the client without waiting for a ``PULL_REQUEST`` message. The feature
+    is automatically enabled if both client and server support it and
+    reduces the of connection setup time by one round-trip time.
+
 Deprecated features
 -------------------
 For an up-to-date list of all deprecated options, see this wiki page:
@@ -72,6 +79,10 @@  User-visible Changes
 - Support for building with OpenSSL 1.0.1 has been removed. The minimum
   supported OpenSSL version is now 1.0.2.
 
+- The GET_CONFIG management state is ommited if the server pushes
+  the client configuration almost immediately as result of the
+  faster connection setup feature.
+
 
 Overview of changes in 2.4
 ==========================
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 30a3fd46..759fdbe1 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -425,6 +425,9 @@  check_push_request_dowork(struct context *c)
  *
  * Options like --up-delay need to be triggered by this function which
  * checks for connection establishment.
+ *
+ * Note: The process_incoming_push_reply currently assumes that this function
+ * only sets ups the pull request timer when pull is enabled.
  */
 void
 check_connection_established(struct context *c)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 84193afe..9c720b42 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -358,6 +358,7 @@  incoming_push_message(struct context *c, const struct buffer *buffer)
             }
         }
         event_timeout_clear(&c->c2.push_request_interval);
+        event_timeout_clear(&c->c2.wait_for_connect);
     }
 
     goto cleanup;