Re: [Openvpn-devel] [PATCH 1/9] Indicate that a client is in pull mode in IV_PROTO

2020-07-20 Thread Steffan Karger
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

2020-07-17 Thread Antonio Quartulli
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

2020-07-17 Thread Arne Schwabe
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