[Openvpn-devel,2/4] rework do_up() for correct order of DCO operations

Message ID	20220429170236.48239-3-kprovost@netgate.com
State	Superseded
Headers	show Return-Path: <openvpn-devel-bounces@lists.sourceforge.net> To: openvpn-devel <openvpn-devel@lists.sourceforge.net> Date: Fri, 29 Apr 2022 19:02:34 +0200 Message-Id: <20220429170236.48239-3-kprovost@netgate.com> In-Reply-To: <20220429170236.48239-1-kprovost@netgate.com> References: <20220429170236.48239-1-kprovost@netgate.com> MIME-Version: 1.0 preview: From: Kristof Provost <kp@FreeBSD.org> We must create the peer before we can dco_set_peer or dco_new_key. On the other hand, we must first process options, because those may change our peer id and we should create the peer with the correct [...] Content analysis details: (-0.2 points, 6.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [209.85.221.45 listed in list.dnswl.org] -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.221.45 listed in wl.mailspike.net] 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Subject: [Openvpn-devel] [PATCH 2/4] rework do_up() for correct order of DCO operations Precedence: list From: Kristof Provost via Openvpn-devel <openvpn-devel@lists.sourceforge.net> Reply-To: Kristof Provost <kprovost@netgate.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox
Series	[Openvpn-devel,1/4] Handle (DCO) timeouts in client mode \| expand [Openvpn-devel,1/4] Handle (DCO) timeouts in client mode [Openvpn-devel,2/4] rework do_up() for correct order of DCO operations [Openvpn-devel,3/4] Ensure dynamic_name is always populated [Openvpn-devel,4/4] ovpn-dco: introduce FreeBSD data-channel offload support

On 29 Apr 2022, at 19:02, Kristof Provost wrote: > From: Kristof Provost <kp@FreeBSD.org> > > We must create the peer before we can dco_set_peer or dco_new_key. > On the other hand, we must first process options, because those may > change our peer id and we should create the peer with the correct id. > > Split up do_deferred_options() in do_deferred_options() and > finish_options(). Call any DCO configuration operations (i.e. > dco_set_peer()/dco_new_key()) after we've created the peer (i.e. > dco_new_peer()). > > Signed-off-by: Kristof Provost <kprovost@netgate.com> > --- > src/openvpn/init.c | 112 > +++++++++++++++++++++++++------------------- > src/openvpn/init.h | 2 + > src/openvpn/multi.c | 2 + > 3 files changed, 68 insertions(+), 48 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index 8496e21f..5d53cf7e 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -2100,26 +2100,26 @@ do_up(struct context *c, bool pulled_options, > unsigned int option_types_found) > } > } > > - if ((c->mode == MODE_POINT_TO_POINT) && c->c2.did_open_tun) > + if (pulled_options) > { > - /* If we are in DCO mode we need to set the new peer > options now */ > - int ret = dco_p2p_add_new_peer(c); > - if (ret < 0) > + if (!do_deferred_options(c, option_types_found)) > { > - msg(D_DCO, "Cannot add peer to DCO: %s", > strerror(-ret)); > + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push > options"); > return false; > } > } > - > - if (pulled_options) > + if (c->mode == MODE_POINT_TO_POINT) > { > - if (!do_deferred_options(c, option_types_found)) > + /* If we are in DCO mode we need to set the new peer > options now */ > + int ret = dco_p2p_add_new_peer(c); > + if (ret < 0) > { > - msg(D_PUSH_ERRORS, "ERROR: Failed to apply push > options"); > + msg(D_DCO, "Cannot add peer to DCO: %s", > strerror(-ret)); > return false; > } > } > - else if (c->mode == MODE_POINT_TO_POINT) > + > + if (!pulled_options && c->mode == MODE_POINT_TO_POINT) > { > if (!do_deferred_p2p_ncp(c)) > { > @@ -2128,6 +2128,13 @@ do_up(struct context *c, bool pulled_options, > unsigned int option_types_found) > } > } > > + > + if (!finish_options(c)) > + { > + msg(D_TLS_ERRORS, "ERROR: Failed to finish option > processing"); > + return false; > + } > + > if (c->c2.did_open_tun) > { > c->c1.pulled_options_digest_save = > c->c2.pulled_options_digest; > @@ -2344,49 +2351,58 @@ do_deferred_options(struct context *c, const > unsigned int found) > { > return false; > } > - struct frame *frame_fragment = NULL; > + } > + > + return true; > +} > + > +bool > +finish_options(struct context *c) > +{ > + if (!c->options.pull || !dco_enabled(&c->options)) > + { > + return true; > + } > + > + struct frame *frame_fragment = NULL; > #ifdef ENABLE_FRAGMENT > - if (c->options.ce.fragment) > - { > - frame_fragment = &c->c2.frame_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(c->c2.tls_multi, > session, > - &c->options, > &c->c2.frame, > - frame_fragment, > - > get_link_socket_info(c))) > - { > - msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto > options"); > - return false; > - } > + struct tls_session *session = > &c->c2.tls_multi->session[TM_ACTIVE]; > + if (!tls_session_update_crypto_params(c->c2.tls_multi, session, > + &c->options, &c->c2.frame, > + frame_fragment, > + get_link_socket_info(c))) > + { > + msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto > options"); > + return false; > + } > > - if (dco_enabled(&c->options)) > - { > - /* Check if the pushed options are compatible with DCO if > we have > - * DCO enabled */ > - if (!check_dco_pull_options(&c->options)) > - { > - msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are > incompatible with " > - "data channel offload. Use --disable-dco to > connect" > - "to this server"); > - return false; > - } > + /* Check if the pushed options are compatible with DCO if we have > + * DCO enabled */ > + if (!check_dco_pull_options(&c->options)) > + { > + msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are > incompatible with " > + "data channel offload. Use --disable-dco to connect" > + "to this server"); > + return false; > + } > > - if (c->options.ping_send_timeout || c->c2.frame.mss_fix) > - { > - int ret = dco_set_peer(&c->c1.tuntap->dco, > - c->c2.tls_multi->peer_id, > - c->options.ping_send_timeout, > - c->options.ping_rec_timeout, > - c->c2.frame.mss_fix); > - if (ret < 0) > - { > - msg(D_DCO, "Cannot set DCO peer: %s", > strerror(-ret)); > - return false; > - } > - } > + if (c->options.ping_send_timeout || c->c2.frame.mss_fix) > + { > + int ret = dco_set_peer(&c->c1.tuntap->dco, > + c->c2.tls_multi->peer_id, > + c->options.ping_send_timeout, > + c->options.ping_rec_timeout, > + c->c2.frame.mss_fix); > + if (ret < 0) > + { > + msg(D_DCO, "Cannot set DCO peer: %s", strerror(-ret)); > + return false; > } > } > return true; > diff --git a/src/openvpn/init.h b/src/openvpn/init.h > index 1c341da3..5cc2a990 100644 > --- a/src/openvpn/init.h > +++ b/src/openvpn/init.h > @@ -97,6 +97,8 @@ void reset_coarse_timers(struct context *c); > > bool do_deferred_options(struct context *c, const unsigned int > found); > > +bool finish_options(struct context *c); > + > void inherit_context_child(struct context *dest, > const struct context *src); > > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 958712f1..47e1c6cc 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2452,6 +2452,8 @@ multi_client_connect_late_setup(struct > multi_context *m, > mi->context.c2.tls_multi->multi_state = CAS_FAILED; > } > > + finish_options(&mi->context); > + > /* send push reply if ready */ > if (mi->context.c2.push_request_received) > { > -- This patch isn’t quite right. It works well for DCO, but breaks non-DCO. It fails to call tls_session_update_crypto_params() if DCO is disabled. It needs this on top of it: commit ee846144a46b20b889ab1966246f789f3266c2f9 (HEAD -> tmp_v10) Author: Kristof Provost <kp@FreeBSD.org> Date: Thu May 5 19:36:13 2022 +0200 Fix finish_options() to perform the required work for non-DCO modes We still need to run tls_session_update_crypto_params(), even when DCO is not enabled. diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 7083b803..ab59cbd7 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2374,7 +2374,7 @@ do_deferred_options(struct context *c, const unsigned int found) bool finish_options(struct context *c) { - if (!c->options.pull || !dco_enabled(&c->options)) + if (!c->options.pull) { return true; } @@ -2399,7 +2399,7 @@ finish_options(struct context *c) /* Check if the pushed options are compatible with DCO if we have * DCO enabled */ - if (!check_dco_pull_options(&c->options)) + if (dco_enabled(&c->options) && !check_dco_pull_options(&c->options)) { msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are incompatible with " "data channel offload. Use --disable-dco to connect" @@ -2407,7 +2407,7 @@ finish_options(struct context *c) return false; } - if (c->options.ping_send_timeout || c->c2.frame.mss_fix) + if (dco_enabled(&c->options) && (c->options.ping_send_timeout || c->c2.frame.mss_fix)) { int ret = dco_set_peer(&c->c1.tuntap->dco, c->c2.tls_multi->peer_id, I’ll post an updated series in due course, but wanted to point this issue out already. Kristof <!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/xhtml; charset=utf-8"> </head> <body><div style="font-family: sans-serif;"><div class="markdown" style="white-space: normal;"> On 29 Apr 2022, at 19:02, Kristof Provost wrote: </div><div class="plaintext" style="white-space: normal;"><blockquote style="margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136BCE; color: #136BCE;">From: Kristof Provost <kp@FreeBSD.org> We must create the peer before we can dco_set_peer or dco_new_key. On the other hand, we must first process options, because those may change our peer id and we should create the peer with the correct id. Split up do_deferred_options() in do_deferred_options() and finish_options(). Call any DCO configuration operations (i.e. dco_set_peer()/dco_new_key()) after we've created the peer (i.e. dco_new_peer()). Signed-off-by: Kristof Provost <kprovost@netgate.com> --- src/openvpn/init.c | 112 +++++++++++++++++++++++++------------------- src/openvpn/init.h | 2 + src/openvpn/multi.c | 2 + 3 files changed, 68 insertions(+), 48 deletions(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 8496e21f..5d53cf7e 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2100,26 +2100,26 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found) } } - if ((c->mode == MODE_POINT_TO_POINT) && c->c2.did_open_tun) + if (pulled_options) { - /* If we are in DCO mode we need to set the new peer options now */ - int ret = dco_p2p_add_new_peer(c); - if (ret < 0) + if (!do_deferred_options(c, option_types_found)) { - msg(D_DCO, "Cannot add peer to DCO: %s", strerror(-ret)); + msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); return false; } } - - if (pulled_options) + if (c->mode == MODE_POINT_TO_POINT) { - if (!do_deferred_options(c, option_types_found)) + /* If we are in DCO mode we need to set the new peer options now */ + int ret = dco_p2p_add_new_peer(c); + if (ret < 0) { - msg(D_PUSH_ERRORS, "ERROR: Failed to apply push options"); + msg(D_DCO, "Cannot add peer to DCO: %s", strerror(-ret)); return false; } } - else if (c->mode == MODE_POINT_TO_POINT) + + if (!pulled_options && c->mode == MODE_POINT_TO_POINT) { if (!do_deferred_p2p_ncp(c)) { @@ -2128,6 +2128,13 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found) } } + + if (!finish_options(c)) + { + msg(D_TLS_ERRORS, "ERROR: Failed to finish option processing"); + return false; + } + if (c->c2.did_open_tun) { c->c1.pulled_options_digest_save = c->c2.pulled_options_digest; @@ -2344,49 +2351,58 @@ do_deferred_options(struct context *c, const unsigned int found) { return false; } - struct frame *frame_fragment = NULL; + } + + return true; +} + +bool +finish_options(struct context *c) +{ + if (!c->options.pull || !dco_enabled(&c->options)) + { + return true; + } + + struct frame *frame_fragment = NULL; #ifdef ENABLE_FRAGMENT - if (c->options.ce.fragment) - { - frame_fragment = &c->c2.frame_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(c->c2.tls_multi, session, - &c->options, &c->c2.frame, - frame_fragment, - get_link_socket_info(c))) - { - msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options"); - return false; - } + struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; + if (!tls_session_update_crypto_params(c->c2.tls_multi, session, + &c->options, &c->c2.frame, + frame_fragment, + get_link_socket_info(c))) + { + msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options"); + return false; + } - if (dco_enabled(&c->options)) - { - /* Check if the pushed options are compatible with DCO if we have - * DCO enabled */ - if (!check_dco_pull_options(&c->options)) - { - msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are incompatible with " - "data channel offload. Use --disable-dco to connect" - "to this server"); - return false; - } + /* Check if the pushed options are compatible with DCO if we have + * DCO enabled */ + if (!check_dco_pull_options(&c->options)) + { + msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are incompatible with " + "data channel offload. Use --disable-dco to connect" + "to this server"); + return false; + } - if (c->options.ping_send_timeout || c->c2.frame.mss_fix) - { - int ret = dco_set_peer(&c->c1.tuntap->dco, - c->c2.tls_multi->peer_id, - c->options.ping_send_timeout, - c->options.ping_rec_timeout, - c->c2.frame.mss_fix); - if (ret < 0) - { - msg(D_DCO, "Cannot set DCO peer: %s", strerror(-ret)); - return false; - } - } + if (c->options.ping_send_timeout || c->c2.frame.mss_fix) + { + int ret = dco_set_peer(&c->c1.tuntap->dco, + c->c2.tls_multi->peer_id, + c->options.ping_send_timeout, + c->options.ping_rec_timeout, + c->c2.frame.mss_fix); + if (ret < 0) + { + msg(D_DCO, "Cannot set DCO peer: %s", strerror(-ret)); + return false; } } return true; diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 1c341da3..5cc2a990 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -97,6 +97,8 @@ void reset_coarse_timers(struct context *c); bool do_deferred_options(struct context *c, const unsigned int found); +bool finish_options(struct context *c); + void inherit_context_child(struct context *dest, const struct context *src); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 958712f1..47e1c6cc 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2452,6 +2452,8 @@ multi_client_connect_late_setup(struct multi_context *m, mi->context.c2.tls_multi->multi_state = CAS_FAILED; } + finish_options(&mi->context); + /* send push reply if ready */ if (mi->context.c2.push_request_received) { -- </blockquote></div> <div class="markdown" style="white-space: normal;"> This patch isn’t quite right. It works well for DCO, but breaks non-DCO. It fails to call tls_session_update_crypto_params() if DCO is disabled. It needs this on top of it: <pre style="margin-left: 15px; margin-right: 15px; padding: 5px; border: thin solid gray; overflow-x: auto; max-width: 90vw; background-color: #E4E4E4;"><code>commit ee846144a46b20b889ab1966246f789f3266c2f9 (HEAD -> tmp_v10) Author: Kristof Provost <kp@FreeBSD.org> Date: Thu May 5 19:36:13 2022 +0200 Fix finish_options() to perform the required work for non-DCO modes We still need to run tls_session_update_crypto_params(), even when DCO is not enabled. diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 7083b803..ab59cbd7 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2374,7 +2374,7 @@ do_deferred_options(struct context *c, const unsigned int found) bool finish_options(struct context *c) { - if (!c->options.pull || !dco_enabled(&c->options)) + if (!c->options.pull) { return true; } @@ -2399,7 +2399,7 @@ finish_options(struct context *c) /* Check if the pushed options are compatible with DCO if we have * DCO enabled */ - if (!check_dco_pull_options(&c->options)) + if (dco_enabled(&c->options) && !check_dco_pull_options(&c->options)) { msg(D_TLS_ERRORS, "OPTIONS ERROR: pushed options are incompatible with " "data channel offload. Use --disable-dco to connect" @@ -2407,7 +2407,7 @@ finish_options(struct context *c) return false; } - if (c->options.ping_send_timeout || c->c2.frame.mss_fix) + if (dco_enabled(&c->options) && (c->options.ping_send_timeout || c->c2.frame.mss_fix)) { int ret = dco_set_peer(&c->c1.tuntap->dco, c->c2.tls_multi->peer_id, </code></pre> I’ll post an updated series in due course, but wanted to point this issue out already. Kristof </div></div></body> </html>

[Openvpn-devel,2/4] rework do_up() for correct order of DCO operations

Commit Message

Comments

Patch