| Message ID | 1537449154-26879-1-git-send-email-lstipakov@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v2] Refactor NCP-negotiable options handling | expand |
Hi, On 20-09-18 15:12, Lev Stipakov wrote: > From: Lev Stipakov <lev@openvpn.net> > > NCP negotiation can alter options. On reconnect > client sends possibly altered options while server > expects original values. This leads to warnings > in log and, if server uses --opt-verify, breaks > reconnect. > > Fix by decouple setting/unsetting NCP options from > the state of TLS context. At startup (and once per sighup) > we load original values to c->c1, which persists over > sigusr1 (restart). When tearing tunnel down we restore > (possibly altered) options back to original values. > > Trac: #1105 > > Signed-off-by: Lev Stipakov <lev@openvpn.net> > --- > v2: refined commit message > > src/openvpn/init.c | 40 +++++++++++++++++++++++++++++++--------- > 1 file changed, 31 insertions(+), 9 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index f432106..9353527 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -612,6 +612,33 @@ uninit_proxy(struct context *c) > uninit_proxy_dowork(c); > } > > +/* > + * Assign NCP-negotiable options to context->c1 > + * from context->options (initially config values). > + * They persist over sigusr1 restart. > + */ > +static void > +do_set_ncp_options(struct context *c) > +{ > + c->c1.ciphername = c->options.ciphername; > + c->c1.authname = c->options.authname; > + c->c1.keysize = c->options.keysize; > +} > + > +/* > + * Restore NCP-negotiable options from c->c1 to > + * c->options. The latter ones can be altered by > + * pushed options and therefore need to be restored > + * to original values on sigusr1 restart. > + */ > +static void > +do_unset_ncp_options(struct context *c) > +{ > + c->options.ciphername = c->c1.ciphername; > + c->options.authname = c->c1.authname; > + c->options.keysize = c->c1.keysize; > +} > + > void > context_init_1(struct context *c) > { > @@ -621,6 +648,8 @@ context_init_1(struct context *c) > > init_connection_list(c); > > + do_set_ncp_options(c); > + > #if defined(ENABLE_PKCS11) > if (c->first_time) > { > @@ -2593,10 +2622,6 @@ do_init_crypto_tls_c1(struct context *c) > /* initialize tls-auth/crypt key */ > do_init_tls_wrap_key(c); > > - c->c1.ciphername = options->ciphername; > - c->c1.authname = options->authname; > - c->c1.keysize = options->keysize; > - > #if 0 /* was: #if ENABLE_INLINE_FILES -- Note that enabling this code will break restarts */ > if (options->priv_key_file_inline) > { > @@ -2609,11 +2634,6 @@ do_init_crypto_tls_c1(struct context *c) > { > msg(D_INIT_MEDIUM, "Re-using SSL/TLS context"); > > - /* Restore pre-NCP cipher options */ > - c->options.ciphername = c->c1.ciphername; > - c->options.authname = c->c1.authname; > - c->options.keysize = c->c1.keysize; > - > /* > * tls-auth/crypt key can be configured per connection block, therefore > * we must reload it as it may have changed > @@ -4293,6 +4313,8 @@ close_instance(struct context *c) > /* free key schedules */ > do_close_free_key_schedule(c, (c->mode == CM_P2P || c->mode == CM_TOP)); > > + do_unset_ncp_options(c); > + > /* close TCP/UDP connection */ > do_close_link_socket(c); > > Finally got to testing this patch, and my testing confirms that this is the right fix. So: Acked-by: Steffan Karger <steffan@karger.me> -Steffan
Your patch has been applied to the master branch and release/2.4 branch.
I would appreciate a followup-patch, though, that explains a bit better
what "do_set_ncp_options()" *does* - basically "save the initial set
of config options into a storage space from where it can be recovered
later on, to get back the original options before NCP overwrote them"
- I understand that now (after asking Steffan) but the next one to
stumble across it will ask the same question...
commit 5fa25eeb7fefdbb17ad639d72fe46f393989159f (master)
commit d2ff5164e68e5101b1da2d2d818e23eb7851dc9f (release/2.4)
Author: Lev Stipakov
Date: Thu Sep 20 16:12:34 2018 +0300
Refactor NCP-negotiable options handling
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Steffan Karger <steffan.karger@fox-it.com>
Message-Id: <1537449154-26879-1-git-send-email-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17477.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index f432106..9353527 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -612,6 +612,33 @@ uninit_proxy(struct context *c) uninit_proxy_dowork(c); } +/* + * Assign NCP-negotiable options to context->c1 + * from context->options (initially config values). + * They persist over sigusr1 restart. + */ +static void +do_set_ncp_options(struct context *c) +{ + c->c1.ciphername = c->options.ciphername; + c->c1.authname = c->options.authname; + c->c1.keysize = c->options.keysize; +} + +/* + * Restore NCP-negotiable options from c->c1 to + * c->options. The latter ones can be altered by + * pushed options and therefore need to be restored + * to original values on sigusr1 restart. + */ +static void +do_unset_ncp_options(struct context *c) +{ + c->options.ciphername = c->c1.ciphername; + c->options.authname = c->c1.authname; + c->options.keysize = c->c1.keysize; +} + void context_init_1(struct context *c) { @@ -621,6 +648,8 @@ context_init_1(struct context *c) init_connection_list(c); + do_set_ncp_options(c); + #if defined(ENABLE_PKCS11) if (c->first_time) { @@ -2593,10 +2622,6 @@ do_init_crypto_tls_c1(struct context *c) /* initialize tls-auth/crypt key */ do_init_tls_wrap_key(c); - c->c1.ciphername = options->ciphername; - c->c1.authname = options->authname; - c->c1.keysize = options->keysize; - #if 0 /* was: #if ENABLE_INLINE_FILES -- Note that enabling this code will break restarts */ if (options->priv_key_file_inline) { @@ -2609,11 +2634,6 @@ do_init_crypto_tls_c1(struct context *c) { msg(D_INIT_MEDIUM, "Re-using SSL/TLS context"); - /* Restore pre-NCP cipher options */ - c->options.ciphername = c->c1.ciphername; - c->options.authname = c->c1.authname; - c->options.keysize = c->c1.keysize; - /* * tls-auth/crypt key can be configured per connection block, therefore * we must reload it as it may have changed @@ -4293,6 +4313,8 @@ close_instance(struct context *c) /* free key schedules */ do_close_free_key_schedule(c, (c->mode == CM_P2P || c->mode == CM_TOP)); + do_unset_ncp_options(c); + /* close TCP/UDP connection */ do_close_link_socket(c);