| 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 | expand |
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" > >
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.
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
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"