Re: [Openvpn-devel] [PATCH v2 5/9] Remove key-method 1

[David S. <op...@sf...>]

On 17/07/2020 15:47, Arne Schwabe wrote:
> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
> 
> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
>           v1 op codes and give a good warning message if we encounter
>           them in the legal op codes pre-check.
> 
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
>  doc/doxygen/doc_control_processor.h |   6 +-
>  doc/doxygen/doc_key_generation.h    |   6 +-
>  doc/doxygen/doc_protocol_overview.h |   2 +-
>  src/openvpn/forward.c               |   2 +-
>  src/openvpn/helper.c                |   5 -
>  src/openvpn/init.c                  |   1 -
>  src/openvpn/options.c               |  35 +----
>  src/openvpn/options.h               |   4 -
>  src/openvpn/ssl.c                   | 230 ++++------------------------
>  src/openvpn/ssl.h                   |  19 +--
>  src/openvpn/ssl_common.h            |   1 -
>  11 files changed, 42 insertions(+), 269 deletions(-)

[...snip...]

> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 00b97352..4144217d 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c

[...snip...]

> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
>      return str;
>  }
>  
> -/*
> - * Handle the reading and writing of key data to and from
> - * the TLS control channel (cleartext).
> - */

Is it needed to remove this comment?  Or move it after the push_peer_info()
function?

> -static bool
> -key_method_1_write(struct buffer *buf, struct tls_session *session)
> -{
[...snip...]
> -}
> -
>  static bool
>  push_peer_info(struct buffer *buf, struct tls_session *session)
>  {
> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>           * 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)
> +        if (session->opt->pull)
>          {
>              iv_proto |= IV_PROTO_REQUEST_PUSH;
>          }
> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)

[...snip...]

> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>              }
>  
>              /* hard reset ? */
> -            if (is_hard_reset(op, 0))
> +            if (is_hard_reset_method2(op))
>              {
>                  /* verify client -> server or server -> client connection */
> -                if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
> -                      || op == P_CONTROL_HARD_RESET_CLIENT_V2
> +                if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>                        || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
> -                    || ((op == P_CONTROL_HARD_RESET_SERVER_V1
> -                         || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
> +                    || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>                  {
>                      msg(D_TLS_ERRORS,
>                          "TLS Error: client->client or server->server connection attempted from %s",
> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>               * Initial packet received.
>               */
>  
> -            if (i == TM_SIZE && is_hard_reset(op, 0))
> +            if (i == TM_SIZE && is_hard_reset_method2(op))
>              {
>                  struct tls_session *session = &multi->session[TM_ACTIVE];
>                  struct key_state *ks = &session->key[KS_PRIMARY];
>  
> -                if (!is_hard_reset(op, multi->opt.key_method))
> -                {
> -                    msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
> -                        multi->opt.key_method,
> -                        packet_opcode_name(op));
> -                    goto error;
> -                }
> -
>                  /*
>                   * If we have no session currently in progress, the initial packet will
>                   * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
>                  }
>              }
>  
> -            if (i == TM_SIZE && is_hard_reset(op, 0))
> +            if (i == TM_SIZE && is_hard_reset_method2(op))

Do we need this if() block?  Considering it is exactly the same if() statement
as the previous if() block.  I would suggest merging them, but you might hit a
challenge with reuse of struct tls_session *session, where one is referring to
TM_ACTIVE vs TM_UNTRUSTED.

>              {
>                  /*
>                   * No match with existing sessions,
> @@ -3614,14 +3450,6 @@ tls_pre_decrypt(struct tls_multi *multi,
>                      goto error;
>                  }
>  
> -                if (!is_hard_reset(op, multi->opt.key_method))
> -                {
> -                    msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
> -                        multi->opt.key_method,
> -                        packet_opcode_name(op));
> -                    goto error;
> -                }
> -
>                  if (!read_control_auth(buf, &session->tls_wrap, from,
>                                         session->opt))
>                  {

I had already started my own approach of removing --key-method when I was made
aware of this patch.  Considering the size of it, my own barely tested changes
is about 90% similar to this patch.  But this patch has some improvements I
didn't consider in my work.

I've done a quick 'make check' as well, and it works fine.  There are just a
couple of minor things to consider, otherwise I will happily ACK this patch.


-- 
kind regards,

David Sommerseth
OpenVPN Inc