diff mbox series

[Openvpn-devel] Fix broken async push with NCP is used

Message ID 20200313165913.12682-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Fix broken async push with NCP is used | expand

Commit Message

Lev Stipakov March 13, 2020, 5:59 a.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

With NCP and deferred auth, we perform cipher negotiation and
generate data channel keys on incoming push request, assuming that auth
succeeded. With async push, when auth succeeds in between push requests,
we send push reply immediately.

The code which generates data channel keys is only called on handling incoming
push requests (incoming_push_message). It might not be called with NCP,
deferred auth and async push because on incoming push request auth might not
be complete yet. When auth is complete in between push requests, push reply
is sent and it is assumed that connection is established. However, since data channel keys
are not generated on the server side, connection doesn't work.

Fix by generating data channel keys when async push is triggered.

Reported-by: smaxfield@duosecurity.com
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/init.c  |  6 ++----
 src/openvpn/multi.c | 24 +++++++++++++++++++++++-
 src/openvpn/push.c  |  6 ++----
 src/openvpn/ssl.c   |  6 ++++++
 src/openvpn/ssl.h   |  5 +++--
 5 files changed, 36 insertions(+), 11 deletions(-)

Comments

Lev Stipakov March 13, 2020, 10:21 a.m. UTC | #1 
I forgot to add trac ticket to the commit message, so maybe @cron2
could do it, assuming v2 won't be needed.

Trac ticket with (lots of) context:
https://community.openvpn.net/openvpn/ticket/1259

pe 13. maalisk. 2020 klo 18.59 Lev Stipakov (lstipakov@gmail.com) kirjoitti:
>
> From: Lev Stipakov <lev@openvpn.net>
>
> With NCP and deferred auth, we perform cipher negotiation and
> generate data channel keys on incoming push request, assuming that auth
> succeeded. With async push, when auth succeeds in between push requests,
> we send push reply immediately.
>
> The code which generates data channel keys is only called on handling incoming
> push requests (incoming_push_message). It might not be called with NCP,
> deferred auth and async push because on incoming push request auth might not
> be complete yet. When auth is complete in between push requests, push reply
> is sent and it is assumed that connection is established. However, since data channel keys
> are not generated on the server side, connection doesn't work.
>
> Fix by generating data channel keys when async push is triggered.
>
> Reported-by: smaxfield@duosecurity.com
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/init.c  |  6 ++----
>  src/openvpn/multi.c | 24 +++++++++++++++++++++++-
>  src/openvpn/push.c  |  6 ++----
>  src/openvpn/ssl.c   |  6 ++++++
>  src/openvpn/ssl.h   |  5 +++--
>  5 files changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 824b6667..b3096dc8 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2343,10 +2343,8 @@ do_deferred_options(struct context *c, const unsigned int found)
>          }
>  #endif
>
> -        /* Do not regenerate keys if server sends an extra push reply */
> -        if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
> -            && !tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
> -                                                 frame_fragment))
> +        if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
> +                                              frame_fragment))
>          {
>              msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
>              return false;
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index da0aeeba..b42bcec9 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2137,8 +2137,30 @@ multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags)
>          {
>              if (mi)
>              {
> -                /* continue authentication and send push_reply */
> +                /* 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 71f22e94..aef00d34 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -298,10 +298,8 @@ incoming_push_message(struct context *c, const struct buffer *buffer)
>              }
>  #endif
>              struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
> -            /* Do not regenerate keys if client send a second push request */
> -            if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
> -                && !tls_session_update_crypto_params(session, &c->options,
> -                                                     &c->c2.frame, frame_fragment))
> +            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;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index e21279f1..56d0576a 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1956,6 +1956,12 @@ tls_session_update_crypto_params(struct tls_session *session,
>                                   struct options *options, struct frame *frame,
>                                   struct frame *frame_fragment)
>  {
> +    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
> +    {
> +        /* keys already generated, nothing to do */
> +        return true;
> +    }
> +
>      if (!session->opt->server
>          && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
>          && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 3449d91e..f0a8ef54 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -474,7 +474,8 @@ void tls_update_remote_addr(struct tls_multi *multi,
>
>  /**
>   * Update TLS session crypto parameters (cipher and auth) and derive data
> - * channel keys based on the supplied options.
> + * channel keys based on the supplied options. Does nothing if keys are already
> + * generated.
>   *
>   * @param session         The TLS session to update.
>   * @param options         The options to use when updating session.
> @@ -482,7 +483,7 @@ void tls_update_remote_addr(struct tls_multi *multi,
>   *                        adjusted based on the selected cipher/auth).
>   * @param frame_fragment  The fragment frame options.
>   *
> - * @return true if updating succeeded, false otherwise.
> + * @return true if updating succeeded or keys are already generated, false otherwise.
>   */
>  bool tls_session_update_crypto_params(struct tls_session *session,
>                                        struct options *options,
> --
> 2.17.1
>
Kristof Provost via Openvpn-devel March 27, 2020, 3:34 a.m. UTC | #2 
Is it possible to get an estimate on a timeline for this fix being released? Duo has a few customers that have been impacted by this and, while we have provided them with workarounds in the meantime, they are asking for updates on when this will be fixed. I also expect more customers will be impacted as time goes on as many are installing OpenVPN from a package repository where the OpenVPN package available there was built with --enable-async-push (e.g. RHEL 8 from EPEL). Since NCP is on by default, this is essentially resulting in broken behavior out-of-the-box for anyone installing OpenVPN 2.4 from a package repo and using a plugin that does a deferred authentication. 

Thanks!

Spencer

On 3/13/20, 5:22 PM, "Lev Stipakov" <lstipakov@gmail.com> wrote:

    I forgot to add trac ticket to the commit message, so maybe @cron2
    could do it, assuming v2 won't be needed.
    
    Trac ticket with (lots of) context:
    https://community.openvpn.net/openvpn/ticket/1259
    
    pe 13. maalisk. 2020 klo 18.59 Lev Stipakov (lstipakov@gmail.com) kirjoitti:
    >

    > From: Lev Stipakov <lev@openvpn.net>

    >

    > With NCP and deferred auth, we perform cipher negotiation and

    > generate data channel keys on incoming push request, assuming that auth

    > succeeded. With async push, when auth succeeds in between push requests,

    > we send push reply immediately.

    >

    > The code which generates data channel keys is only called on handling incoming

    > push requests (incoming_push_message). It might not be called with NCP,

    > deferred auth and async push because on incoming push request auth might not

    > be complete yet. When auth is complete in between push requests, push reply

    > is sent and it is assumed that connection is established. However, since data channel keys

    > are not generated on the server side, connection doesn't work.

    >

    > Fix by generating data channel keys when async push is triggered.

    >

    > Reported-by: smaxfield@duosecurity.com

    > Signed-off-by: Lev Stipakov <lev@openvpn.net>

    > ---

    >  src/openvpn/init.c  |  6 ++----

    >  src/openvpn/multi.c | 24 +++++++++++++++++++++++-

    >  src/openvpn/push.c  |  6 ++----

    >  src/openvpn/ssl.c   |  6 ++++++

    >  src/openvpn/ssl.h   |  5 +++--

    >  5 files changed, 36 insertions(+), 11 deletions(-)

    >

    > diff --git a/src/openvpn/init.c b/src/openvpn/init.c

    > index 824b6667..b3096dc8 100644

    > --- a/src/openvpn/init.c

    > +++ b/src/openvpn/init.c

    > @@ -2343,10 +2343,8 @@ do_deferred_options(struct context *c, const unsigned int found)

    >          }

    >  #endif

    >

    > -        /* Do not regenerate keys if server sends an extra push reply */

    > -        if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized

    > -            && !tls_session_update_crypto_params(session, &c->options, &c->c2.frame,

    > -                                                 frame_fragment))

    > +        if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,

    > +                                              frame_fragment))

    >          {

    >              msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");

    >              return false;

    > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c

    > index da0aeeba..b42bcec9 100644

    > --- a/src/openvpn/multi.c

    > +++ b/src/openvpn/multi.c

    > @@ -2137,8 +2137,30 @@ multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags)

    >          {

    >              if (mi)

    >              {

    > -                /* continue authentication and send push_reply */

    > +                /* 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 71f22e94..aef00d34 100644

    > --- a/src/openvpn/push.c

    > +++ b/src/openvpn/push.c

    > @@ -298,10 +298,8 @@ incoming_push_message(struct context *c, const struct buffer *buffer)

    >              }

    >  #endif

    >              struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];

    > -            /* Do not regenerate keys if client send a second push request */

    > -            if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized

    > -                && !tls_session_update_crypto_params(session, &c->options,

    > -                                                     &c->c2.frame, frame_fragment))

    > +            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;

    > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c

    > index e21279f1..56d0576a 100644

    > --- a/src/openvpn/ssl.c

    > +++ b/src/openvpn/ssl.c

    > @@ -1956,6 +1956,12 @@ tls_session_update_crypto_params(struct tls_session *session,

    >                                   struct options *options, struct frame *frame,

    >                                   struct frame *frame_fragment)

    >  {

    > +    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)

    > +    {

    > +        /* keys already generated, nothing to do */

    > +        return true;

    > +    }

    > +

    >      if (!session->opt->server

    >          && 0 != strcmp(options->ciphername, session->opt->config_ciphername)

    >          && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))

    > diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h

    > index 3449d91e..f0a8ef54 100644

    > --- a/src/openvpn/ssl.h

    > +++ b/src/openvpn/ssl.h

    > @@ -474,7 +474,8 @@ void tls_update_remote_addr(struct tls_multi *multi,

    >

    >  /**

    >   * Update TLS session crypto parameters (cipher and auth) and derive data

    > - * channel keys based on the supplied options.

    > + * channel keys based on the supplied options. Does nothing if keys are already

    > + * generated.

    >   *

    >   * @param session         The TLS session to update.

    >   * @param options         The options to use when updating session.

    > @@ -482,7 +483,7 @@ void tls_update_remote_addr(struct tls_multi *multi,

    >   *                        adjusted based on the selected cipher/auth).

    >   * @param frame_fragment  The fragment frame options.

    >   *

    > - * @return true if updating succeeded, false otherwise.

    > + * @return true if updating succeeded or keys are already generated, false otherwise.

    >   */

    >  bool tls_session_update_crypto_params(struct tls_session *session,

    >                                        struct options *options,

    > --

    > 2.17.1

    >

    
    
    -- 
    -Lev
    
    
    _______________________________________________
    Openvpn-devel mailing list
    Openvpn-devel@lists.sourceforge.net
    https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Arne Schwabe April 15, 2020, 9:24 a.m. UTC | #3 
Am 13.03.20 um 17:59 schrieb Lev Stipakov:
> From: Lev Stipakov <lev@openvpn.net>
> 
> With NCP and deferred auth, we perform cipher negotiation and
> generate data channel keys on incoming push request, assuming that auth
> succeeded. With async push, when auth succeeds in between push requests,
> we send push reply immediately.
> 
> The code which generates data channel keys is only called on handling incoming
> push requests (incoming_push_message). It might not be called with NCP,
> deferred auth and async push because on incoming push request auth might not
> be complete yet. When auth is complete in between push requests, push reply
> is sent and it is assumed that connection is established. However, since data channel keys
> are not generated on the server side, connection doesn't work.
> 
> Fix by generating data channel keys when async push is triggered.
> 
> Reported-by: smaxfield@duosecurity.com
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  src/openvpn/init.c  |  6 ++----
>  src/openvpn/multi.c | 24 +++++++++++++++++++++++-
>  src/openvpn/push.c  |  6 ++----
>  src/openvpn/ssl.c   |  6 ++++++
>  src/openvpn/ssl.h   |  5 +++--
>  5 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 824b6667..b3096dc8 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2343,10 +2343,8 @@ do_deferred_options(struct context *c, const unsigned int found)
>          }
>  #endif
>  
> -        /* Do not regenerate keys if server sends an extra push reply */
> -        if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
> -            && !tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
> -                                                 frame_fragment))
> +        if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
> +                                              frame_fragment))
>          {
>              msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
>              return false;
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index da0aeeba..b42bcec9 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2137,8 +2137,30 @@ multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags)
>          {
>              if (mi)
>              {
> -                /* continue authentication and send push_reply */
> +                /* 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");
> +                }

I am not happy to see this code fragment being copied here and having
the same code now three times almost identical.

This is the point where we should put it into its own fuction.



Otherwise the code looks good.
So

Semi-Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering April 15, 2020, 9:47 p.m. UTC | #4 
Your patch has been applied to the master and release/2.4 branch (bugfix).

I have read the semi-ACK from Arne, discussed with Lev, and we decided to 
go for "we'll merge this simple change now, so that the bug is fixed, and 
then we can have a longer discussion on how to make this code nicer"
(refactoring).  Did some review on my own, code looks good, explanation
makes sense, so "this is the right fix".

Done some minimal testing on Linux (t_client), just to see we didn't
break something big.  I do not have an async-cc server testbed today,
but it seems I need to add one (note to self!)...

As a side note: the patch does two things, one is "add generation of
data channel keys when async push is triggered", and also a bit of
refactoring, moving the (numerous) "only do channel keys if they have 
not been done already" checks into tls_session_update_crypto_params()
(added a note to that extent to the commit message).

Trac reference to 1259 added to commit message.

commit 3b06b57d9f1d972ec16f0893d06697439c1bb1fe (master)
commit 9bb285e3e63fb6d716923e0353436bb8d8e89313 (release/2.4)
Author: Lev Stipakov
Date:   Fri Mar 13 18:59:13 2020 +0200

     Fix broken async push with NCP is used

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200313165913.12682-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19553.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 824b6667..b3096dc8 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2343,10 +2343,8 @@  do_deferred_options(struct context *c, const unsigned int found)
         }
 #endif
 
-        /* Do not regenerate keys if server sends an extra push reply */
-        if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
-            && !tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
-                                                 frame_fragment))
+        if (!tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
+                                              frame_fragment))
         {
             msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
             return false;
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index da0aeeba..b42bcec9 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2137,8 +2137,30 @@  multi_process_file_closed(struct multi_context *m, const unsigned int mpp_flags)
         {
             if (mi)
             {
-                /* continue authentication and send push_reply */
+                /* 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 71f22e94..aef00d34 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -298,10 +298,8 @@  incoming_push_message(struct context *c, const struct buffer *buffer)
             }
 #endif
             struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-            /* Do not regenerate keys if client send a second push request */
-            if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
-                && !tls_session_update_crypto_params(session, &c->options,
-                                                     &c->c2.frame, frame_fragment))
+            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;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index e21279f1..56d0576a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1956,6 +1956,12 @@  tls_session_update_crypto_params(struct tls_session *session,
                                  struct options *options, struct frame *frame,
                                  struct frame *frame_fragment)
 {
+    if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+    {
+        /* keys already generated, nothing to do */
+        return true;
+    }
+
     if (!session->opt->server
         && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
         && !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 3449d91e..f0a8ef54 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -474,7 +474,8 @@  void tls_update_remote_addr(struct tls_multi *multi,
 
 /**
  * Update TLS session crypto parameters (cipher and auth) and derive data
- * channel keys based on the supplied options.
+ * channel keys based on the supplied options. Does nothing if keys are already
+ * generated.
  *
  * @param session         The TLS session to update.
  * @param options         The options to use when updating session.
@@ -482,7 +483,7 @@  void tls_update_remote_addr(struct tls_multi *multi,
  *                        adjusted based on the selected cipher/auth).
  * @param frame_fragment  The fragment frame options.
  *
- * @return true if updating succeeded, false otherwise.
+ * @return true if updating succeeded or keys are already generated, false otherwise.
  */
 bool tls_session_update_crypto_params(struct tls_session *session,
                                       struct options *options,