[Openvpn-devel,1/9,v3] Indicate that a client is in pull mode in IV_PROTO

Message ID 20200721163811.22745-1-arne@rfc2549.org
State Accepted
Delegated to: Gert Doering
Headers show
Series
  • [Openvpn-devel,1/9,v3] Indicate that a client is in pull mode in IV_PROTO
Related show

Commit Message

Arne Schwabe July 21, 2020, 4:38 p.m.
This allows us to skip waiting for the first PUSH_REQUEST message from
the client to send the response.

This changes the interpretation of IV_PROTO from a scalar to a bitfield
Since we only have IV_PROTO=2 defined so far and will support DATA_V2
this should not make any problem. This avoid adding another IV_xxx variable
that takes valuable space in the protocol frame.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.

Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
          by tincanteksup.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/man-sections/server-options.rst | 10 ++++++++--
 src/openvpn/multi.c                 | 12 ++++++++++--
 src/openvpn/ssl.c                   | 15 +++++++++++++--
 src/openvpn/ssl.h                   | 16 ++++++++++++++++
 4 files changed, 47 insertions(+), 6 deletions(-)

Comments

tincanteksup July 21, 2020, 7:34 p.m. | #1
1x spelling 1x grammar

On 21/07/2020 17:38, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> This changes the interpretation of IV_PROTO from a scalar to a bitfield
> Since we only have IV_PROTO=2 defined so far and will support DATA_V2
> this should not make any problem. This avoid adding another IV_xxx variable
> that takes valuable space in the protocol frame.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.
> 
> Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
>            by tincanteksup.

incoperate -> incorporate =]

Please feel free use my full details:
Richard Bonhomme <tincanteksup@gmail.com>

> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   doc/man-sections/server-options.rst | 10 ++++++++--
>   src/openvpn/multi.c                 | 12 ++++++++++--
>   src/openvpn/ssl.c                   | 15 +++++++++++++--
>   src/openvpn/ssl.h                   | 16 ++++++++++++++++
>   4 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
> index c8e9fc61..babf33d3 100644
> --- a/doc/man-sections/server-options.rst
> +++ b/doc/man-sections/server-options.rst
> @@ -464,8 +464,14 @@ fast hardware. SSL/TLS authentication must be used in this mode.
>     :code:`IV_LZ4=1`
>           If the client supports LZ4 compressions.
>   
> -  :code:`IV_PROTO=2`
> -        If the client supports peer-id floating mechanism
> +  :code:`IV_PROTO`
> +    Details about protocol extensions that the peer supports. The
> +    variable is a bitfield and the bits are defined as follows
> +    (starting a bit 0 for the first (unused) bit:
> +
> +    - bit 1: The peer supports peer-id floating mechanism
> +    - bit 2: The client expects a push-reply and the server may
> +      send this reply without waiting for a push-request first.
>   
>     :code:`IV_NCP=2`
>           Negotiable ciphers, client supports ``--cipher`` pushed by
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 2ae9c03e..b83a0f06 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1792,10 +1792,18 @@ multi_client_set_protocol_options(struct context *c)
>       {
>           int proto = 0;
>           int r = sscanf(optstr, "IV_PROTO=%d", &proto);
> -        if ((r == 1) && (proto >= 2))
> +        if (r == 1)
>           {
> -            tls_multi->use_peer_id = true;
> +            if (proto & IV_PROTO_DATA_V2)
> +            {
> +                tls_multi->use_peer_id = true;
> +            }
> +            if (proto & IV_PROTO_REQUEST_PUSH)
> +            {
> +                c->c2.push_request_received = true;
> +            }
>           }
> +
>       }
>   
>       /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>           buf_printf(&out, "IV_PLAT=win\n");
>   #endif
>   
> -        /* support for P_DATA_V2 */
> -        buf_printf(&out, "IV_PROTO=2\n");
> +        /* support for P_DATA_V2*/
> +        int iv_proto = IV_PROTO_DATA_V2;
> +
> +        /* support for receiving push_reply before sending
> +         * push request, also signal that the client wants
> +         * to get push-reply messages without without requiring a round
> +         * trip for a push request message*/
> +        if(session->opt->pull)
> +        {
> +            iv_proto |= IV_PROTO_REQUEST_PUSH;
> +        }
> +
> +        buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
>   
>           /* support for Negotiable Crypto Parameters */
>           if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..0da16974 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,22 @@
>   /* Maximum length of OCC options string passed as part of auth handshake */
>   #define TLS_OPTIONS_LEN 512
>   
> +/* Definitions of the bits in the IV_PROTO bitfield
> + *
> + * In older OpenVPN versions this used in a comparison

In older OpenVPN versions this used in a comparison ->
In older OpenVPN versions this is used in a comparison


> + * IV_PROTO >= 2 to determine if DATA_V2 is supported.
> + * Therefore any client announcing any of the flags must
> + * also announce IV_PROTO_DATA_V2. We also treat bit 0
> + * as reserved for this reason */
> +
> +/** Support P_DATA_V2 */
> +#define IV_PROTO_DATA_V2        (1<<1)
> +
> +/** Assume client will send a push request and server does not need
> + * to wait for a push-request to send a push-reply */
> +#define IV_PROTO_REQUEST_PUSH   (1<<2)
> +
> +
>   /* Default field in X509 to be username */
>   #define X509_USERNAME_FIELD_DEFAULT "CN"
>   
>
Antonio Quartulli July 21, 2020, 7:40 p.m. | #2
Hi,

On 21/07/2020 18:38, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
> 
> This changes the interpretation of IV_PROTO from a scalar to a bitfield
> Since we only have IV_PROTO=2 defined so far and will support DATA_V2
> this should not make any problem. This avoid adding another IV_xxx variable
> that takes valuable space in the protocol frame.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> Patch V2: Use bitmask for IV_PROTO_DATA_V2 and add more documentation.
> 
> Patch V3: Rewrite IV_PROTO paragraph in man page, incoperate spelling fixes
>           by tincanteksup.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>


Thanks a lot for all the fixes. Looks good to me now.
Gert Doering July 22, 2020, 9:01 a.m. | #3
Acked-by: Gert Doering <gert@greenie.muc.de>

Stared at code, loooks good.

Tested the client side against an unpatched server -> works (which is not 
surprising, but worth a test).

Then patched the server, tested the patched client again -> works \o/

Jul 22 10:09:15 gentoo tun-tcp-p2mp[5572]: 194.97.140.21:50479 peer info: IV_PROTO=6
Jul 22 10:09:15 gentoo tun-tcp-p2mp[5572]: ... SENT CONTROL [cron2-freebsd-tc-amd64]: 'PUSH_REPLY,...'
Jul 22 10:09:17 gentoo tun-tcp-p2mp[5572]: ... PUSH: Received control message: 'PUSH_REQUEST'

(saves 1-2 seconds - coarse timers on the client side...)


The client side state machine is not truly convincing me yet, though - 
while this works as it is, the client does not properly register that 
it already *has* a PUSH_REPLY, so keeps sending PUSH_REQUESTS until the 
server gives in after 30s (NO-SOUP debounce timer) and sends another 
PUSH_REPLY:

2020-07-22 10:14:26 [server] Peer Connection Initiated with [AF_INET6]2001:608:0:814::f000:11:51198
2020-07-22 10:14:26 PUSH: Received control message: 'PUSH_REPLY,route 10.204.0.0 ...'
[tun/route init stuff, crypto init, ...]
2020-07-22 10:14:26 Initialization Sequence Completed
2020-07-22 10:14:27 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:32 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:37 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:42 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:47 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:52 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:57 SENT CONTROL [server]: 'PUSH_REQUEST' (status=1)
2020-07-22 10:14:57 PUSH: Received control message: 'PUSH_REPLY,route 10.204.0.0 ...




I decided to keep the patch and improve on the client side 
later :-) - and *then* ran the full test with 2.2/2.3/2.4/master(patched) 
against the same server, and AGES later... 

22...
Test sets succeeded: 1 2 3 4 6 8.
Test sets failed: none.
..
master...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 2f 3 4 5 5a 5b 5c 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 8 8a 9 4b.
Test sets failed: none.


My RTTs are too low to make a difference wrt packets RTTs, but the
"coarse timer on client" delay alone makes this an improvement.

Your patch has been applied to the master branch.

commit c290df558f8df995f1eeb3ddbf408508a455273e
Author: Arne Schwabe
Date:   Tue Jul 21 18:38:11 2020 +0200

     Indicate that a client is in pull mode in IV_PROTO

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


--
kind regards,

Gert Doering

Patch

diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index c8e9fc61..babf33d3 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -464,8 +464,14 @@  fast hardware. SSL/TLS authentication must be used in this mode.
   :code:`IV_LZ4=1`
         If the client supports LZ4 compressions.
 
-  :code:`IV_PROTO=2`
-        If the client supports peer-id floating mechanism
+  :code:`IV_PROTO`
+    Details about protocol extensions that the peer supports. The
+    variable is a bitfield and the bits are defined as follows
+    (starting a bit 0 for the first (unused) bit:
+
+    - bit 1: The peer supports peer-id floating mechanism
+    - bit 2: The client expects a push-reply and the server may
+      send this reply without waiting for a push-request first.
 
   :code:`IV_NCP=2`
         Negotiable ciphers, client supports ``--cipher`` pushed by
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 2ae9c03e..b83a0f06 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1792,10 +1792,18 @@  multi_client_set_protocol_options(struct context *c)
     {
         int proto = 0;
         int r = sscanf(optstr, "IV_PROTO=%d", &proto);
-        if ((r == 1) && (proto >= 2))
+        if (r == 1)
         {
-            tls_multi->use_peer_id = true;
+            if (proto & IV_PROTO_DATA_V2)
+            {
+                tls_multi->use_peer_id = true;
+            }
+            if (proto & IV_PROTO_REQUEST_PUSH)
+            {
+                c->c2.push_request_received = true;
+            }
         }
+
     }
 
     /* Select cipher if client supports Negotiable Crypto Parameters */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 54a23011..04d78a81 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2299,8 +2299,19 @@  push_peer_info(struct buffer *buf, struct tls_session *session)
         buf_printf(&out, "IV_PLAT=win\n");
 #endif
 
-        /* support for P_DATA_V2 */
-        buf_printf(&out, "IV_PROTO=2\n");
+        /* support for P_DATA_V2*/
+        int iv_proto = IV_PROTO_DATA_V2;
+
+        /* support for receiving push_reply before sending
+         * push request, also signal that the client wants
+         * to get push-reply messages without without requiring a round
+         * trip for a push request message*/
+        if(session->opt->pull)
+        {
+            iv_proto |= IV_PROTO_REQUEST_PUSH;
+        }
+
+        buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
 
         /* support for Negotiable Crypto Parameters */
         if (session->opt->ncp_enabled
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 58a9b0d4..0da16974 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -99,6 +99,22 @@ 
 /* Maximum length of OCC options string passed as part of auth handshake */
 #define TLS_OPTIONS_LEN 512
 
+/* Definitions of the bits in the IV_PROTO bitfield
+ *
+ * In older OpenVPN versions this used in a comparison
+ * IV_PROTO >= 2 to determine if DATA_V2 is supported.
+ * Therefore any client announcing any of the flags must
+ * also announce IV_PROTO_DATA_V2. We also treat bit 0
+ * as reserved for this reason */
+
+/** Support P_DATA_V2 */
+#define IV_PROTO_DATA_V2        (1<<1)
+
+/** Assume client will send a push request and server does not need
+ * to wait for a push-request to send a push-reply */
+#define IV_PROTO_REQUEST_PUSH   (1<<2)
+
+
 /* Default field in X509 to be username */
 #define X509_USERNAME_FIELD_DEFAULT "CN"