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 <a...@rfc2549.org> > --- > 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
