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 <a...@rfc2549.org>
> ---
> 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", &proto);
> - 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(&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)
This needs a space behind the if.
> + {
> + 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..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
