Re: [Openvpn-devel] [PATCH 1/9] Indicate that a client is in pull mode in IV_PROTO
Hi, On 17-07-2020 15:47, Arne Schwabe wrote: > This allows us to skip waiting for the first PUSH_REQUEST message from > the client to send the response. Feature-ACK, clever use of existing infra. Some comments though: This commit message could use a bit more information. In particular, it would be good to explain how the various OpenVPN versions currently interpret IV_PROTO values, and why the change from a version-like interpretation to a bitfield will not cause issues. > Signed-off-by: Arne Schwabe > --- > src/openvpn/multi.c | 12 ++-- > src/openvpn/ssl.c | 15 +-- > src/openvpn/ssl.h | 7 +++ > 3 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 0095c871..88ba9db2 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c) > { > int proto = 0; > int r = sscanf(optstr, "IV_PROTO=%d", ); > -if ((r == 1) && (proto >= 2)) > +if (r == 1) > { > -tls_multi->use_peer_id = true; > +if (proto >= 2) > +{ > +tls_multi->use_peer_id = true; > +} Wouldn't checking for proto & IV_PROTO_REQUEST_PUSH suffice? Any 2.3/2.4 client will only ever send "2", and newer clients will know this is a bitfield now. This works fine, but - in particular without an explaining comment - first checking for a value, then continue as a bitfield is somewhat surprising. > +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(, "IV_PLAT=win\n"); > #endif > > -/* support for P_DATA_V2 */ > -buf_printf(, "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) This needs a space behind the if. > +{ > +iv_proto |= IV_PROTO_REQUEST_PUSH; > +} > + > +buf_printf(, "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..86fb6853 100644 > --- a/src/openvpn/ssl.h > +++ b/src/openvpn/ssl.h > @@ -99,6 +99,13 @@ > /* 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 */ > +#define IV_PROTO_DATA_V2(1<<1) /**< Support P_DATA_V2 */ > +#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push > + * request and server does not need > + * to wait for a push-request to > send > + * a push-reply */ > + Style nit: would this not be more readable if you place the (rather long) comment above the value, instead of behind? Also: any particular reason to no use 1<<0 ? If we should not use that for some reason, let's mark it as reserved. Otherwise: maybe just use it? -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/9] Indicate that a client is in pull mode in IV_PROTO
Hi, On 17/07/2020 15:47, Arne Schwabe wrote: > This allows us to skip waiting for the first PUSH_REQUEST message from > the client to send the response. > > Signed-off-by: Arne Schwabe > --- > src/openvpn/multi.c | 12 ++-- > src/openvpn/ssl.c | 15 +-- > src/openvpn/ssl.h | 7 +++ > 3 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 0095c871..88ba9db2 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c) > { > int proto = 0; > int r = sscanf(optstr, "IV_PROTO=%d", ); > -if ((r == 1) && (proto >= 2)) > +if (r == 1) > { > -tls_multi->use_peer_id = true; > +if (proto >= 2) I thought it was agreed (but I may be wrong) to substitute this check with a bitwise AND, since this variable is now treated as a bitfield. Regards, > +{ > +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(, "IV_PLAT=win\n"); > #endif > > -/* support for P_DATA_V2 */ > -buf_printf(, "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(, "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..86fb6853 100644 > --- a/src/openvpn/ssl.h > +++ b/src/openvpn/ssl.h > @@ -99,6 +99,13 @@ > /* 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 */ > +#define IV_PROTO_DATA_V2(1<<1) /**< Support P_DATA_V2 */ > +#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push > + * request and server does not need > + * to wait for a push-request to > send > + * a push-reply */ > + > /* Default field in X509 to be username */ > #define X509_USERNAME_FIELD_DEFAULT "CN" > > -- Antonio Quartulli ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 1/9] Indicate that a client is in pull mode in IV_PROTO
This allows us to skip waiting for the first PUSH_REQUEST message from the client to send the response. Signed-off-by: Arne Schwabe --- src/openvpn/multi.c | 12 ++-- src/openvpn/ssl.c | 15 +-- src/openvpn/ssl.h | 7 +++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 0095c871..88ba9db2 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c) { int proto = 0; int r = sscanf(optstr, "IV_PROTO=%d", ); -if ((r == 1) && (proto >= 2)) +if (r == 1) { -tls_multi->use_peer_id = true; +if (proto >= 2) +{ +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(, "IV_PLAT=win\n"); #endif -/* support for P_DATA_V2 */ -buf_printf(, "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(, "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..86fb6853 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -99,6 +99,13 @@ /* 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 */ +#define IV_PROTO_DATA_V2(1<<1) /**< Support P_DATA_V2 */ +#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push + * request and server does not need + * to wait for a push-request to send + * a push-reply */ + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" -- 2.26.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel