| Message ID | 20200709101603.11941-5-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/8] Deprecate ncp-disable and add improved ncp to Changes.rst | expand |
possible white-space error ? On 09/07/2020 11:16, Arne Schwabe wrote: > The simplify the control flow, it makes more sense to generate the > data keys when all the prerequisites for generating the data channel > keys (ncp cipher selection etc) are met instead of delaying it to the > next incoming PUSH_REQUEST message. > > This also eliminates the need for the hack introduced by commit > 3b06b57d9 to generate the data channel keys on the async file close > event. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > src/openvpn/multi.c | 54 ++++++++++++++++++++++++++------------------- > src/openvpn/push.c | 27 ++++------------------- > 2 files changed, 35 insertions(+), 46 deletions(-) > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index f04c4c90..810e489a 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -1843,6 +1843,30 @@ multi_client_set_protocol_options(struct context *c) > } > } > > +/** > + * Generates the data channel keys > + */ > +static bool > +multi_client_generate_tls_keys(struct context *c) > +{ > + struct frame *frame_fragment = NULL; > +#ifdef ENABLE_FRAGMENT > + if (c->options.ce.fragment) > + { > + frame_fragment = &c->c2.frame_fragment; > + } > +#endif > + struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; > + if (!tls_session_update_crypto_params(session, &c->options, > + &c->c2.frame, frame_fragment)) > + { > + msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); > + register_signal(c, SIGUSR1, "process-push-msg-failed"); > + return false; > + } > + > + return true; > +} > > /* > * Called as soon as the SSL/TLS connection authenticates. > @@ -2149,7 +2173,13 @@ script_failed: > /* authentication complete, calculate dynamic client specific options */ > multi_client_set_protocol_options(&mi->context); > > - /* send push reply if ready*/ > + /* Generate data channel keys */ > + if (!multi_client_generate_tls_keys(&mi->context)) > + { > + mi->context.c2.context_auth = CAS_FAILED; > + } > + > + /* send push reply if ready */ > if (mi->context.c2.push_request_received) > { > process_incoming_push_request(&mi->context); > @@ -2205,28 +2235,6 @@ multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags) > { > /* continue authentication, perform NCP negotiation and send push_reply */ > multi_process_post(m, mi, mpp_flags); > - > - /* With NCP and deferred authentication, we perform cipher negotiation and > - * data channel keys generation on incoming push request, assuming that auth > - * succeeded. When auth succeeds in between push requests and async push is used, > - * we send push reply immediately. Above multi_process_post() call performs > - * NCP negotiation and here we do keys generation. */ > - > - struct context *c = &mi->context; > - struct frame *frame_fragment = NULL; > -#ifdef ENABLE_FRAGMENT > - if (c->options.ce.fragment) > - { > - frame_fragment = &c->c2.frame_fragment; > - } > -#endif > - struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; > - if (!tls_session_update_crypto_params(session, &c->options, > - &c->c2.frame, frame_fragment)) > - { > - msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); > - register_signal(c, SIGUSR1, "init-data-channel-failed"); > - } > } > else > { > diff --git a/src/openvpn/push.c b/src/openvpn/push.c > index 92a28a14..5bc4328c 100644 > --- a/src/openvpn/push.c > +++ b/src/openvpn/push.c > @@ -359,30 +359,11 @@ incoming_push_message(struct context *c, const struct buffer *buffer) > } > event_timeout_clear(&c->c2.push_request_interval); > } > - else if (status == PUSH_MSG_REQUEST) > - { > - if (c->options.mode == MODE_SERVER) > - { > - struct frame *frame_fragment = NULL; > -#ifdef ENABLE_FRAGMENT > - if (c->options.ce.fragment) > - { > - frame_fragment = &c->c2.frame_fragment; > - } > -#endif > - struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; > - if (!tls_session_update_crypto_params(session, &c->options, > - &c->c2.frame, frame_fragment)) > - { > - msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); > - goto error; > - } > - } > - } > > goto cleanup; > + > error: > - register_signal(c, SIGUSR1, "process-push-msg-failed"); > + register_signal(c, SIGUSR1, "process-push-msg-failed"); White-space ? just making a note of it > cleanup: > gc_free(&gc); > } > @@ -748,7 +729,6 @@ process_incoming_push_request(struct context *c) > { > int ret = PUSH_MSG_ERROR; > > - c->c2.push_request_received = true; > if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED) > { > const char *client_reason = tls_client_reason(c->c2.tls_multi); > @@ -810,7 +790,7 @@ push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt) > static int > process_incoming_push_reply(struct context *c, > unsigned int permission_mask, > - const unsigned int *option_types_found, > + unsigned int *option_types_found, > struct buffer *buf) > { > int ret = PUSH_MSG_ERROR; > @@ -875,6 +855,7 @@ process_incoming_push_msg(struct context *c, > > if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) > { > + c->c2.push_request_received = true; > return process_incoming_push_request(c); > } > else if (honor_received_options >
Acked-by: Gert Doering <gert@greenie.muc.de> Antonio and I have stared at the code. It has stared back. And it makes sense (AND it reduces quite a bit of code!) - *and* it passes all client and server side tests I have, including async-auth pam. I have fixed the whitespace mistake, and skipped the "const" hunk (that we already did in 2/8). Your patch has been applied to the master branch. commit 07560d9ed11a4bfdd7f2446f2c6ff854ee091154 Author: Arne Schwabe Date: Thu Jul 9 12:16:00 2020 +0200 Generate data channel keys after connect options have been parsed Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20200709101603.11941-5-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20253.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f04c4c90..810e489a 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -1843,6 +1843,30 @@ multi_client_set_protocol_options(struct context *c) } } +/** + * Generates the data channel keys + */ +static bool +multi_client_generate_tls_keys(struct context *c) +{ + struct frame *frame_fragment = NULL; +#ifdef ENABLE_FRAGMENT + if (c->options.ce.fragment) + { + frame_fragment = &c->c2.frame_fragment; + } +#endif + struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; + if (!tls_session_update_crypto_params(session, &c->options, + &c->c2.frame, frame_fragment)) + { + msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); + register_signal(c, SIGUSR1, "process-push-msg-failed"); + return false; + } + + return true; +} /* * Called as soon as the SSL/TLS connection authenticates. @@ -2149,7 +2173,13 @@ script_failed: /* authentication complete, calculate dynamic client specific options */ multi_client_set_protocol_options(&mi->context); - /* send push reply if ready*/ + /* Generate data channel keys */ + if (!multi_client_generate_tls_keys(&mi->context)) + { + mi->context.c2.context_auth = CAS_FAILED; + } + + /* send push reply if ready */ if (mi->context.c2.push_request_received) { process_incoming_push_request(&mi->context); @@ -2205,28 +2235,6 @@ multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags) { /* continue authentication, perform NCP negotiation and send push_reply */ multi_process_post(m, mi, mpp_flags); - - /* With NCP and deferred authentication, we perform cipher negotiation and - * data channel keys generation on incoming push request, assuming that auth - * succeeded. When auth succeeds in between push requests and async push is used, - * we send push reply immediately. Above multi_process_post() call performs - * NCP negotiation and here we do keys generation. */ - - struct context *c = &mi->context; - struct frame *frame_fragment = NULL; -#ifdef ENABLE_FRAGMENT - if (c->options.ce.fragment) - { - frame_fragment = &c->c2.frame_fragment; - } -#endif - struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; - if (!tls_session_update_crypto_params(session, &c->options, - &c->c2.frame, frame_fragment)) - { - msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); - register_signal(c, SIGUSR1, "init-data-channel-failed"); - } } else { diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 92a28a14..5bc4328c 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -359,30 +359,11 @@ incoming_push_message(struct context *c, const struct buffer *buffer) } event_timeout_clear(&c->c2.push_request_interval); } - else if (status == PUSH_MSG_REQUEST) - { - if (c->options.mode == MODE_SERVER) - { - struct frame *frame_fragment = NULL; -#ifdef ENABLE_FRAGMENT - if (c->options.ce.fragment) - { - frame_fragment = &c->c2.frame_fragment; - } -#endif - struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; - if (!tls_session_update_crypto_params(session, &c->options, - &c->c2.frame, frame_fragment)) - { - msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed"); - goto error; - } - } - } goto cleanup; + error: - register_signal(c, SIGUSR1, "process-push-msg-failed"); + register_signal(c, SIGUSR1, "process-push-msg-failed"); cleanup: gc_free(&gc); } @@ -748,7 +729,6 @@ process_incoming_push_request(struct context *c) { int ret = PUSH_MSG_ERROR; - c->c2.push_request_received = true; if (tls_authentication_status(c->c2.tls_multi, 0) == TLS_AUTHENTICATION_FAILED || c->c2.context_auth == CAS_FAILED) { const char *client_reason = tls_client_reason(c->c2.tls_multi); @@ -810,7 +790,7 @@ push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt) static int process_incoming_push_reply(struct context *c, unsigned int permission_mask, - const unsigned int *option_types_found, + unsigned int *option_types_found, struct buffer *buf) { int ret = PUSH_MSG_ERROR; @@ -875,6 +855,7 @@ process_incoming_push_msg(struct context *c, if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) { + c->c2.push_request_received = true; return process_incoming_push_request(c); } else if (honor_received_options
The simplify the control flow, it makes more sense to generate the data keys when all the prerequisites for generating the data channel keys (ncp cipher selection etc) are met instead of delaying it to the next incoming PUSH_REQUEST message. This also eliminates the need for the hack introduced by commit 3b06b57d9 to generate the data channel keys on the async file close event. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/multi.c | 54 ++++++++++++++++++++++++++------------------- src/openvpn/push.c | 27 ++++------------------- 2 files changed, 35 insertions(+), 46 deletions(-)